Re: putenv()/environ bug

[Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx>]

On Sun, 29 Jul 2007 14:08:42 -0500 (CDT)
"Sean C. Farley" <scf@xxxxxxxxxxx> wrote:
> On Sat, 28 Jul 2007, Andrey Borzenkov wrote:
> > What about - check if (un-)setenv is available and use them. On legacy
> > systems use existing implementation. This probably will mean we will
> > be using native utilities everywhere on modern systems.
> 
> That would work for me.  If anyone would like me to test any patches, I
> will.

This sounds the best, and it's working on Fedora 7.

I've tested if both setenv() and unsetenv() are available and if they
are we don't do any management of the environment ourselves, so
the hack in addenv() doesn't occur... I hope that works, otherwise
the implementation of the library is fairly well broken, the whole
point of setenv/unsetenv being to be able to do this in a standard
fashion.

The only references to environ we make with this code are to read it at
the start to initialise internval variables and to pass it to execs.
This seems to be sanctioned by POSIX.

As a TODO indicates, this means we don't actually need to store an "env"
string in the parameter structure, just a flag saying we've stuck the
value in the environment.  That can change when this stuff is clearly
working.

Index: configure.ac
===================================================================
RCS file: /cvsroot/zsh/zsh/configure.ac,v
retrieving revision 1.66
diff -u -r1.66 configure.ac
--- configure.ac	25 Jul 2007 14:54:57 -0000	1.66
+++ configure.ac	30 Jul 2007 20:36:27 -0000
@@ -1126,7 +1126,7 @@
 	       setlocale \
 	       uname \
 	       signgam \
