PATCH: fix memory leak in new setenv code

PATCH: fix memory leak in new setenv code

[Oliver Kiddle]

I noticed using Solaris libumem and mdb that zsh is leaking memory
every time an environment variable is modified. I traced this down to
some relatively recent change to use setenv(3) and the fact that the
old value was not being freed. I think the patch below is the correct
fix.

Oliver

--- Src/params.c.orig	Wed Oct 31 17:30:57 2007
+++ Src/params.c	Wed Oct 31 17:28:49 2007
@@ -3998,6 +3998,8 @@
       * the other branch?  If so, we don't actually need to
       * store pm->env at all, just a flag that the value was set.
       */
+     if (pm->env)
+         zsfree(pm->env);
      pm->env = newenv;
 #else
     /*

Re: PATCH: fix memory leak in new setenv code

[Geoff Wing]

On Thursday 2007-11-01 03:39 +1100, Oliver Kiddle output:
:I noticed using Solaris libumem and mdb that zsh is leaking memory
:every time an environment variable is modified. I traced this down to
:some relatively recent change to use setenv(3) and the fact that the
:old value was not being freed. I think the patch below is the correct
:fix.
:
:Oliver
:
:--- Src/params.c.orig	Wed Oct 31 17:30:57 2007
:+++ Src/params.c	Wed Oct 31 17:28:49 2007
:@@ -3998,6 +3998,8 @@
:       * the other branch?  If so, we don't actually need to
:       * store pm->env at all, just a flag that the value was set.
:       */
:+     if (pm->env)
:+         zsfree(pm->env);
:      pm->env = newenv;
: #else
:     /*
:

This can't be right and corrupts my environment (using zsh memory
routines).  Running "env" spews out lots of bad stuff.

If we're supposed to be zsfree()ing that, why aren't we doing it
consistently, i.e. around the line which assigns newenv and before
the call to zputenv()?  Maybe it should be managed differently?

Regards,

Re: PATCH: fix memory leak in new setenv code

[Bart Schaefer]

On Nov 22,  1:57pm, Geoff Wing wrote:
} Subject: Re: PATCH: fix memory leak in new setenv code
}
} On Thursday 2007-11-01 03:39 +1100, Oliver Kiddle output:
} :
} :--- Src/params.c.orig	Wed Oct 31 17:30:57 2007
} :+++ Src/params.c	Wed Oct 31 17:28:49 2007
} :@@ -3998,6 +3998,8 @@
} :       * the other branch?  If so, we don't actually need to
} :       * store pm->env at all, just a flag that the value was set.
} :       */
} :+     if (pm->env)
} :+         zsfree(pm->env);
} :      pm->env = newenv;
} : #else
} :     /*
} :
} 
} This can't be right and corrupts my environment (using zsh memory
} routines).  Running "env" spews out lots of bad stuff.

I've just spent a few minutes looking at this.

newenv comes from mkenvstr() which allocates with zalloc(), and is
then assigned to pm->env.

pm->env() does indeed point to a string allocated with zalloc(), so
it should be correct to zsfree().

However, I think createparamtable() is incompletely converted to the
USE_SET_UNSET_ENV case, so that the initial environment is not being
properly maintained.  In particular we have *envp pointing into the
global environ, which is modified in the "incorporate environment
variables we are inheriting" loop (look around line 695 in params.c).

This could cause setenv() to leak or corrupt memory.  I'm not quite
sure about the patch below, but it seems to match the other cases, to
wit, the memory in pm->env is not shared with the global environ [the
latter instead being left unchanged in that loop in createparamtable()
and thereafter mananged only by setenv()/unsetenv()].
 
} If we're supposed to be zsfree()ing that, why aren't we doing it
} consistently, i.e. around the line which assigns newenv and before
} the call to zputenv()?

newenv is a newly allocated string, which (in the USE_SET_UNSET_ENV
case) is then copied into the environment.  If that fails (zputenv
returns nonzero) we discard it; there probably should be a call to
zsfree(pm->env) before pm->env = NULL in that branch as well.  I'm
not sure whether pm->env = NULL is correct there, though; failure of 
zputenv means newenv was not added to the environment, but does it
also mean that the previous value of the parameter is no longer in
the enviroment, or does it mean that the environment was unchanged?

So the patch below is at best incomplete.  Try it with Oliver's
patch still in place and see if it resolves your corruption problem.

--- current/Src/params.c	2007-11-01 08:23:52.000000000 -0700
+++ Src/params.c	2007-11-21 23:30:46.000000000 -0800
@@ -692,13 +692,17 @@
 					    getsparam(pm->node.nam), pm->node.flags);
 		    else
 			pm->env = ztrdup(*envp2);
+#ifndef USE_SET_UNSET_ENV
 		    *envp++ = pm->env;
+#endif
 		}
 	    }
 	}
     }
     popheap();
+#ifndef USE_SET_UNSET_ENV
     *envp = '\0';
+#endif
     opts[ALLEXPORT] = oae;
 
     if (emulation == EMULATE_ZSH)

Re: PATCH: fix memory leak in new setenv code

[Geoff Wing]

On Thursday 2007-11-22 18:41 +1100, Bart Schaefer output:
:On Nov 22,  1:57pm, Geoff Wing wrote:
[...]
:     the memory in pm->env is not shared with the global environ [the
:latter instead being left unchanged in that loop in createparamtable()
:and thereafter mananged only by setenv()/unsetenv()].

OK, I guess that addresses my concern there.

:So the patch below is at best incomplete.  Try it with Oliver's
:patch still in place and see if it resolves your corruption problem.

Thanks, it does.

Regards,
Geoff