Re: [PATCH] Fix --reuse-cmdline so it is usable.

[Simon Horman <horms@verge.net.au>]

On Mon, Feb 01, 2010 at 04:21:16PM -0800, Eric W. Biederman wrote:
> Simon Horman <horms@verge.net.au> writes:
> 
> > On Mon, Feb 01, 2010 at 08:08:51AM -0800, Eric W. Biederman wrote:
> >> Simon Horman <horms@verge.net.au> writes:
> >> 
> >> > On Mon, Jan 25, 2010 at 12:14:03AM -0800, Eric W. Biederman wrote:
> >> >> Simon Horman <horms@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@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.

Noted, I'll make the change you suggested.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec