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.
Beyond that the idiom
if (xyz)
free(xyz)
can just become:
free(xyz)
Eric
> Signed-off-by: Simon Horman <horms at verge.net.au>
>
> Index: kexec-tools/kexec/arch/i386/kexec-bzImage.c
> ===================================================================
> --- kexec-tools.orig/kexec/arch/i386/kexec-bzImage.c 2010-02-01 23:07:15.000000000 +1100
> +++ kexec-tools/kexec/arch/i386/kexec-bzImage.c 2010-02-01 23:07:15.000000000 +1100
> @@ -332,8 +332,8 @@ int do_bzImage_load(struct kexec_info *i
> int bzImage_load(int argc, char **argv, const char *buf, off_t len,
> struct kexec_info *info)
> {
> - const char *command_line = NULL, *append = NULL;
> - const char *ramdisk;
> + char *command_line = NULL;
> + const char *ramdisk, *append = NULL;
> char *ramdisk_buf;
> off_t ramdisk_length;
> int command_line_len;
> @@ -406,5 +406,7 @@ int bzImage_load(int argc, char **argv,
> ramdisk_buf, ramdisk_length,
> real_mode_entry, debug);
>
> + if (command_line)
> + free(command_line);
> return result;
> }
> Index: kexec-tools/kexec/arch/i386/kexec-elf-x86.c
> ===================================================================
> --- kexec-tools.orig/kexec/arch/i386/kexec-elf-x86.c 2010-02-01 23:07:15.000000000 +1100
> +++ kexec-tools/kexec/arch/i386/kexec-elf-x86.c 2010-02-01 23:07:15.000000000 +1100
> @@ -88,8 +88,8 @@ int elf_x86_load(int argc, char **argv,
> struct kexec_info *info)
> {
> struct mem_ehdr ehdr;
> - const char *command_line = NULL, *append = NULL;
> - char *modified_cmdline;
> + char *command_line = NULL, *modified_cmdline = NULL;
> + const char *append = NULL;
> int command_line_len;
> int modified_cmdline_len;
> const char *ramdisk;
> @@ -264,8 +264,11 @@ int elf_x86_load(int argc, char **argv,
> if (rc < 0)
> return -1;
> /* Use new command line. */
> + if (command_line)
> + free(command_line);
> command_line = modified_cmdline;
> command_line_len = strlen(modified_cmdline) + 1;
> + modified_cmdline = NULL;
> }
>
> /* Tell the kernel what is going on */
> @@ -288,5 +291,10 @@ int elf_x86_load(int argc, char **argv,
> else {
> die("Unknown argument style\n");
> }
> +
> + if (command_line)
> + free(command_line);
> + if (modified_cmdline)
> + free(modified_cmdline);
> return 0;
> }
> Index: kexec-tools/kexec/kexec.c
> ===================================================================
> --- kexec-tools.orig/kexec/kexec.c 2010-02-01 23:07:15.000000000 +1100
> +++ kexec-tools/kexec/kexec.c 2010-02-01 23:07:15.000000000 +1100
> @@ -62,6 +62,14 @@ void die(char *fmt, ...)
> exit(1);
> }
>
> +char *xstrdup(const char *str)
> +{
> + char *new = strdup(str);
> + if (!new)
> + die("Cannot strdup \"%s\": %s\n",
> + str, strerror(errno));
> + return new;
> +}
>
> void *xmalloc(size_t size)
> {
> @@ -994,15 +1002,15 @@ void check_reuse_initrd(void)
> free(line);
> }
>
> -const char *concat_cmdline(const char *base, const char *append)
> +char *concat_cmdline(const char *base, const char *append)
> {
> - const char *cmdline;
> + char *cmdline;
> if (!base && !append)
> return NULL;
> - if (!base)
> - return append;
> - if (!append)
> - return base;
> + if (append)
> + return xstrdup(append);
> + if (base)
> + return xstrdup(base);
> cmdline = xmalloc(strlen(base) + 1 + strlen(append) + 1);
> strcpy(cmdline, base);
> strcat(cmdline, " ");
> Index: kexec-tools/kexec/arch/i386/kexec-multiboot-x86.c
> ===================================================================
> --- kexec-tools.orig/kexec/arch/i386/kexec-multiboot-x86.c 2010-02-01 23:07:15.000000000 +1100
> +++ kexec-tools/kexec/arch/i386/kexec-multiboot-x86.c 2010-02-01 23:07:15.000000000 +1100
> @@ -147,8 +147,8 @@ int multiboot_x86_load(int argc, char **
> unsigned long mbi_base;
> struct entry32_regs regs;
> size_t mbi_bytes, mbi_offset;
> - const char *command_line=NULL, *append=NULL;
> - char *imagename, *cp;
> + char *command_line = NULL;
> + char *imagename, *cp, *append = NULL;;
> struct memory_range *range;
> int ranges;
> struct AddrRangeDesc *mmap;
> @@ -389,6 +389,8 @@ int multiboot_x86_load(int argc, char **
> regs.eip = ehdr.e_entry;
> elf_rel_set_symbol(&info->rhdr, "entry32_regs", ®s, sizeof(regs));
>
> + if (command_line)
> + free(command_line);
> return 0;
> }
>
> Index: kexec-tools/kexec/arch/x86_64/kexec-elf-x86_64.c
> ===================================================================
> --- kexec-tools.orig/kexec/arch/x86_64/kexec-elf-x86_64.c 2010-02-01 23:07:15.000000000 +1100
> +++ kexec-tools/kexec/arch/x86_64/kexec-elf-x86_64.c 2010-02-01 23:07:15.000000000 +1100
> @@ -87,8 +87,8 @@ int elf_x86_64_load(int argc, char **arg
> struct kexec_info *info)
> {
> struct mem_ehdr ehdr;
> - const char *command_line = NULL, *append = NULL;
> - char *modified_cmdline;
> + const char *append = NULL;
> + char *command_line = NULL, *modified_cmdline;
> int command_line_len;
> int modified_cmdline_len;
> const char *ramdisk;
> @@ -249,8 +249,11 @@ int elf_x86_64_load(int argc, char **arg
> if (rc < 0)
> return -1;
> /* Use new command line. */
> + if (command_line)
> + free(command_line);
> command_line = modified_cmdline;
> command_line_len = strlen(modified_cmdline) + 1;
> + modified_cmdline = NULL;
> }
>
> /* Tell the kernel what is going on */
> @@ -273,5 +276,10 @@ int elf_x86_64_load(int argc, char **arg
> else {
> die("Unknown argument style\n");
> }
> +
> + if (command_line)
> + free(command_line);
> + if (modified_cmdline)
> + free(modified_cmdline);
> return 0;
> }
> Index: kexec-tools/kexec/kexec.h
> ===================================================================
> --- kexec-tools.orig/kexec/kexec.h 2010-02-01 23:07:15.000000000 +1100
> +++ kexec-tools/kexec/kexec.h 2010-02-01 23:07:15.000000000 +1100
> @@ -261,6 +261,6 @@ static inline int __attribute__ ((format
> dbgprintf(const char *fmt, ...) {return 0;}
> #endif
>
> -const char *concat_cmdline(const char *base, const char *append);
> +char *concat_cmdline(const char *base, const char *append);
>
> #endif /* KEXEC_H */
