Message ID | 1416686560-12666-1-git-send-email-ebiggers3@gmail.com |
---|---|
State | Committed |
Headers |
Received: (qmail 14669 invoked by alias); 22 Nov 2014 20:05:06 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 14659 invoked by uid 89); 22 Nov 2014 20:05:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=BAYES_00, FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-ie0-f173.google.com X-Received: by 10.51.15.132 with SMTP id fo4mr4471641igd.36.1416686702397; Sat, 22 Nov 2014 12:05:02 -0800 (PST) From: Eric Biggers <ebiggers3@gmail.com> To: libc-alpha@sourceware.org Cc: Eric Biggers <ebiggers3@gmail.com> Subject: [PATCH] setenv(): fix memory leak when setting large, duplicate string Date: Sat, 22 Nov 2014 14:02:40 -0600 Message-Id: <1416686560-12666-1-git-send-email-ebiggers3@gmail.com> |
Commit Message
Eric Biggers
Nov. 22, 2014, 8:02 p.m. UTC
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(+)
Comments
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;