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

[David Mair]

Hi Dave,

Here's my answers, I saw your other post and it looks like you are
seeing the same concept as I am anyway.

On 06/13/2014 07:59 AM, Dave Anderson wrote:
> 
> 
> ----- 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?  

It took an accidental kernel build outcome where vmlinux.gz was built
with code+text+debuginfo included and a debuginfo was created from it
anyway then the command line had something like:

crash vmlinux.gz vmlinux.debug vmdump

but crash had no authority to unlink vmlinux.debug and fail to unlink
the uncompressed vmlinux temporary regardless of the unexpectedly
unnecessary command-line elements.

> 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. 

That is normal in the cases I see most of the time so the symptoms were
unexpected (and uncommon to be fair).

>> 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()?

clean_exit() at this:

	if ((pc->namelist_orig) && file_exists(pc->namelist, NULL))
		unlink(pc->namelist);

pc->namelist_orig is vmlinux.gz
pc->namelist is vmlinux.debug
pc->namelist_debug is vmlinux_XXXXXX

where _XXXXXX is replaced with the unique name element. namelist_orig is
associated with what's currently in namelist_debug and the actual unlink
used should have been this:

	if ((pc->namelist_debug_orig) && file_exists(pc->namelist_debug, NULL))
		unlink(pc->namelist_debug);

pc->namelist_debug_orig is NULL at the point of failure.

> And then with your fix applied, why does the crash session still fail?

Because for the case described crash used the uncompressed vmlinux.gz as
namelist_debug for gdb and vmlinux.debug as namelist. The method of
detection of which file is debug info for gdb is safe most of the time
but you can create a kernel where the vmlinux is larger than the
vmlinux.debug and it causes crash to fail to start because the namelist
that gets used has no code.

-- 
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 at suse.com>
>> ---
>>  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 at redhat.com
>> https://www.redhat.com/mailman/listinfo/crash-utility
>>
> 
> --
> Crash-utility mailing list
> Crash-utility at redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility
>