putenv()/environ bug

putenv()/environ bug

[Sean C. Farley]

As noticed here[1] following a change[2] in FreeBSD's *env() functions,
zsh is mixing *env() (putenv() in this case) functions with direct
access to the environ variable's contents against the IEEE Std 1003.1
specification.  For the FreeBSD port, there is an easy solution in the
thread to tell configure that putenv() does not exist.  I just wanted to
let the zsh development team know about this issue.

Regarding FreeBSD, if you replace the environ pointer, a call to
setenv(), putenv() or unsetenv() will detect this and update itself
internally.  getenv() will use environ until one of the other *env()
functions is called.  If you manipulate environ values after a call to
setenv(), putenv() or unsetenv() is made, then the change will go
unnoticed.

BTW, is there a particular reason the standard *env() functions cannot
be used for all operations to environ if found?

Sean
   1. http://lists.freebsd.org/pipermail/freebsd-current/2007-July/075541.html
   2. http://lists.freebsd.org/pipermail/freebsd-ports/2007-May/041577.html
-- 
scf@FreeBSD.org

Re: putenv()/environ bug

[Peter Stephenson]

On Wed, 25 Jul 2007 10:09:22 -0500 (CDT)
"Sean C. Farley" <scf@FreeBSD.org> wrote:
> As noticed here[1] following a change[2] in FreeBSD's *env() functions,
> zsh is mixing *env() (putenv() in this case) functions with direct
> access to the environ variable's contents against the IEEE Std 1003.1
> specification.
> 
> BTW, is there a particular reason the standard *env() functions cannot
> be used for all operations to environ if found?

There's a long history of fiddling with these for problems on various
systems, so I'm a little unwilling to change it without some guidance.

For example,

    /*
     * Under Cygwin we must use putenv() to maintain consistency.
     * Unfortunately, current version (1.1.2) copies argument and may
     * silently reuse existing environment string. This tries to
     * check for both cases
     */

This is a little confusing since the code in question (addenv in
params.c) doesn't actually use putenv().

Given we manipulate environ quite a lot anyway, is there any harm in
using only the zsh versions of zgetenv() and zputenv()?  There's a
getenv() instead of a zgetenv() in init.c:  I think that was just a typo
by me.

Index: Src/init.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/init.c,v
retrieving revision 1.76
diff -u -r1.76 init.c
--- Src/init.c	6 Jul 2007 21:52:39 -0000	1.76
+++ Src/init.c	25 Jul 2007 20:53:06 -0000
@@ -832,7 +832,7 @@
     if (emulation == EMULATE_ZSH)
 	ptr = home;
     else
-	ptr = getenv("HOME");
+	ptr = zgetenv("HOME");
     if (ptr && ispwd(ptr))
 	pwd = ztrdup(ptr);
     else if ((ptr = zgetenv("PWD")) && (strlen(ptr) < PATH_MAX) &&

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

Re: putenv()/environ bug

[Sean C. Farley]

On Wed, 25 Jul 2007, Peter Stephenson wrote:

> On Wed, 25 Jul 2007 10:09:22 -0500 (CDT)
> "Sean C. Farley" <scf@FreeBSD.org> wrote:
>> As noticed here following a change in FreeBSD's *env() functions, zsh
>> is mixing *env() (putenv() in this case) functions with direct access
>> to the environ variable's contents against the IEEE Std 1003.1
>> specification.
>>
>> BTW, is there a particular reason the standard *env() functions
>> cannot be used for all operations to environ if found?
>
> There's a long history of fiddling with these for problems on various
> systems, so I'm a little unwilling to change it without some guidance.
>
> For example,
>
>    /*
>     * Under Cygwin we must use putenv() to maintain consistency.
>     * Unfortunately, current version (1.1.2) copies argument and may
>     * silently reuse existing environment string. This tries to
>     * check for both cases
>     */

I understand.

> This is a little confusing since the code in question (addenv in
> params.c) doesn't actually use putenv().

Legacy comments are only meant to throw developers off the track.  :)