-	       putenv getenv \
+	       putenv getenv setenv unsetenv xw\
 	       brk sbrk \
 	       pathconf sysconf \
 	       tgetent tigetflag tigetnum tigetstr setupterm \
Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.119
diff -u -r1.119 exec.c
--- Src/exec.c	13 Jul 2007 10:09:07 -0000	1.119
+++ Src/exec.c	30 Jul 2007 20:36:29 -0000
@@ -524,7 +524,16 @@
      * that as argv[0] for this external command       */
     if (unset(RESTRICTED) && (z = zgetenv("ARGV0"))) {
 	setdata(firstnode(args), (void *) ztrdup(z));
+	/*
+	 * Note we don't do anything with the parameter structure
+	 * for ARGV0: that's OK since we're about to exec or exit
+	 * on failure.
+	 */
+#ifdef HAVE_UNSETENV
+	unsetenv("ARGV0");
+#else
 	delenvvalue(z - 6);
+#endif
     } else if (flags & BINF_DASH) {
     /* Else if the pre-command `-' was given, we add `-' *
      * to the front of argv[0] for this command.         */
Index: Src/params.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/params.c,v
retrieving revision 1.133
diff -u -r1.133 params.c
--- Src/params.c	25 Jul 2007 09:26:51 -0000	1.133
+++ Src/params.c	30 Jul 2007 20:36:34 -0000
@@ -610,7 +610,7 @@
 createparamtable(void)
 {
     Param ip, pm;
-#ifndef HAVE_PUTENV
+#if !defined(HAVE_PUTENV) && !defined(HAVE_SETENV)
     char **new_environ;
     int  envsize;
 #endif
@@ -665,7 +665,7 @@
 
     setsparam("LOGNAME", ztrdup((str = getlogin()) && *str ? str : cached_username));
 
-#ifndef HAVE_PUTENV
+#if !defined(HAVE_PUTENV) && !defined(HAVE_SETENV)
     /* Copy the environment variables we are inheriting to dynamic *
      * memory, so we can do mallocs and frees on it.               */
     envsize = sizeof(char *)*(1 + arrlen(environ));
@@ -3855,6 +3855,30 @@
 int
 zputenv(char *str)
 {
+#ifdef HAVE_SETENV
+    /*
+     * If we are using unsetenv() to remove values from the
+     * environment, which is the safe thing to do, we
+     * need to use setenv() to put them there in the first place.
+     * Unfortunately this is a slightly different interface
+     * from what zputenv() assumes.
+     */
+    char *ptr;
+    int ret;
+
+    for (ptr = str; *ptr && *ptr != '='; ptr++)
+	;
+    if (*ptr) {
+	*ptr = '\0';
+	ret = setenv(str, ptr+1, 1);
+	*ptr = '=';
+    } else {
+	/* safety first */
+	DPUTS(1, "bad environment string");
+	ret = setenv(str, ptr, 1);
+    }
+    return ret;
+#else
 #ifdef HAVE_PUTENV
     return putenv(str);
 #else
@@ -3878,9 +3902,12 @@
     }
     return 0;
 #endif
+#endif
 }
 
 /**/
+#ifndef HAVE_UNSETENV
+/**/
 static int
 findenv(char *name, int *pos)
 {
@@ -3899,6 +3926,8 @@
     
     return 0;
 }
+/**/
+#endif
 
 /* Given *name = "foo", it searches the environment for string *
  * "foo=bar", and returns a pointer to the beginning of "bar"  */
@@ -3939,14 +3968,20 @@
 void
 addenv(Param pm, char *value)
 {
-    char *oldenv = 0, *newenv = 0, *env = 0;
+    char *newenv = 0;
+#ifndef HAVE_UNSETENV
+    char *oldenv = 0, *env = 0;
     int pos;
+#endif
 
-    /* First check if there is already an environment *
-     * variable matching string `name'. If not, and   *
-     * we are not requested to add new, return        */
+#ifndef HAVE_UNSETENV
+    /*
+     * First check if there is already an environment
+     * variable matching string `name'.
+     */
     if (findenv(pm->node.nam, &pos))
 	oldenv = environ[pos];
+#endif
 
      newenv = mkenvstr(pm->node.nam, value, pm->node.flags);
      if (zputenv(newenv)) {
@@ -3954,6 +3989,19 @@
 	pm->env = NULL;
 	return;
     }
+#ifdef HAVE_UNSETENV
+     /*
+      * If we are using setenv/unsetenv to manage the environment,
+      * we simply store the string we created in pm->env since
+      * memory management of the environment is handled entirely
+      * by the system.
+      *
+      * TODO: is this good enough to fix problem cases from
+      * the other branch?  If so, we don't actually need to
+      * store pm->env at all, just a flag that the value was set.
+      */
+     pm->env = newenv;
+#else
     /*
      * Under Cygwin we must use putenv() to maintain consistency.
      * Unfortunately, current version (1.1.2) copies argument and may
@@ -3973,6 +4021,7 @@
 
     DPUTS(1, "addenv should never reach the end");
     pm->env = NULL;
+#endif
 }
 
 
@@ -4003,6 +4052,7 @@
  * string.                                         */
 
 
+#ifndef HAVE_UNSETENV
 /**/
 void
 delenvvalue(char *x)
@@ -4018,6 +4068,8 @@
     }
     zsfree(x);
 }
+#endif
+
 
 /* Delete a pointer from the list of pointers to environment *
  * variables by shifting all the other pointers up one slot. */
@@ -4026,7 +4078,12 @@
 void
 delenv(Param pm)
 {
+#ifdef HAVE_UNSETENV
+    unsetenv(pm->node.nam);
+    zsfree(pm->env);
+#else
     delenvvalue(pm->env);
+#endif
     pm->env = NULL;
     /*
      * Note we don't remove PM_EXPORT from the flags.  This
Index: Src/system.h
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/system.h,v
retrieving revision 1.44
diff -u -r1.44 system.h
--- Src/system.h	23 Apr 2007 15:15:13 -0000	1.44
+++ Src/system.h	30 Jul 2007 20:36:34 -0000
@@ -693,6 +693,15 @@
 
 extern char **environ;
 
+/*
+ * We always need setenv and unsetenv in pairs, because
+ * we don't know how to do memory management on the values set.
+ */
+#ifndef HAVE_UNSETENV
+#undef HAVE_SETENV
+#endif
+
+
 /* These variables are sometimes defined in, *
  * and needed by, the termcap library.       */
 #if MUST_DEFINE_OSPEED


-- 
Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/