On Mon, Feb 01, 2010 at 04:21:16PM -0800, Eric W. Biederman wrote: > Simon Horman <[email protected]> writes: > > > On Mon, Feb 01, 2010 at 08:08:51AM -0800, Eric W. Biederman wrote: > >> Simon Horman <[email protected]> writes: > >> > >> > On Mon, Jan 25, 2010 at 12:14:03AM -0800, Eric W. Biederman wrote: > >> >> Simon Horman <[email protected]> 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 <[email protected]> > >> > > >> > 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 [email protected] http://lists.infradead.org/mailman/listinfo/kexec
