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 Signed-off-by: Simon Horman <[email protected]> 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 */ _______________________________________________ kexec mailing list [email protected] http://lists.infradead.org/mailman/listinfo/kexec
