| Message ID | 1416686560-12666-1-git-send-email-ebiggers3@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Sat, Nov 22, 2014 at 02:02:40PM -0600, Eric Biggers wrote: > glibc maintains a binary tree of environment strings it malloc()ed > itself. However, it's possible for it to malloc() a string, then find > that an identical string is already in the tree. In this case, the > memory is leaked and is not freed if the application later calls > __libc_freeres(). Fix this by freeing 'new_value' when it's unneeded. > > Test case: > #include <stdlib.h> > #include <string.h> > > int main() > { > char *p = calloc(100000, 1); > memset(p, 'A', 99999); > setenv("TESTVAR", p, 1); > setenv("TESTVAR", p, 1); > free(p); > } > > Leak that was reported by valgrind: > 100,008 bytes in 1 blocks are definitely lost in loss record 1 of 1 > at 0x4C29F90: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > by 0x4E6B3D4: __add_to_environ (setenv.c:176) > by 0x4C31B8F: setenv (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > by 0x400642: main (in /mnt/tmpfs/a.out) Thanks, the patch looks good but before I apply it, please provide a ChangeLog for the patch and also file a bug report and give me the bug number. Siddhesh
On Mon, Dec 1, 2014 at 2:23 AM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote: > On Sat, Nov 22, 2014 at 02:02:40PM -0600, Eric Biggers wrote: >> glibc maintains a binary tree of environment strings it malloc()ed >> itself. However, it's possible for it to malloc() a string, then find >> that an identical string is already in the tree. In this case, the >> memory is leaked and is not freed if the application later calls >> __libc_freeres(). Fix this by freeing 'new_value' when it's unneeded. >> >> Test case: >> #include <stdlib.h> >> #include <string.h> >> >> int main() >> { >> char *p = calloc(100000, 1); >> memset(p, 'A', 99999); >> setenv("TESTVAR", p, 1); >> setenv("TESTVAR", p, 1); >> free(p); >> } >> >> Leak that was reported by valgrind: >> 100,008 bytes in 1 blocks are definitely lost in loss record 1 of 1 >> at 0x4C29F90: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) >> by 0x4E6B3D4: __add_to_environ (setenv.c:176) >> by 0x4C31B8F: setenv (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) >> by 0x400642: main (in /mnt/tmpfs/a.out) > > Thanks, the patch looks good but before I apply it, please provide a > ChangeLog for the patch and also file a bug report and give me the bug > number. > > Siddhesh I opened: https://sourceware.org/bugzilla/show_bug.cgi?id=17805
On Tue, Jan 06, 2015 at 09:09:09AM -0800, H.J. Lu wrote: > On Mon, Dec 1, 2014 at 2:23 AM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote: > > On Sat, Nov 22, 2014 at 02:02:40PM -0600, Eric Biggers wrote: > > > > Thanks, the patch looks good but before I apply it, please provide a > > ChangeLog for the patch and also file a bug report and give me the bug > > number. > > > > Siddhesh > > I opened: > > https://sourceware.org/bugzilla/show_bug.cgi?id=17805 > I created exactly same report some time ago. So now we wait when Eric will write changelog.
On Tue, Jan 6, 2015 at 9:57 AM, Ondřej Bílka <neleai@seznam.cz> wrote: > On Tue, Jan 06, 2015 at 09:09:09AM -0800, H.J. Lu wrote: >> On Mon, Dec 1, 2014 at 2:23 AM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote: >> > On Sat, Nov 22, 2014 at 02:02:40PM -0600, Eric Biggers wrote: >> > >> > Thanks, the patch looks good but before I apply it, please provide a >> > ChangeLog for the patch and also file a bug report and give me the bug >> > number. >> > >> > Siddhesh >> >> I opened: >> >> https://sourceware.org/bugzilla/show_bug.cgi?id=17805 >> > I created exactly same report some time ago. So now we wait when Eric > will write changelog. For small patches like this, won't it be easier just to write up some ChangeLog and commit it?
On Tue, Jan 06, 2015 at 10:08:26AM -0800, H.J. Lu wrote: > On Tue, Jan 6, 2015 at 9:57 AM, Ondřej Bílka <neleai@seznam.cz> wrote: > > On Tue, Jan 06, 2015 at 09:09:09AM -0800, H.J. Lu wrote: > >> On Mon, Dec 1, 2014 at 2:23 AM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote: > >> > On Sat, Nov 22, 2014 at 02:02:40PM -0600, Eric Biggers wrote: > >> > > >> > Thanks, the patch looks good but before I apply it, please provide a > >> > ChangeLog for the patch and also file a bug report and give me the bug > >> > number. > >> > > >> > Siddhesh > >> > >> I opened: > >> > >> https://sourceware.org/bugzilla/show_bug.cgi?id=17805 > >> > > I created exactly same report some time ago. So now we wait when Eric > > will write changelog. > > For small patches like this, won't it be easier just to write up some > ChangeLog and commit it? > Yes I was planning to do that but decided wait bit if Eric responds.
On Tue, Jan 06, 2015 at 07:24:41PM +0100, Ondřej Bílka wrote:
> Yes I was planning to do that but decided wait bit if Eric responds.
If you need a ChangeLog entry you can use the following. But I would think many
contributors would prefer that a maintainer takes care of the ChangeLog. It is
redundant with the git history and it's impossible for a contributor to know the
date on which the change will be committed.
2015-01-06 Eric Biggers <ebiggers3@gmail.com>
[BZ #17658]
* stdlib/setenv.c: fix memory leak when setting large, duplicate string
On Tue, Jan 06, 2015 at 05:28:48PM -0600, Eric Biggers wrote: > On Tue, Jan 06, 2015 at 07:24:41PM +0100, Ondřej Bílka wrote: > > Yes I was planning to do that but decided wait bit if Eric responds. > > If you need a ChangeLog entry you can use the following. But I would think many > contributors would prefer that a maintainer takes care of the ChangeLog. It is > redundant with the git history and it's impossible for a contributor to know the > date on which the change will be committed. > > 2015-01-06 Eric Biggers <ebiggers3@gmail.com> > > [BZ #17658] > * stdlib/setenv.c: fix memory leak when setting large, duplicate string Thanks, I've committed this now. I agree that the ChangeLog is mostly redundant with git history (and have advocated getting rid of them in the past) but there isn't a consensus on this in the glibc project. That said, the contribution checklist currently requires contributors to submit a ChangeLog along with their patch. If you feel strongly about it, please feel free to discuss this and drive the change in the contribution checklist. Siddhesh
diff --git a/stdlib/setenv.c b/stdlib/setenv.c index 8de5328..3699a33 100644 --- a/stdlib/setenv.c +++ b/stdlib/setenv.c @@ -217,6 +217,13 @@ __add_to_environ (name, value, combined, replace) /* And remember the value. */ STORE_VALUE (np); } +#ifdef USE_TSEARCH + else + { + if (__glibc_unlikely (! use_alloca)) + free (new_value); + } +#endif } *ep = np;
glibc maintains a binary tree of environment strings it malloc()ed itself. However, it's possible for it to malloc() a string, then find that an identical string is already in the tree. In this case, the memory is leaked and is not freed if the application later calls __libc_freeres(). Fix this by freeing 'new_value' when it's unneeded. Test case: #include <stdlib.h> #include <string.h> int main() { char *p = calloc(100000, 1); memset(p, 'A', 99999); setenv("TESTVAR", p, 1); setenv("TESTVAR", p, 1); free(p); } Leak that was reported by valgrind: 100,008 bytes in 1 blocks are definitely lost in loss record 1 of 1 at 0x4C29F90: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) by 0x4E6B3D4: __add_to_environ (setenv.c:176) by 0x4C31B8F: setenv (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) by 0x400642: main (in /mnt/tmpfs/a.out) --- stdlib/setenv.c | 7 +++++++ 1 file changed, 7 insertions(+)