bin/99826: setenv()/unsetenv() leak memory

[Sean Farley]

>Number:         99826
>Category:       bin
>Synopsis:       setenv()/unsetenv() leak memory
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Thu Jul 06 02:00:32 GMT 2006
>Closed-Date:
>Last-Modified:
>Originator:     Sean Farley
>Release:        FreeBSD 6.1-STABLE i386
>Organization:
>Environment:
System: FreeBSD thor.farley.org 6.1-STABLE FreeBSD 6.1-STABLE #0: Tue Jun 6 12:54:08 CDT 2006 root at thor.farley.org:/usr/obj/usr/src/sys/THOR i386

>Description:
When writing code to use a third-party library, I ran into a leak that
setenv() exhibits when overwriting a variable with a larger value.
Calling unsetenv() will just leak memory.  All of this is documented in
the man page (setenv(3)):

    Successive calls to setenv() or putenv() assigning a differently
    sized value to the same name will result in a memory leak.  The
    FreeBSD seman- tics for these functions (namely, that the contents
    of value are copied and that old values remain accessible
    indefinitely) make this bug unavoidable.  Future versions may
    eliminate one or both of these semantic guarantees in order to fix
    the bug.

Unfortunately, I must reset an environment variable quite a lot
resulting in a quick memory leak.

To prevent the leak, I have written a patch which copies the entire
environment into dynamic memory upon the first setenv().  This way any
time an existing variable has its value updated with a larger value the
old value is freed and replaced with a new copy.  unsetenv() will free
values if setenv() has been called first.  This was tested with dmalloc
and MALLOC_OPTIONS=AX.

A test program can be found here[1] and expanded here[2].

  1. http://www.farley.org/freebsd/tmp/setenv-3.tar.bz2
  2. http://www.farley.org/freebsd/tmp/setenv-3/


>How-To-Repeat:
Run hungry and watch it grow via top.  Run lean and watch it hold
steady.

>Fix:
It can be also found at
http://www.farley.org/freebsd/tmp/setenv-3/setenv.c.diff

-----Begin Fix-----
--- /usr/src/lib/libc/stdlib/setenv.c	Fri Mar 22 15:53:10 2002
+++ setenv.c	Wed Jul  5 20:47:35 2006
@@ -40,9 +40,12 @@
 #include <stddef.h>
 #include <stdlib.h>
 #include <string.h>
+#include <sys/types.h>
 
 char *__findenv(const char *, int *);
 
+static uint8_t dynamicEnv = 0;			/* Env vars were copied */
+
 /*
  * setenv --
  *	Set the value of the environmental variable "name" to be
@@ -59,6 +62,18 @@
 	char *c;
 	int l_value, offset;
 
+	/*
+	 * Make copies of environment variables to allow for
+	 * reallocation.  This stops a potential memory leak
+	 * when resetting a variable with larger values.
+	 */
+	if (!dynamicEnv) {
+		for (offset = 0; environ[offset]; offset++)
+			if ((environ[offset] = strdup(environ[offset])) == NULL)
+				return (-1);
+		dynamicEnv = 1;
+	}
+
 	if (*value == '=')			/* no `=' in value */
 		++value;
 	l_value = strlen(value);
@@ -74,27 +89,21 @@
 		char **p;
 
 		for (p = environ, cnt = 0; *p; ++p, ++cnt);
-		if (alloced == environ) {			/* just increase size */
-			p = (char **)realloc((char *)environ,
-			    (size_t)(sizeof(char *) * (cnt + 2)));
-			if (!p)
-				return (-1);
-			alloced = environ = p;
-		}
-		else {				/* get new space */
-						/* copy old entries into it */
-			p = malloc((size_t)(sizeof(char *) * (cnt + 2)));
-			if (!p)
-				return (-1);
+		p = (char **)realloc((char *)alloced,
+		    (size_t)(sizeof(char *) * (cnt + 2)));
+		if (!p)
+			return (-1);
+		if (alloced != environ) {		/* alloced environ */
+			/* copy old entries into it */
 			bcopy(environ, p, cnt * sizeof(char *));
-			alloced = environ = p;
 		}
+		alloced = environ = p;
 		environ[cnt + 1] = NULL;
 		offset = cnt;
 	}
 	for (c = (char *)name; *c && *c != '='; ++c);	/* no `=' in name */
 	if (!(environ[offset] =			/* name + `=' + value */
-	    malloc((size_t)((int)(c - name) + l_value + 2))))
+	    reallocf(environ[offset], (size_t)((int)(c - name) + l_value + 2))))
 		return (-1);
 	for (c = environ[offset]; (*c = *name++) && *c != '='; ++c);
 	for (*c++ = '='; (*c++ = *value++); );
@@ -113,8 +122,11 @@
 	char **p;
 	int offset;
 
-	while (__findenv(name, &offset))	/* if set multiple times */
+	while (__findenv(name, &offset)) {	/* if set multiple times */
+		if (dynamicEnv)
+			free(environ[offset]);
 		for (p = &environ[offset];; ++p)
 			if (!(*p = *(p + 1)))
 				break;
+	}
 }
-----End Fix-----
>Release-Note:
>Audit-Trail:
>Unformatted: