Re: [PATCH] Fix for some command-line cases an unintended unlink of a file crash didn't create and a leftover temporary

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Dave,

On 06/13/2014 12:55 PM, Dave Anderson wrote:
> 
> 
> ----- Original Message -----
>>
>>
>> ----- Original Message -----
>>> When the crash command line includes two filenames where one is a
>>> compressed kernel and the other is a debuginfo file and they appear in
>>> that order then if the uncompressed temporary version of the kernel is
>>> actually larger than the debuginfo then crash will end with an error but
>>> will also unlink the debuginfo file and will not clean up the (intended
>>> temporary) uncompressed copy of the kernel.
>>
>> Hi Dave,
>>
>> It's taken me awhile to wrap my head around this, but my first question is:
>> how is it possible that the uncompressed stripped kernel can possibly be
>> larger than the debuginfo file?
>>
>> I only have old RHEL3 kernels as examples, but the .debug versions
>> of the kernel are 5 to 6 times larger than the stripped kernel.
>>
>>> This patch at least fixes the unintended unlink and leaving the
>>> temporary present. It doesn't fix the failure to start but that's
>>> because the wrong files are assumed the debuginfo and kernel. The size
>>> case that led to this discovery is probably rare.
>>
>> Is the unintended unlink() done in the first display_sys_stats() or
>> in clean_exit()?
>>
>> And then with your fix applied, why does the crash session still fail?
>>
>> Dave
> 
> Hi Dave,
> 
> Although I'm still interested in answers to the questions above, this
> patch also fixes a different scenario.
> 
> In testing the 8 possible combinations using RHEL3 vmlinux/vmlinux.gz
> and vmlinux.debug/vmlinux.debug.gz files, I found that I could reproduce
> the problem with this particular combination/order:
> 
>  $ crash vmlinux.debug.gz vmlinux ...
> 
> Similar to your setup, the pc->namelist_orig field continues to 
> be non-NULL, and therefore pc->namelist (vmlinux) gets removed in
> clean_exit(), and since pc->namelist_debug_orig doesn't get set,
> the uncompressed temporary debug file remains!

Yes, vmlinux.gz passes is_compressed_kernel() and there is a tmpname
(vmlinux.debug_XXXXXX with unique name replacement of XXXXXX).
select_namelist() gets called with tmpname (as the argument new) and all
the namelist values in pc are NULL. select_namelist() uses new to set
pc->namelist (to vmlinux.debug_XXXXXX). On return to main(),
pc->namelist_orig gets set to argv[optind] (vmlinux.debug.gz).

argv[optind] = vmlinux.debug.gz

// State before select_namelist()
pc->namelist is NULL
pc->namelist_orig is NULL
pc->namelist_debug is NULL
pc->namelist_debug_orig is NULL

select_namelist(vmlinux.debug_XXXXXX)

// State shortly after select_namelist()
pc->namelist is vmlinux.debug_XXXXXX
pc->namelist_orig is vmlinux.debug.gz
pc->namelist_debug is NULL
pc->namelist_debug_orig is NULL

The next argument, vmlinux, passes is_kernel() and select_namelist()
gets called with argv[optind] (as the argument new). Since there is a
namelist value already select_namelist() gets the struct stat for each
file (vmlinux.debug_XXXXXX and vmlinux, the first is a larger file).
select_namelist() copies pc->namelist (vmlinux.debug_XXXXXX) to
pc->namelist_debug and sets pc->namelist from new (vmlinux) leaving
pc->namelist_orig set to vmlinux.debug.gz and pc->namelist_debug_orig as
NULL.

argv[optind] = vmlinux

// State before select_namelist()
pc->namelist is vmlinux.debug_XXXXXX
pc->namelist_orig is vmlinux.debug.gz
pc->namelist_debug is NULL
pc->namelist_debug_orig is NULL

select_namelist(vmlinux)

// State shortly after select_namelist()
pc->namelist is vmlinux
pc->namelist_orig is vmlinux.debug.gz
pc->namelist_debug is vmlinux.debug_XXXXXX
pc->namelist_debug_orig is NULL

That leaves namelist_debug with no indication it is an uncompressed,
temporary file (the equivalent _orig member of pc is NULL) and indicates
vmlinux is an uncompressed temporary file (the equivalent _orig member
of pc is non-NULL) when clean_exit() is used so there is an unintended
unlink of vmlinux and an uncleaned vmlinux.debug_XXXXXX temporary.

> Nice fix!

Thanks, I'm glad to help.

-- 
David.

> 
>>
>>  
>>> The cause is that evidence of a temporary file to unlink is that there
>>> is a value in pc->namelist and pc->namelist_orig (or pc->namelist_debug
>>> and pc->namelist_orig_debug) but when the file size test in
>>> select_namelist() results in the pc->namelist copy to pc->namelist_debug
>>> the _orig is not copied as well so the implied file to unlink is the one
>>> set in pc->namelist (the debuginfo filename now).
>>>
>>> The patch causes a populated namelist_orig value to be swapped with
>>> namelist_debug_orig if the namelist value is copied to namelist_debug.
>>>
>>>     Signed-off-by: David Mair <dmair@xxxxxxxx>
>>> ---
>>>  symbols.c | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/symbols.c b/symbols.c
>>> index 4c6fbf4..e1ed719 100644
>>> --- a/symbols.c
>>> +++ b/symbols.c
>>> @@ -3520,6 +3520,7 @@ int
>>>  select_namelist(char *new)
>>>  {
>>>         struct stat stat1, stat2;
>>> +       char *namecp;
>>>
>>>         if (pc->server_namelist) {
>>>                 pc->namelist_debug = new;
>>> @@ -3533,6 +3534,12 @@ select_namelist(char *new)
>>>
>>>         if (stat1.st_size > stat2.st_size) {
>>>                 pc->namelist_debug = pc->namelist;
>>> +               if (pc->namelist_orig)
>>> +               {
>>> +                       namecp = pc->namelist_debug_orig;
>>> +                       pc->namelist_debug_orig = pc->namelist_orig;
>>> +                       pc->namelist_orig = namecp;
>>> +               }
>>>                 pc->namelist = new;
>>>         } else if (stat2.st_size > stat1.st_size)
>>>                 pc->namelist_debug = new;
>>>
>>> --
>>> Crash-utility mailing list
>>> Crash-utility@xxxxxxxxxx
>>> https://www.redhat.com/mailman/listinfo/crash-utility
>>>
>>
>> --
>> Crash-utility mailing list
>> Crash-utility@xxxxxxxxxx
>> https://www.redhat.com/mailman/listinfo/crash-utility
>>
> 
> --
> Crash-utility mailing list
> Crash-utility@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/crash-utility
> 

--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility




[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux