NetBSD Problem Report #43899

From njoly@lanfeust.sis.pasteur.fr  Thu Sep 23 13:08:30 2010
Return-Path: <njoly@lanfeust.sis.pasteur.fr>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by www.NetBSD.org (Postfix) with ESMTP id 77D7363B97A
	for <gnats-bugs@gnats.NetBSD.org>; Thu, 23 Sep 2010 13:08:30 +0000 (UTC)
Message-Id: <20100923130827.A6887DC9BB@lanfeust.sis.pasteur.fr>
Date: Thu, 23 Sep 2010 15:08:27 +0200 (CEST)
From: njoly@pasteur.fr
Reply-To: njoly@pasteur.fr
To: gnats-bugs@gnats.NetBSD.org
Subject: setenv(3)/unsetenv(3) memory leak
X-Send-Pr-Version: 3.95

>Number:         43899
>Category:       lib
>Synopsis:       setenv(3)/unsetenv(3) memory leak
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    lib-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Sep 23 13:10:00 +0000 2010
>Last-Modified:  Mon Sep 27 16:25:01 +0000 2010
>Originator:     Nicolas Joly
>Release:        NetBSD 5.99.39
>Organization:
Institut Pasteur
>Environment:
System: NetBSD lanfeust.sis.pasteur.fr 5.99.39 NetBSD 5.99.39 (LANFEUST) #9: Thu Sep 23 14:27:34 CEST 2010 njoly@lanfeust.sis.pasteur.fr:/local/src/NetBSD/obj.amd64/sys/arch/amd64/compile/LANFEUST amd64
Architecture: x86_64
Machine: amd64
>Description:
I just noticed that unsetenv(3) does not seems to reclaim memory that was
allocated by setenv(3) for a single variable.
>How-To-Repeat:
Compile the following piece of code, run it and watch the process eating
memory.

#include <stdlib.h>
int main() {
  while (1) {
    setenv("dummyvar", "dummyval", 0);
    unsetenv("dummyvar"); }
  return 0; }

>Fix:

>Audit-Trail:
From: Christos Zoulas <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/43899 CVS commit: src/lib/libc/stdlib
Date: Thu, 23 Sep 2010 12:02:42 -0400

 Module Name:	src
 Committed By:	christos
 Date:		Thu Sep 23 16:02:41 UTC 2010

 Modified Files:
 	src/lib/libc/stdlib: setenv.c

 Log Message:
 PR/43899: Nicolas Joly: setenv(3)/unsetenv(3) memory leak.
 Partial fix: Don't allocate a new string if the length is equal to the
 old length, because presumably the old string was also nul terminated
 so it has the extra byte needed.
 The real fix is to keep an adjunct array of bits, one for each environment
 variable and keep track if the entry was allocated or not so that we can
 free it in unsetenv.


 To generate a diff of this commit:
 cvs rdiff -u -r1.32 -r1.33 src/lib/libc/stdlib/setenv.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: itohy@NetBSD.org (ITOH Yasufumi)
To: gnats-bugs@NetBSD.org, netbsd-bugs@NetBSD.org
Cc: 
Subject: Re: lib/43899: setenv(3)/unsetenv(3) memory leak
Date: Sat, 25 Sep 2010 17:16:33 +0900

 In article <20100923131001.5C93F63B995@www.NetBSD.org>
 njoly@pasteur.fr writes:

 > >Description:
 > I just noticed that unsetenv(3) does not seems to reclaim memory that was
 > allocated by setenv(3) for a single variable.

 setenv(3) leaks memory by its nature.  It's a spec.

 The problem is that our implementation of putenv(3) leaks memory.
 The standard says we can use putenv(3) without memory leaks.
 http://www.opengroup.org/onlinepubs/9699919799/functions/putenv.html

 Thanks,
 -- 
 ITOH Yasufumi

From: Takehiko NOZAKI <takehiko.nozaki@gmail.com>
To: gnats-bugs@netbsd.org
Cc: lib-bug-people@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org, 
	njoly@pasteur.fr
