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

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

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

Signed-off-by: Simon Horman <horms@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", &regs, 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
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec