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

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

The following reply was made to PR lib/43899; it has been noted by GNATS.

From: Takehiko NOZAKI <takehiko.nozaki%gmail.com@localhost>
To: gnats-bugs%netbsd.org@localhost
Cc: lib-bug-people%netbsd.org@localhost, gnats-admin%netbsd.org@localhost, 
netbsd-bugs%netbsd.org@localhost, 
        njoly%pasteur.fr@localhost
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@localhost>