Re: CVS commit: src/lib/libc/stdlib

[enami tsugutomo <tsugutomo.enami%jp.sony.com@localhost>]

> 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 environmen=
> t
> 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

Actual change done in setenv.c rev. 1.33 causes following program
memory leak (while rev. 1.32 does not).

        #include <stdlib.h>

        main()
        {

                while (1)
                        setenv("dummyvar", "dummyval", 1);
        }

And rev. 1.34 introduces lock leak on error path while it leaves
another fixable memory leak.

I guess following patch is worth to be applied.

enami.

Index: lib/libc/stdlib/setenv.c
===================================================================
RCS file: /cvsroot/src/lib/libc/stdlib/setenv.c,v
retrieving revision 1.34
diff -u -r1.34 setenv.c
--- lib/libc/stdlib/setenv.c    23 Sep 2010 17:30:49 -0000      1.34
+++ lib/libc/stdlib/setenv.c    24 Sep 2010 03:43:19 -0000
@@ -81,7 +81,7 @@
        c = __findenv(name, &offset);
 
        if (__allocenv(offset) == -1)
-               return -1;
+               goto bad;
 
        if (*value == '=')                      /* no `=' in value */
                ++value;
@@ -90,7 +90,7 @@
        if (c != NULL) {
                if (!rewrite)
                        goto good;
-               if (strlen(c) > l_value)        /* old larger; copy over */
+               if (strlen(c) >= l_value)       /* old is enough; copy over */
                        goto copy;
        } else {                                        /* create new slot */
                size = (size_t)(sizeof(char *) * (offset + 2));
@@ -113,6 +113,8 @@
        /* name + `=' + value */
        if ((c = malloc(size + l_value + 2)) == NULL)
                goto bad;
+       if (bit_test(__environ_malloced, offset))
+               free(environ[offset]);
        environ[offset] = c;
        (void)memcpy(c, name, size);
        c += size;