[Patch] fs/binfmt_elf.c: fix a wrong free

[Patch] fs/binfmt_elf.c: fix a wrong free

[WANG Cong]

Fix a wrong free in fs/binfmt_elf.c::elf_core_dump().

Signed-off-by: WANG Cong <wangcong@zeuux.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Eric Youngdale <ericy@cais.com>

---
 fs/binfmt_elf.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index b25707f..43254e3 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -2032,10 +2032,10 @@ static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file, un
 
 end_coredump:
 	set_fs(fs);
+	free_note_info(&info);
 
 cleanup:
 	kfree(elf);
-	free_note_info(&info);
 	return has_dumped;
 }
 
-- 
1.5.4.1

Re: [Patch] fs/binfmt_elf.c: fix a wrong free

[Pekka J Enberg]

On Sat, 3 May 2008, WANG Cong wrote:
> Fix a wrong free in fs/binfmt_elf.c::elf_core_dump().
> 
> Signed-off-by: WANG Cong <wangcong@zeuux.org>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Eric Youngdale <ericy@cais.com>
> 
> ---
>  fs/binfmt_elf.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index b25707f..43254e3 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -2032,10 +2032,10 @@ static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file, un
>  
>  end_coredump:
>  	set_fs(fs);
> +	free_note_info(&info);
>  
>  cleanup:
>  	kfree(elf);
> -	free_note_info(&info);
>  	return has_dumped;
>  }

Looks like fill_note_info() requires that you call free_note_info() if it 
fails; otherwise we'll leak memory. So perhaps something like the 
following totally untested patch?

		Pekka

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index b25707f..febd208 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1917,7 +1917,7 @@ static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file, un
 	 */
 	if (!fill_note_info(elf, segs + 1, /* including notes section */
 			    &info, signr, regs))
-		goto cleanup;
+		goto cleanup_note;
 
 	has_dumped = 1;
 	current->flags |= PF_DUMPCORE;
@@ -2033,9 +2033,11 @@ static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file, un
 end_coredump:
 	set_fs(fs);
 
+cleanup_note:
+	free_note_info(&info);
+
 cleanup:
 	kfree(elf);
-	free_note_info(&info);
 	return has_dumped;
 }
 

Re: [Patch] fs/binfmt_elf.c: fix a wrong free

[WANG Cong]

On Sat, May 03, 2008 at 04:26:11PM +0300, Pekka J Enberg wrote:
>On Sat, 3 May 2008, WANG Cong wrote:
>> Fix a wrong free in fs/binfmt_elf.c::elf_core_dump().
>> 
>> Signed-off-by: WANG Cong <wangcong@zeuux.org>
>> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
>> Cc: Eric Youngdale <ericy@cais.com>
>> 
>> ---
>>  fs/binfmt_elf.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>> 
>> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
>> index b25707f..43254e3 100644
>> --- a/fs/binfmt_elf.c
>> +++ b/fs/binfmt_elf.c
>> @@ -2032,10 +2032,10 @@ static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file, un
>>  
>>  end_coredump:
>>  	set_fs(fs);
>> +	free_note_info(&info);
>>  
>>  cleanup:
>>  	kfree(elf);
>> -	free_note_info(&info);
>>  	return has_dumped;
>>  }
>
>Looks like fill_note_info() requires that you call free_note_info() if it 
>fails; otherwise we'll leak memory. So perhaps something like the 
>following totally untested patch?
>

Hi, Pekka!

Thanks for your comments. Yes, it seems that fill_note_info() is ugly.
:-) How about the below one?

Signed-off-by: WANG Cong <wangcong@zeuux.org>

---
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index f6d5a9d..357b503 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1900,7 +1900,7 @@ static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file, un
 	/* alloc memory for large data structures: too large to be on stack */
 	elf = kmalloc(sizeof(*elf), GFP_KERNEL);
 	if (!elf)
-		goto cleanup;
+		goto ret;
 	
 	segs = current->mm->map_count;
 #ifdef ELF_CORE_EXTRA_PHDRS
@@ -2034,8 +2034,9 @@ end_coredump:
 	set_fs(fs);
 
 cleanup:
-	kfree(elf);
 	free_note_info(&info);
+	kfree(elf);
+ret:
 	return has_dumped;
 }
 

Re: [Patch] fs/binfmt_elf.c: fix a wrong free

[Pekka Enberg]

WANG Cong wrote:
> Hi, Pekka!
> 
> Thanks for your comments. Yes, it seems that fill_note_info() is ugly.
> :-) How about the below one?
> 
> Signed-off-by: WANG Cong <wangcong@zeuux.org>
> 
> ---
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index f6d5a9d..357b503 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -1900,7 +1900,7 @@ static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file, un
>  	/* alloc memory for large data structures: too large to be on stack */
>  	elf = kmalloc(sizeof(*elf), GFP_KERNEL);
>  	if (!elf)
> -		goto cleanup;
> +		goto ret;
>  	
>  	segs = current->mm->map_count;
>  #ifdef ELF_CORE_EXTRA_PHDRS
> @@ -2034,8 +2034,9 @@ end_coredump:
>  	set_fs(fs);
>  
>  cleanup:
> -	kfree(elf);
>  	free_note_info(&info);
> +	kfree(elf);
> +ret:
>  	return has_dumped;

Looks good although 'ret' is usually reserved for the variable that 
contains the return value so you might want to consider using the more 
idiomatic 'out' as the label name.

		Pekka

Re: [Patch] fs/binfmt_elf.c: fix a wrong free

[WANG Cong]

On Sun, May 04, 2008 at 01:39:03PM +0300, Pekka Enberg wrote:

> Looks good although 'ret' is usually reserved for the variable that 
> contains the return value so you might want to consider using the more 
> idiomatic 'out' as the label name.
>

Thanks. Done.

---
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index f6d5a9d..357b503 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1900,7 +1900,7 @@ static int elf_core_dump(long signr, struct pt_regs *regs, struct file *file, un
 	/* alloc memory for large data structures: too large to be on stack */
 	elf = kmalloc(sizeof(*elf), GFP_KERNEL);
 	if (!elf)
-		goto cleanup;
+		goto out;
 	
 	segs = current->mm->map_count;
 #ifdef ELF_CORE_EXTRA_PHDRS
@@ -2034,8 +2034,9 @@ end_coredump:
 	set_fs(fs);
 
 cleanup:
-	kfree(elf);
 	free_note_info(&info);
+	kfree(elf);
+out:
 	return has_dumped;
 }