Re: PATCH: fix memory leak in new setenv code

[Bart Schaefer <schaefer@brasslantern.com>]

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)