Re: lib/43899: setenv(3)/unsetenv(3) memory leak

[Takehiko NOZAKI <takehiko.nozaki%gmail.com@localhost>]

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@localhost>