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@xxxxxxxxxxxx>
don't leak in concat_cmdline
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 */
