setenv(): fix memory leak when setting large, duplicate string
diff mbox

Message ID 1416686560-12666-1-git-send-email-ebiggers3@gmail.com
State New
Headers show

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

Siddhesh Poyarekar Dec. 1, 2014, 10:23 a.m. UTC | #1
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
H.J. Lu Jan. 6, 2015, 5:09 p.m. UTC | #2
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
Ondřej Bílka Jan. 6, 2015, 5:57 p.m. UTC | #3
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.
H.J. Lu Jan. 6, 2015, 6:08 p.m. UTC | #4
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?
Ondřej Bílka Jan. 6, 2015, 6:24 p.m. UTC | #5
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.
Eric Biggers Jan. 6, 2015, 11:28 p.m. UTC | #6
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
Siddhesh Poyarekar Jan. 7, 2015, 6:53 a.m. UTC | #7
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

Patch
diff mbox

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;