> Given we manipulate environ quite a lot anyway, is there any harm in
> using only the zsh versions of zgetenv() and zputenv()?  There's a
> getenv() instead of a zgetenv() in init.c:  I think that was just a
> typo by me.

*code snipped*

Unfortunately, this does not fix the problem.  unsets are only affecting
the zsh environment but not environ.  For example, here is a reduced set
of calls to duplicate it:

export FOO=BAR
exec zsh
unset FOO
env | grep FOO
echo $FOO

The call to env (/bin/env) will show "FOO=BAR" while the echo will not.

It is difficult to decide what to do.  Here are some thoughts in my
head:
1. Telling zsh that putenv() does not exist forces it to manipulate a
    new copy of environ (zputenv() and createparamtable()).  This almost
    (see #2) prevents it from mixing *env() functions with direct changes
    to environ.
2. To truly prevent mixing, zsh may need to be told getenv() does not
    exist.  This way it is not directly (see #3) calling any *env()
    functions.  I have not seen a need (any bug reports) for this.
3. There may be other libc functions that call *env() functions.  For
    example, setlocale() calls getenv().  If all setlocale() calls are
    done prior to any manipulation of environ, then this should be safe.
4. The FreeBSD *env() code can detect if environ != prevEnviron but not
    if environ[x] has changed.
5. OpenSolaris does some similar stuff with its *env() functions[1].
    Somebody may want to check if it is also affected.
6. The FreeBSD port[2] should have a fix relatively soon to avoid this
    by telling configure not to look for putenv().

Sean
   1. http://cvs.opensolaris.org/source/xref/clearview/usr/src/lib/libc/port/gen/getenv.c
   2. http://www.freebsd.org/cgi/cvsweb.cgi/ports/shells/zsh/
-- 
scf@FreeBSD.org

Re: putenv()/environ bug

[Andrey Borzenkov]

[-- Attachment #1: Type: text/plain, Size: 2261 bytes --]

On Thursday 26 July 2007, Sean C. Farley wrote:
> On Wed, 25 Jul 2007, Peter Stephenson wrote:
> > On Wed, 25 Jul 2007 10:09:22 -0500 (CDT)
> >
> > "Sean C. Farley" <scf@FreeBSD.org> wrote:
> >> As noticed here following a change in FreeBSD's *env() functions, zsh
> >> is mixing *env() (putenv() in this case) functions with direct access
> >> to the environ variable's contents against the IEEE Std 1003.1
> >> specification.
> >>
> >> BTW, is there a particular reason the standard *env() functions
> >> cannot be used for all operations to environ if found?
> >

The code in question is quite old and I believe it predates (un-)setenv. And 
you simply did not have any way to *unset* environment string in some 
systems; I think (un-)setenv is BSD extension.

> > There's a long history of fiddling with these for problems on various
> > systems, so I'm a little unwilling to change it without some guidance.
> >
> > For example,
> >
> >    /*
> >     * Under Cygwin we must use putenv() to maintain consistency.
> >     * Unfortunately, current version (1.1.2) copies argument and may
> >     * silently reuse existing environment string. This tries to
> >     * check for both cases
> >     */
>
> I understand.
>
> > This is a little confusing since the code in question (addenv in
> > params.c) doesn't actually use putenv().
>
> Legacy comments are only meant to throw developers off the track.  :)
>

Huh? addenv() is using zputenv() that is using putenv() where avaialable. Now 
it may be legacy in the sense Cygwin implemenation has changed; but 
unfortunately I use Cygwin no more nor have environment to check it.

> > Given we manipulate environ quite a lot anyway, is there any harm in
> > using only the zsh versions of zgetenv() and zputenv()?  There's a
> > getenv() instead of a zgetenv() in init.c:  I think that was just a
> > typo by me.
>
> *code snipped*
>
> Unfortunately, this does not fix the problem.  unsets are only affecting
> the zsh environment but not environ.

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.

-andrey

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: putenv()/environ bug

[Sean C. Farley]

On Sat, 28 Jul 2007, Andrey Borzenkov wrote:

> On Thursday 26 July 2007, Sean C. Farley wrote:
>> On Wed, 25 Jul 2007, Peter Stephenson wrote:
>>> On Wed, 25 Jul 2007 10:09:22 -0500 (CDT)
>>>
>>> "Sean C. Farley" <scf@FreeBSD.org> wrote:
>>>> As noticed here following a change in FreeBSD's *env() functions,
>>>> zsh is mixing *env() (putenv() in this case) functions with direct
>>>> access to the environ variable's contents against the IEEE Std
>>>> 1003.1 specification.
>>>>
>>>> BTW, is there a particular reason the standard *env() functions
>>>> cannot be used for all operations to environ if found?
>>>
>
> The code in question is quite old and I believe it predates
> (un-)setenv. And you simply did not have any way to *unset*
> environment string in some systems; I think (un-)setenv is BSD
> extension.

OK.  That would be old.  I cannot think of any relatively current system
that does not have unsetenv().

>>> There's a long history of fiddling with these for problems on
>>> various systems, so I'm a little unwilling to change it without some
>>> guidance.
>>>
>>> For example,
>>>
>>>    /*
>>>     * Under Cygwin we must use putenv() to maintain consistency.
>>>     * Unfortunately, current version (1.1.2) copies argument and may
>>>     * silently reuse existing environment string. This tries to
>>>     * check for both cases
>>>     */
>>
>> I understand.
>>
>>> This is a little confusing since the code in question (addenv in
>>> params.c) doesn't actually use putenv().
>>
>> Legacy comments are only meant to throw developers off the track.  :)
>
> Huh? addenv() is using zputenv() that is using putenv() where
> avaialable. Now it may be legacy in the sense Cygwin implemenation has
> changed; but unfortunately I use Cygwin no more nor have environment
> to check it.

Well, the comment comes after the use of zputenv().  The block of code
under the comment does not use *putenv().

>>> Given we manipulate environ quite a lot anyway, is there any harm in
>>> using only the zsh versions of zgetenv() and zputenv()?  There's a
>>> getenv() instead of a zgetenv() in init.c:  I think that was just a
>>> typo by me.
>>
>> *code snipped*
>>
>> Unfortunately, this does not fix the problem.  unsets are only
>> affecting the zsh environment but not environ.
>
> 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.

Sean
-- 
scf@FreeBSD.org

Re: putenv()/environ bug

[Peter Stephenson]

On Sun, 29 Jul 2007 14:08:42 -0500 (CDT)
"Sean C. Farley" <scf@FreeBSD.org> 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@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: putenv()/environ bug

[Peter Stephenson]

Paranoia.

Index: Test/B02typeset.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/B02typeset.ztst,v
retrieving revision 1.13
diff -u -r1.13 B02typeset.ztst
--- Test/B02typeset.ztst	28 May 2007 22:57:42 -0000	1.13
+++ Test/B02typeset.ztst	30 Jul 2007 20:52:56 -0000
@@ -377,3 +377,14 @@
 >integer local i
 >local tagged scalar
 >preserved
+
+ export ENVFOO=bar
+ print ENVFOO in environment
+ env | grep '^ENVFOO'
+ unset ENVFOO
+ print ENVFOO no longer in environment
+ env | grep '^ENVFOO'
+1:Adding and removing values to and from the environment
+>ENVFOO in environment
+>ENVFOO=bar
+>ENVFOO no longer in environment


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

Re: putenv()/environ bug

[Sean C. Farley]

On Mon, 30 Jul 2007, Peter Stephenson wrote:

> Paranoia.
>
> Index: Test/B02typeset.ztst
> ===================================================================
> RCS file: /cvsroot/zsh/zsh/Test/B02typeset.ztst,v
> retrieving revision 1.13
> diff -u -r1.13 B02typeset.ztst
> --- Test/B02typeset.ztst	28 May 2007 22:57:42 -0000	1.13
> +++ Test/B02typeset.ztst	30 Jul 2007 20:52:56 -0000
> @@ -377,3 +377,14 @@
> >integer local i
> >local tagged scalar
> >preserved
> +
> + export ENVFOO=bar
> + print ENVFOO in environment
> + env | grep '^ENVFOO'
> + unset ENVFOO
> + print ENVFOO no longer in environment
> + env | grep '^ENVFOO'
> +1:Adding and removing values to and from the environment
> +>ENVFOO in environment
> +>ENVFOO=bar
> +>ENVFOO no longer in environment

This test would work even before the patch.  The issue only showed up if
zsh was executed again.

Example:
zsh
export FOO=BAR
exec zsh
unset FOO
env | grep FOO
echo $FOO

Sean
-- 
scf@FreeBSD.org

Re: putenv()/environ bug

[Peter Stephenson]

On Mon, 30 Jul 2007 17:44:12 -0500 (CDT)
"Sean C. Farley" <scf@FreeBSD.org> wrote:
> The issue only showed up if zsh was executed again.
> 
> Example:
> zsh
> export FOO=BAR
> exec zsh
> unset FOO
> env | grep FOO
> echo $FOO

That's useful, I've added it.  I've also made the code a bit more rational
about always using setenv() and unsetenv() in pairs (this shouldn't change
the effect except with a wonky library).

Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.120
diff -u -r1.120 exec.c
--- Src/exec.c	30 Jul 2007 20:46:05 -0000	1.120
+++ Src/exec.c	31 Jul 2007 08:57:21 -0000
@@ -529,7 +529,7 @@
 	 * for ARGV0: that's OK since we're about to exec or exit
 	 * on failure.
 	 */
-#ifdef HAVE_UNSETENV
+#ifdef USE_SET_UNSET_ENV
 	unsetenv("ARGV0");
 #else
 	delenvvalue(z - 6);
Index: Src/params.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/params.c,v
retrieving revision 1.134
diff -u -r1.134 params.c
--- Src/params.c	30 Jul 2007 20:46:05 -0000	1.134
+++ Src/params.c	31 Jul 2007 08:57:21 -0000
@@ -610,7 +610,7 @@
 createparamtable(void)
 {
     Param ip, pm;
-#if !defined(HAVE_PUTENV) && !defined(HAVE_SETENV)
+#if !defined(HAVE_PUTENV) && !defined(USE_SET_UNSET_ENV)
     char **new_environ;
     int  envsize;
 #endif
@@ -665,7 +665,7 @@
 
     setsparam("LOGNAME", ztrdup((str = getlogin()) && *str ? str : cached_username));
 
-#if !defined(HAVE_PUTENV) && !defined(HAVE_SETENV)
+#if !defined(HAVE_PUTENV) && !defined(USE_SET_UNSET_ENV)
     /* 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,7 +3855,7 @@
 int
 zputenv(char *str)
 {
-#ifdef HAVE_SETENV
+#ifdef USE_SET_UNSET_ENV
     /*
      * If we are using unsetenv() to remove values from the
      * environment, which is the safe thing to do, we
@@ -3906,7 +3906,7 @@
 }
 
 /**/
-#ifndef HAVE_UNSETENV
+#ifndef USE_SET_UNSET_ENV
 /**/
 static int
 findenv(char *name, int *pos)
@@ -3969,12 +3969,10 @@
 addenv(Param pm, char *value)
 {
     char *newenv = 0;
-#ifndef HAVE_UNSETENV
+#ifndef USE_SET_UNSET_ENV
     char *oldenv = 0, *env = 0;
     int pos;
-#endif
 
-#ifndef HAVE_UNSETENV
     /*
      * First check if there is already an environment
      * variable matching string `name'.
@@ -3989,7 +3987,7 @@
 	pm->env = NULL;
 	return;
     }
-#ifdef HAVE_UNSETENV
+#ifdef USE_SET_UNSET_ENV
      /*
       * If we are using setenv/unsetenv to manage the environment,
       * we simply store the string we created in pm->env since
@@ -4052,7 +4050,7 @@
  * string.                                         */
 
 
-#ifndef HAVE_UNSETENV
+#ifndef USE_SET_UNSET_ENV
 /**/
 void
 delenvvalue(char *x)
@@ -4078,7 +4076,7 @@
 void
 delenv(Param pm)
 {
-#ifdef HAVE_UNSETENV
+#ifdef USE_SET_UNSET_ENV
     unsetenv(pm->node.nam);
     zsfree(pm->env);
 #else
Index: Src/system.h
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/system.h,v
retrieving revision 1.45
diff -u -r1.45 system.h
--- Src/system.h	30 Jul 2007 20:46:05 -0000	1.45
+++ Src/system.h	31 Jul 2007 08:57:22 -0000
@@ -697,8 +697,8 @@
  * 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
+#if defined(HAVE_SETENV) && defined(HAVE_UNSETENV)
+# define USE_SET_UNSET_ENV
 #endif
 
 
Index: Test/B02typeset.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/B02typeset.ztst,v
retrieving revision 1.14
diff -u -r1.14 B02typeset.ztst
--- Test/B02typeset.ztst	30 Jul 2007 20:55:41 -0000	1.14
+++ Test/B02typeset.ztst	31 Jul 2007 08:57:22 -0000
@@ -381,10 +381,27 @@
  export ENVFOO=bar
  print ENVFOO in environment
  env | grep '^ENVFOO'
+ print Changing ENVFOO
+ ENVFOO="not bar any more"
+ env | grep '^ENVFOO'
  unset ENVFOO
  print ENVFOO no longer in environment
  env | grep '^ENVFOO'
 1:Adding and removing values to and from the environment
 >ENVFOO in environment
 >ENVFOO=bar
+>Changing ENVFOO
+>ENVFOO=not bar any more
 >ENVFOO no longer in environment
+
+ (export FOOENV=BAR
+ env | grep '^FOOENV'
+ print Exec
+ exec $ZTST_testdir/../Src/zsh -c '
+ print Unset
+ unset FOOENV
+ env | grep "^FOOENV"')
+1:Can unset environment variables after exec
+>FOOENV=BAR
+>Exec
+>Unset


.

Re: putenv()/environ bug

[Sean C. Farley]

On Tue, 31 Jul 2007, Peter Stephenson wrote:

> On Mon, 30 Jul 2007 17:44:12 -0500 (CDT)
> "Sean C. Farley" <scf@FreeBSD.org> wrote:
>> The issue only showed up if zsh was executed again.

*snip test case*

> That's useful, I've added it.  I've also made the code a bit more
> rational about always using setenv() and unsetenv() in pairs (this
> shouldn't change the effect except with a wonky library).

*snip patch*

This new patch works too.  Thank you.

Note to FreeBSD 7-CURRENT users:  I will submit a FreeBSD PR, so it will
be patched in the port.

Sean
-- 
scf@FreeBSD.org

Re: putenv()/environ bug

[Sean C. Farley]

On Mon, 30 Jul 2007, Peter Stephenson wrote:

> On Sun, 29 Jul 2007 14:08:42 -0500 (CDT)
> "Sean C. Farley" <scf@FreeBSD.org> 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.

This change fixes the issue concerning the *env() functions in FreeBSD
7-CURRENT.  It works also with 6-STABLE.  Thank you.

Sean

P.S.  I am not subscribed; please keep me in the Cc.
-- 
scf@FreeBSD.org