[Crash-utility] [PATCH] Fix for some command-line cases an unintended unlink of a file crash didn't create and a leftover temporary
Dave Anderson
anderson at redhat.com
Mon Jun 16 14:16:29 UTC 2014
----- Original Message -----
> 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).
Right -- once I had my own reproducer, the problem became self-evident.
But I was still interested in how your particular case was possible.
Queued for crash-7.0.8:
https://github.com/crash-utility/crash/commit/13c0faff75cc12a322f4d16d3c4712f30399cdde
Thanks again,
Dave
> 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 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
> >>
> >
> > --
> > 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
>
More information about the Crash-utility
mailing list