Subject: Re: lib/43899: setenv(3)/unsetenv(3) memory leak
Date: Sun, 26 Sep 2010 03:05:52 +0900

 hi, all.

 please backout recent (un)?setenv(3) change.
 it doesn't consider user-modified environ(7).

 example, following code may cause memory fault.

 (snip)
 #include <stdlib.h>

 extern char **environ;

 int
 main (void)
 {
         char **p;
         setenv("FOO", "bar", 1);
         for (p = environ; *p != NULL; ++p) {
                 if (!memcmp(*p, "FOO=", 4)) {
                         *p = "FOO=bar"; /* brutal, but permitted */
                         break;
                 }
         }
         unsetenv("FOO");
 }
 (snip)


 on more problem, old setenv.c#rev1.32 is not enough for this example.
 apply this patch such as OpenBSD do.

 (snip)
 --- setenv.c.orig	2010-09-05 05:25:01.000000000 +0900
 +++ setenv.c	2010-09-05 05:25:03.000000000 +0900
 @@ -82,8 +82,10 @@
  	if ((c = __findenv(name, &offset)) != NULL) {
  		if (!rewrite)
  			goto good;
 +#if 0 /* XXX - existing entry may not be writable */
  		if (strlen(c) >= l_value)	/* old larger; copy over */
  			goto copy;
 +#endif
  	} else {					/* create new slot */
  		size = (size_t)(sizeof(char *) * (offset + 2));
  		if (saveenv == environ) {		/* just increase size */
 (snip)



 BTW, Solaris and FreeBSD introduce more reasonable(but very complex)
 anti-memory leak (un)?setenv(3). see following link:
 http://www.freebsd.org/cgi/cvsweb.cgi/src/lib/libc/stdlib/getenv.c?rev=1.20;content-type=text/plain

 and they have also "POSIX-strict" putenv(3).
 spec says:

     the string pointed to by string shall become part of the environment,
     so altering the string shall change the environment.

 but our putenv(3) is internally call setenv(3), so that altering the string
 doesn't change environ(7) (traditional bug from 4.3BSD Reno)

 if there's someone who intersted in porting FreeBSD's environ(7) stuff,
 don't forget apply following patch, fix bug and kill vuln(*1).

 (snip)
 --- getenv.c.orig	2010-09-05 06:02:18.000000000 +0900
 +++ getenv.c	2010-09-05 06:02:13.000000000 +0900
 @@ -361,8 +361,7 @@
  		} else {
  			__env_warnx(CorruptEnvValueMsg, envVars[envNdx].name,
  			    strlen(envVars[envNdx].name));
 -			errno = EFAULT;
 -			goto Failure;
 +			continue;
  		}

  		/*
 @@ -377,8 +376,7 @@
  		    false) == NULL) {
  			__env_warnx(CorruptEnvFindMsg, envVars[envNdx].name,
  			    nameLen);
 -			errno = EFAULT;
 -			goto Failure;
 +			continue;
  		}
  		envVars[activeNdx].active = true;
  	}
 @@ -505,9 +503,9 @@
  		envVars[envNdx].valueSize = valueLen;

  		/* Save name of name/value pair. */
 -		env = stpcpy(envVars[envNdx].name, name);
 -		if ((envVars[envNdx].name)[nameLen] != '=')
 -			env = stpcpy(env, "=");
 +		memcpy(envVars[envNdx].name, name, nameLen);
 +		env = envVars[envNdx].name + nameLen;
 +		*env++ = '=';
  	}
  	else
  		env = envVars[envNdx].value;
 @@ -560,8 +558,7 @@
  				if ((equals = strchr(*env, '=')) == NULL) {
  					__env_warnx(CorruptEnvValueMsg, *env,
  					    strlen(*env));
 -					errno = EFAULT;
 -					return (-1);
 +					continue;
  				}
  				if (__setenv(*env, equals - *env, equals + 1,
  				    1) == -1)
 (snip)

 (*1) unsetenv(3) returning EFAULT is not good idea for old application.
 old prototype is void, never return error, so many application doesn't check
 return value(see FreeBSD-SA-09:16).


 very truly yours.
 --
 Takehiko NOZAKI <tnozaki@NetBSD.org>

From: Nicolas Joly <njoly@pasteur.fr>
To: gnats-bugs@NetBSD.org, netbsd-bugs@NetBSD.org
Cc: 
Subject: Re: lib/43899: setenv(3)/unsetenv(3) memory leak
Date: Mon, 27 Sep 2010 18:24:07 +0200

 On Sat, Sep 25, 2010 at 05:16:33PM +0900, ITOH Yasufumi wrote:
 > In article <20100923131001.5C93F63B995@www.NetBSD.org>
 > njoly@pasteur.fr writes:
 > 
 > > >Description:
 > > I just noticed that unsetenv(3) does not seems to reclaim memory that was
 > > allocated by setenv(3) for a single variable.
 > 
 > setenv(3) leaks memory by its nature.  It's a spec.

 True. While i can understand that setenv(3) only in a loop do/can leak
 memory, but if associated with unsetenv(3) in the same loop it should
 not.

 > The problem is that our implementation of putenv(3) leaks memory.
 > The standard says we can use putenv(3) without memory leaks.
 > http://www.opengroup.org/onlinepubs/9699919799/functions/putenv.html

 Looks like it should try to unset the variable if it exists before
 setting it again.

 -- 
 Nicolas Joly

 Biological Software and Databanks.
 Institut Pasteur, Paris.

Home	PR Database Search