* [Patch] fs/binfmt_elf.c: fix a wrong free @ 2008-05-03 12:46 WANG Cong 2008-05-03 13:26 ` Pekka J Enberg 0 siblings, 1 reply; 5+ messages in thread From: WANG Cong @ 2008-05-03 12:46 UTC (permalink / raw) To: LKML; +Cc: Eric Youngdale, viro, linux-fsdevel, Andrew Morton 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Patch] fs/binfmt_elf.c: fix a wrong free 2008-05-03 12:46 [Patch] fs/binfmt_elf.c: fix a wrong free WANG Cong @ 2008-05-03 13:26 ` Pekka J Enberg 2008-05-03 14:39 ` WANG Cong 0 siblings, 1 reply; 5+ messages in thread From: Pekka J Enberg @ 2008-05-03 13:26 UTC (permalink / raw) To: WANG Cong; +Cc: LKML, Eric Youngdale, viro, linux-fsdevel, Andrew Morton 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; } ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Patch] fs/binfmt_elf.c: fix a wrong free 2008-05-03 13:26 ` Pekka J Enberg @ 2008-05-03 14:39 ` WANG Cong 2008-05-04 10:39 ` Pekka Enberg 0 siblings, 1 reply; 5+ messages in thread From: WANG Cong @ 2008-05-03 14:39 UTC (permalink / raw) To: Pekka J Enberg Cc: WANG Cong, LKML, Eric Youngdale, viro, linux-fsdevel, Andrew Morton 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; } ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Patch] fs/binfmt_elf.c: fix a wrong free 2008-05-03 14:39 ` WANG Cong @ 2008-05-04 10:39 ` Pekka Enberg 2008-05-04 14:30 ` WANG Cong 0 siblings, 1 reply; 5+ messages in thread From: Pekka Enberg @ 2008-05-04 10:39 UTC (permalink / raw) To: WANG Cong; +Cc: LKML, Eric Youngdale, viro, linux-fsdevel, Andrew Morton 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Patch] fs/binfmt_elf.c: fix a wrong free 2008-05-04 10:39 ` Pekka Enberg @ 2008-05-04 14:30 ` WANG Cong 0 siblings, 0 replies; 5+ messages in thread From: WANG Cong @ 2008-05-04 14:30 UTC (permalink / raw) To: Pekka Enberg Cc: WANG Cong, LKML, Eric Youngdale, viro, linux-fsdevel, Andrew Morton 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; } ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2008-05-04 14:26 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-05-03 12:46 [Patch] fs/binfmt_elf.c: fix a wrong free WANG Cong 2008-05-03 13:26 ` Pekka J Enberg 2008-05-03 14:39 ` WANG Cong 2008-05-04 10:39 ` Pekka Enberg 2008-05-04 14:30 ` WANG Cong
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).