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

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

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

Update the processing of --append so that --reuse-cmdline followed
by --append actually appends the parameters specified by --reuse-cmdline.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 kexec/arch/i386/kexec-bzImage.c       |    8 ++++----
 kexec/arch/i386/kexec-elf-x86.c       |    8 ++++----
 kexec/arch/i386/kexec-multiboot-x86.c |    7 ++++---
 kexec/arch/x86_64/kexec-elf-x86_64.c  |    8 ++++----
 kexec/kexec.c                         |   16 ++++++++++++++++
 kexec/kexec.h                         |    2 ++
 6 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/kexec/arch/i386/kexec-bzImage.c b/kexec/arch/i386/kexec-bzImage.c
index be88a3f..21c1ddf 100644
--- a/kexec/arch/i386/kexec-bzImage.c
+++ b/kexec/arch/i386/kexec-bzImage.c
@@ -332,7 +332,7 @@ int do_bzImage_load(struct kexec_info *info,
 int bzImage_load(int argc, char **argv, const char *buf, off_t len, 
 	struct kexec_info *info)
 {
-	const char *command_line;
+	const char *command_line = NULL, *append = NULL;
 	const char *ramdisk;
 	char *ramdisk_buf;
 	off_t ramdisk_length;
@@ -349,7 +349,7 @@ int bzImage_load(int argc, char **argv, const char *buf, off_t len,
 		{ "debug",		0, 0, OPT_DEBUG },
 		{ "command-line",	1, 0, OPT_APPEND },
 		{ "append",		1, 0, OPT_APPEND },
-		{ "reuse-cmdline",	1, 0, OPT_REUSE_CMDLINE },
+		{ "reuse-cmdline",	0, 0, OPT_REUSE_CMDLINE },
 		{ "initrd",		1, 0, OPT_RAMDISK },
 		{ "ramdisk",		1, 0, OPT_RAMDISK },
 		{ "real-mode",		0, 0, OPT_REAL_MODE },
@@ -362,7 +362,6 @@ int bzImage_load(int argc, char **argv, const char *buf, off_t len,
 	 */
 	debug = 0;
 	real_mode_entry = 0;
-	command_line = 0;
 	ramdisk = 0;
 	ramdisk_length = 0;
 	while((opt = getopt_long(argc, argv, short_options, options, 0)) != -1) {
@@ -379,7 +378,7 @@ int bzImage_load(int argc, char **argv, const char *buf, off_t len,
 			debug = 1;
 			break;
 		case OPT_APPEND:
-			command_line = optarg;
+			append = optarg;
 			break;
 		case OPT_REUSE_CMDLINE:
 			command_line = get_command_line();
@@ -392,6 +391,7 @@ int bzImage_load(int argc, char **argv, const char *buf, off_t len,
 			break;
 		}
 	}
+	command_line = concat_cmdline(command_line, append);
 	command_line_len = 0;
 	if (command_line) {
 		command_line_len = strlen(command_line) +1;
diff --git a/kexec/arch/i386/kexec-elf-x86.c b/kexec/arch/i386/kexec-elf-x86.c
index afa0eb5..aaf46ba 100644
--- a/kexec/arch/i386/kexec-elf-x86.c
+++ b/kexec/arch/i386/kexec-elf-x86.c
@@ -88,7 +88,7 @@ int elf_x86_load(int argc, char **argv, const char *buf, off_t len,
 	struct kexec_info *info)
 {
 	struct mem_ehdr ehdr;
-	const char *command_line;
+	const char *command_line = NULL, *append = NULL;
 	char *modified_cmdline;
 	int command_line_len;
 	int modified_cmdline_len;
@@ -110,7 +110,7 @@ int elf_x86_load(int argc, char **argv, const char *buf, off_t len,
 		KEXEC_ARCH_OPTIONS
 		{ "command-line",	1, NULL, OPT_APPEND },
 		{ "append",		1, NULL, OPT_APPEND },
-		{ "reuse-cmdline",	1, NULL, OPT_REUSE_CMDLINE },
+		{ "reuse-cmdline",	0, NULL, OPT_REUSE_CMDLINE },
 		{ "initrd",		1, NULL, OPT_RAMDISK },
 		{ "ramdisk",		1, NULL, OPT_RAMDISK },
 		{ "args-elf",		0, NULL, OPT_ARGS_ELF },
@@ -125,7 +125,6 @@ int elf_x86_load(int argc, char **argv, const char *buf, off_t len,
 	 * Parse the command line arguments
 	 */
 	arg_style = ARG_STYLE_ELF;
-	command_line = 0;
 	modified_cmdline = 0;
 	modified_cmdline_len = 0;
 	ramdisk = 0;
@@ -140,7 +139,7 @@ int elf_x86_load(int argc, char **argv, const char *buf, off_t len,
 			usage();
 			return -1;
 		case OPT_APPEND:
-			command_line = optarg;
+			append = optarg;
 			break;
 		case OPT_REUSE_CMDLINE:
 			command_line = get_command_line();
@@ -163,6 +162,7 @@ int elf_x86_load(int argc, char **argv, const char *buf, off_t len,
 			break;
 		}
 	}
+	command_line = concat_cmdline(command_line, append);
 	command_line_len = 0;
 	if (command_line) {
 		command_line_len = strlen(command_line) +1;
diff --git a/kexec/arch/i386/kexec-multiboot-x86.c b/kexec/arch/i386/kexec-multiboot-x86.c
index 9b41698..9817ba9 100644
--- a/kexec/arch/i386/kexec-multiboot-x86.c
+++ b/kexec/arch/i386/kexec-multiboot-x86.c
@@ -147,7 +147,7 @@ int multiboot_x86_load(int argc, char **argv, const char *buf, off_t len,
 	unsigned long mbi_base;
 	struct entry32_regs regs;
 	size_t mbi_bytes, mbi_offset;
-	const char *command_line=NULL;
+	const char *command_line=NULL, *append=NULL;
 	char *imagename, *cp;
 	struct memory_range *range;
 	int ranges;
@@ -164,7 +164,7 @@ int multiboot_x86_load(int argc, char **argv, const char *buf, off_t len,
 		KEXEC_ARCH_OPTIONS
 		{ "command-line",		1, 0, OPT_CL },
 		{ "append",			1, 0, OPT_CL },
-		{ "reuse-cmdline",		1, 0, OPT_REUSE_CMDLINE },
+		{ "reuse-cmdline",		0, 0, OPT_REUSE_CMDLINE },
 		{ "module",			1, 0, OPT_MOD },
 		{ 0, 				0, 0, 0 },
 	};
@@ -195,7 +195,7 @@ int multiboot_x86_load(int argc, char **argv, const char *buf, off_t len,
 			usage();
 			return -1;
 		case OPT_CL:
-			command_line = optarg;
+			append = optarg;
 			break;
 		case OPT_REUSE_CMDLINE:
 			command_line = get_command_line();
@@ -207,6 +207,7 @@ int multiboot_x86_load(int argc, char **argv, const char *buf, off_t len,
 		}
 	}
 	imagename = argv[optind];
+	command_line = concat_cmdline(command_line, append);
 	command_line_len = strlen(command_line) + strlen(imagename) + 2;
 
 
diff --git a/kexec/arch/x86_64/kexec-elf-x86_64.c b/kexec/arch/x86_64/kexec-elf-x86_64.c
index 95ebef6..901262f 100644
--- a/kexec/arch/x86_64/kexec-elf-x86_64.c
+++ b/kexec/arch/x86_64/kexec-elf-x86_64.c
@@ -87,7 +87,7 @@ int elf_x86_64_load(int argc, char **argv, const char *buf, off_t len,
 	struct kexec_info *info)
 {
 	struct mem_ehdr ehdr;
-	const char *command_line;
+	const char *command_line = NULL, *append = NULL;
 	char *modified_cmdline;
 	int command_line_len;
 	int modified_cmdline_len;
@@ -109,7 +109,7 @@ int elf_x86_64_load(int argc, char **argv, const char *buf, off_t len,
 		KEXEC_ARCH_OPTIONS
 		{ "command-line",	1, NULL, OPT_APPEND },
 		{ "append",		1, NULL, OPT_APPEND },
-		{ "reuse-cmdline",	1, NULL, OPT_REUSE_CMDLINE },
+		{ "reuse-cmdline",	0, NULL, OPT_REUSE_CMDLINE },
 		{ "initrd",		1, NULL, OPT_RAMDISK },
 		{ "ramdisk",		1, NULL, OPT_RAMDISK },
 		{ "args-elf",		0, NULL, OPT_ARGS_ELF },
@@ -124,7 +124,6 @@ int elf_x86_64_load(int argc, char **argv, const char *buf, off_t len,
 	 * Parse the command line arguments
 	 */
 	arg_style = ARG_STYLE_ELF;
-	command_line = 0;
 	modified_cmdline = 0;
 	modified_cmdline_len = 0;
 	ramdisk = 0;
@@ -140,7 +139,7 @@ int elf_x86_64_load(int argc, char **argv, const char *buf, off_t len,
 			usage();
 			return -1;
 		case OPT_APPEND:
-			command_line = optarg;
+			append = optarg;
 			break;
 		case OPT_REUSE_CMDLINE:
 			command_line = get_command_line();
@@ -163,6 +162,7 @@ int elf_x86_64_load(int argc, char **argv, const char *buf, off_t len,
 			break;
 		}
 	}
+	command_line = concat_cmdline(command_line, append);
 	command_line_len = 0;
 	if (command_line) {
 		command_line_len = strlen(command_line) +1;
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;
+}
+
 
 int main(int argc, char *argv[])
 {
diff --git a/kexec/kexec.h b/kexec/kexec.h
index 53aa58c..ebc5a95 100644
--- a/kexec/kexec.h
+++ b/kexec/kexec.h
@@ -261,4 +261,6 @@ static inline int __attribute__ ((format (printf, 1, 2)))
 	dbgprintf(const char *fmt, ...) {return 0;}
 #endif
 
+const char *concat_cmdline(const char *base, const char *append);
+
 #endif /* KEXEC_H */
-- 
1.6.2.5


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Simon Horman]

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.

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

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

[snip]


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Eric W. Biederman]

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

Eric


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Bernhard Walle]

Am 25.01.2010 09:14, schrieb Eric W. Biederman:
> Where does the memory that getopt uses come from?

The optarg string points to the original *argv[] array and doesn't
allocate new memory.


Regards,
Bernhard





_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Simon Horman]

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

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

[Eric W. Biederman]

Simon Horman <horms@verge.net.au> writes:

> 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

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

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

[Simon Horman]

On Mon, Feb 01, 2010 at 08:08:51AM -0800, Eric W. Biederman wrote:
> Simon Horman <horms@verge.net.au> writes:
> 
> > 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
> 
> 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 *’

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


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Eric W. Biederman]

Simon Horman <horms@verge.net.au> writes:

> On Mon, Feb 01, 2010 at 08:08:51AM -0800, Eric W. Biederman wrote:
>> Simon Horman <horms@verge.net.au> writes:
>> 
>> > 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
>> 
>> 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 *’

Odd.  I did not see those errors, but those are definitely bugs in
concat_cmdline. 

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

free is well defined when passed NULL.  At least according to my local
manpages.

Eric


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Simon Horman]

On Mon, Feb 01, 2010 at 04:21:16PM -0800, Eric W. Biederman wrote:
> Simon Horman <horms@verge.net.au> writes:
> 
> > On Mon, Feb 01, 2010 at 08:08:51AM -0800, Eric W. Biederman wrote:
> >> Simon Horman <horms@verge.net.au> writes:
> >> 
> >> > 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
> >> 
> >> 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 *’
> 
> Odd.  I did not see those errors, but those are definitely bugs in
> concat_cmdline. 
> 
> >> 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.
> 
> free is well defined when passed NULL.  At least according to my local
> manpages.

Noted, I'll make the change you suggested.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Simon Horman]

Hi Eric,

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


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Eric W. Biederman]

Simon Horman <horms@verge.net.au> writes:

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

No problem.

Eric


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec