Fix --reuse-cmdline so it is usable.

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.

kexec/kexec.c +6 -0

index a1cec86..f4c22a6 100644
@@ -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)

Quoted 7 lines · Eric W. Biederman

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.

Quoted 2 lines

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.

[snip]

Quoted 18 lines

kexec/kexec.c +10 -0

index a1cec86..f4c22a6 100644
@@ -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);

kexec/kexec.c void check_reuse_initrd(void) · line 1012

Quoted 6 lines

kexec/kexec.c +6 -0

+	strcpy(cmdline, base);
+	strcat(cmdline, " ");
+	strcat(cmdline, append);
+	return cmdline;
+}
+

This introduces a memory leak.

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 :-)

Where does the memory that getopt uses come from?

Quoted 8 lines · Simon Horman

Quoted 7 lines · Eric W. Biederman

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 20 lines

[snip]

Quoted 18 lines

kexec/kexec.c +10 -0

index a1cec86..f4c22a6 100644
@@ -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);

Quoted 8 lines

kexec/kexec.c void check_reuse_initrd(void) · line 1012

Quoted 6 lines

kexec/kexec.c +6 -0

+	strcpy(cmdline, base);
+	strcat(cmdline, " ");
+	strcat(cmdline, append);
+	return cmdline;
+}
+

This introduces a memory leak.

Yep.

Quoted 7 lines

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 ;)

Quoted 59 lines · Eric W. Biederman

Quoted 8 lines · Simon Horman

Quoted 7 lines · Eric W. Biederman

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 20 lines

[snip]

Quoted 18 lines

kexec/kexec.c +10 -0

index a1cec86..f4c22a6 100644
@@ -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);

Quoted 8 lines

kexec/kexec.c void check_reuse_initrd(void) · line 1012

Quoted 6 lines

kexec/kexec.c +6 -0

+	strcpy(cmdline, base);
+	strcat(cmdline, " ");
+	strcat(cmdline, append);
+	return cmdline;
+}
+

This introduces a memory leak.

Yep.

Quoted 7 lines

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 :-)

Quoted 5 lines

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/arch/i386/kexec-bzImage.c +4 -2

@@ -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/arch/i386/kexec-elf-x86.c +10 -2

@@ -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/kexec.c +14 -6

@@ -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/arch/i386/kexec-multiboot-x86.c +4 -2

@@ -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/arch/x86_64/kexec-elf-x86_64.c +10 -2

@@ -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
===================================================================

Quoted 69 lines · Simon Horman

Quoted 59 lines · Eric W. Biederman

Quoted 8 lines · Simon Horman

Quoted 7 lines · Eric W. Biederman

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 20 lines

[snip]

Quoted 18 lines

kexec/kexec.c +10 -0

index a1cec86..f4c22a6 100644
@@ -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);

Quoted 8 lines

kexec/kexec.c void check_reuse_initrd(void) · line 1012

Quoted 6 lines

kexec/kexec.c +6 -0

+	strcpy(cmdline, base);
+	strcat(cmdline, " ");
+	strcat(cmdline, append);
+	return cmdline;
+}
+

This introduces a memory leak.

Yep.

Quoted 7 lines

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 :-)

Quoted 5 lines

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.

Quoted 5 lines

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.

Beyond that the idiom
if (xyz)
free(xyz)
can just become:
free(xyz)

Quoted 71 lines · Eric W. Biederman

Quoted 69 lines · Simon Horman

Quoted 59 lines · Eric W. Biederman

Quoted 8 lines · Simon Horman

Quoted 7 lines · Eric W. Biederman

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 20 lines

[snip]

Quoted 18 lines

kexec/kexec.c +10 -0

index a1cec86..f4c22a6 100644
@@ -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);

Quoted 8 lines

kexec/kexec.c void check_reuse_initrd(void) · line 1012

Quoted 6 lines

kexec/kexec.c +6 -0

+	strcpy(cmdline, base);
+	strcat(cmdline, " ");
+	strcat(cmdline, append);
+	return cmdline;
+}
+

This introduces a memory leak.

Yep.

Quoted 7 lines

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 :-)

Quoted 5 lines

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.

Quoted 7 lines

Quoted 5 lines

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 *’

Quoted 5 lines

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.

Quoted 88 lines · Simon Horman

Quoted 71 lines · Eric W. Biederman

Quoted 69 lines · Simon Horman

Quoted 59 lines · Eric W. Biederman

Quoted 8 lines · Simon Horman

Quoted 7 lines · Eric W. Biederman

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 20 lines

[snip]

Quoted 18 lines

kexec/kexec.c +10 -0

index a1cec86..f4c22a6 100644
@@ -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);

Quoted 8 lines

kexec/kexec.c void check_reuse_initrd(void) · line 1012

Quoted 6 lines

kexec/kexec.c +6 -0

+	strcpy(cmdline, base);
+	strcat(cmdline, " ");
+	strcat(cmdline, append);
+	return cmdline;
+}
+

This introduces a memory leak.

Yep.

Quoted 7 lines

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 :-)

Quoted 5 lines

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.

Quoted 7 lines

Quoted 5 lines

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.

Quoted 8 lines

# 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.

Quoted 6 lines

Quoted 5 lines

Beyond that the idiom
if (xyz)
free(xyz)
can just become:
free(xyz)

Quoted 2 lines

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.

Hi Eric,

sorry for all the mucking around regarding this change. I have applied it to my tree + my free() patch.