Simon Horman <horms at verge.net.au> writes:
> On Mon, Feb 01, 2010 at 08:08:51AM -0800, Eric W. Biederman wrote:
>> Simon Horman <horms at verge.net.au> writes:
>>
>> > On Mon, Jan 25, 2010 at 12:14:03AM -0800, Eric W. Biederman wrote:
>> >> Simon Horman <horms at verge.net.au> writes:
>> >>
>> >> > On Tue, Jan 19, 2010 at 12:05:03AM -0800, Eric W. Biederman wrote:
>> >> >>
>> >> >> A colleague of mine implemented kdump and it used --reuse-cmdline
>> >> >> with some rather interesting and unexpected results.
>> >> >>
>> >> >> Update the getopt specification so that --reuse-cmdline does not
>> >> >> attempt to take an argument that it will not use.
>> >> >>
>> >> >> Update the processing of --append so that --reuse-cmdline followed
>> >> >> by --append actually appends the parameters specified by --reuse-cmdline.
>> >> >
>> >> > Hi Eric,
>> >> >
>> >> > sorry for being slow. Been semi-offline for LCA and am now catching
>> >> > up on things.
>> >>
>> >> No problem, I am pretty out of it right now as well.
>> >>
>> >> > [snip]
>> >> >
>> >> >> diff --git a/kexec/kexec.c b/kexec/kexec.c
>> >> >> index a1cec86..f4c22a6 100644
>> >> >> --- a/kexec/kexec.c
>> >> >> +++ b/kexec/kexec.c
>> >> >> @@ -994,6 +994,22 @@ void check_reuse_initrd(void)
>> >> >> free(line);
>> >> >> }
>> >> >>
>> >> >> +const char *concat_cmdline(const char *base, const char *append)
>> >> >> +{
>> >> >> + const char *cmdline;
>> >> >> + if (!base && !append)
>> >> >> + return NULL;
>> >> >> + if (!base)
>> >> >> + return append;
>> >> >> + if (!append)
>> >> >> + return base;
>> >> >> + cmdline = xmalloc(strlen(base) + 1 + strlen(append) + 1);
>> >> >> + strcpy(cmdline, base);
>> >> >> + strcat(cmdline, " ");
>> >> >> + strcat(cmdline, append);
>> >> >> + return cmdline;
>> >> >> +}
>> >> >> +
>> >> >
>> >> > This introduces a memory leak.
>> >>
>> >> Yep.
>> >>
>> >> > Perhaps it should strdup append and base in the !base and !append cases
>> >> > respectively and expect the caller to always call free.
>> >> >
>> >> > I realise that its a small leak in a programme that will soon exit anyway.
>> >> > But for the sake of being able to use tools like valgrind to analyse
>> >> > problems it seems to me that leaks are worth avoiding. (Not that I have
>> >> > run valgrind on kexec-tools to see what happens :-)
>> >>
>> >> I see your point but I think we already have a memory leak here (
>> >> Where does the memory that getopt uses come from? ), and I think on a
>> >> trivial application like /sbin/kexec that is simply not long running
>> >> it can't matter. I'm even willing to call not freeing memory
>> >> explicitly a performance optimization in cases like this ;)
>> >
>> > Clearly this is a matter of taste. And as it happens I fall on
>> > the side of the fence that thinks that the leak should be avoided.
>> >
>> > I propose applying the following after your patch:
>> >
>> > From: Simon Horman <horms at verge.net.au>
>> >
>> > don't leak in concat_cmdline
>>
>> It is a bit of a shame that we loose the const attributes.
>
> Indeed, though it seems to be at least partially broken in your original patch.
>
> # gcc --version
> gcc (Debian 4.4.2-8) 4.4.2
> Copyright (C) 2009 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions. There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>
> # make
> kexec/kexec.c: In function ?concat_cmdline?:
> kexec/kexec.c:1007: warning: passing argument 1 of ?strcpy? discards qualifiers from pointer target type
> /usr/include/string.h:127: note: expected ?char * __restrict__? but argument is of type ?const char *?
> kexec/kexec.c:1008: warning: passing argument 1 of ?strcat? discards qualifiers from pointer target type
> /usr/include/string.h:135: note: expected ?char * __restrict__? but argument is of type ?const char *?
> kexec/kexec.c:1009: warning: passing argument 1 of ?strcat? discards qualifiers from pointer target type
> /usr/include/string.h:135: note: expected ?char * __restrict__? but argument is of type ?const char *?
Odd. I did not see those errors, but those are definitely bugs in
concat_cmdline.
>> Beyond that the idiom
>> if (xyz)
>> free(xyz)
>> can just become:
>> free(xyz)
>
> concat_cmdline() may return NULL in the case where both
> base and append are NULL.
free is well defined when passed NULL. At least according to my local
manpages.
Eric
