Some general problems with exporting cariables (Was: TERMCAP problem. )

["Andrej Borsenkow" <Andrej.Borsenkow@mow.siemens.ru>]

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

>
>
> >
> > It's changed since then.  If you have HAVE_PUTENV defined in config.h, try
> > undefining it for something a bit more like (but not identical to) the old
> > behaviour to see if that makes a difference.
> >
>
> I believe, I know what happens. When zsh creates parameters from
> environment,
> it splits env string in-place;

Well, it turned out to be not directly related.  What actually happens is,
when zsh initially (in creatparamtable) gets env string for predefined colon
array it finally calls colnarrsetfn that unconditionally calls arrfixenv().
This one does not even check if variable is exported but simply tries to
replace environment string  with replenv() that tries to free previous
environment string ... that resides somewhere in global memory.

After we looped over env's all is clean again - every env string was allocated
by Zsh. But the above simply happens too early.

Ironically, when Zsh did not copy original env strings it appeared to work
(but who knows, what memory corruption could it cause). But when I wrote a
patch to copy original string, zsh started to crash.

I do not know, if it was my fault or was there before. I probably have not
changed usage of replenv() bit may have changed it's semantic. Anyway, as is
stands now, there seems to be no point in having it at all - either variable
is exported and has PM_EXPORTED set or it is not exported and does not have
this flag. If it is not true, something is seriously broken anyway. So, this
patch removes all replenv() usage with checking for PM_EXPORTED and addenv().
I still removed env string in-place modification just in case.

Koen, could you test if it helps? It appears to work here, but so did
unmodified Zsh as well. I appreciate, if anybody could review memory
allocation usage here - I still do not grok completely Zsh memory usage.

The patch is against current CVS.

-andrej

[-- Attachment #2: zsh.replenv.diff --]
[-- Type: application/octet-stream, Size: 3840 bytes --]

Index: Src/params.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/params.c,v
retrieving revision 1.30
diff -u -r1.30 params.c
--- Src/params.c	2001/01/16 13:44:20	1.30
+++ Src/params.c	2001/01/17 18:34:13
@@ -446,6 +446,32 @@
 	return NULL;
 }
 
+/*
+ * Split environment string into (name, vlaue) pair.
+ * this is used to avoid in-place editing of environment table
+ * that results in core dump on some systems
+ */
+
+static int
+split_env_string(char *env, char **name, char **value)
+{
+    char *str, *tenv;
+
+    if (!env || !name || !value)
+	return 0;
+
+    tenv = strcpy(zhalloc(strlen(env) + 1), env);
+    for (str = tenv; *str && *str != '='; str++)
+	;
+    if (str != tenv && *str == '=') {
+	*str = '\0';
+	*name = tenv;
+	*value = str + 1;
+	return 1;
+    } else
+	return 0;
+}
+    
 /* Set up parameter hash table.  This will add predefined  *
  * parameter entries as well as setting up parameter table *
  * entries for environment variables we inherit.           */
@@ -460,7 +486,7 @@
     int  envsize;
 #endif
     char **envp, **envp2, **sigptr, **t;
-    char buf[50], *str, *iname, *hostnam;
+    char buf[50], *str, *iname, *ivalue, *hostnam;
     int  oae = opts[ALLEXPORT];
 #ifdef HAVE_UNAME
     struct utsname unamebuf;
@@ -513,20 +539,18 @@
     environ = new_environ;
 #endif
 
+    /* Use heap allocation to avoid many small alloc/free calls */
+    pushheap();
+
     /* Now incorporate environment variables we are inheriting *
      * into the parameter hash table. Copy them into dynamic   *
      * memory so that we can free them if needed               */
     for (envp = envp2 = environ; *envp2; envp2++) {
-	for (str = *envp2; *str && *str != '='; str++);
-	if (*str == '=') {
-	    iname = NULL;
-	    *str = '\0';
-	    if (!idigit(**envp2) && isident(*envp2) && !strchr(*envp2, '[')) {
-		iname = *envp2;
+	if (split_env_string(*envp2, &iname, &ivalue)) {
+	    if (!idigit(*iname) && isident(iname) && !strchr(iname, '[')) {
 		if ((!(pm = (Param) paramtab->getnode(paramtab, iname)) ||
 		     !(pm->flags & PM_DONTIMPORT || pm->flags & PM_EXPORTED)) &&
-		    (pm = setsparam(iname, metafy(str + 1, -1, META_DUP)))) {
-		    *str = '=';
+		    (pm = setsparam(iname, metafy(ivalue, -1, META_DUP)))) {
 		    pm->flags |= PM_EXPORTED;
 		    if (pm->flags & PM_SPECIAL)
 			pm->env = mkenvstr (pm->nam,
@@ -536,9 +560,9 @@
 		    *envp++ = pm->env;
 		}
 	    }
-	    *str = '=';
 	}
     }
+    popheap();
     *envp = '\0';
     opts[ALLEXPORT] = oae;
 
@@ -1503,12 +1527,16 @@
 			pm->flags, NULL);
     else
 	val = pm->gets.cfn(pm);
+#if 0  /* looks like replenv is evil and should be removed */
     if (pm->env)
 	pm->env = replenv(pm->nam, val, pm->flags);
     else {
+#endif
 	pm->flags |= PM_EXPORTED;
 	pm->env = addenv(pm->nam, val, pm->flags);
+#if 0
     }
+#endif
 }
 
 /**/
@@ -2867,20 +2895,31 @@
     char *u;
     Param pm;
 
+    if (t == path)
+	cmdnamtab->emptytable(cmdnamtab);
+
     pm = (Param) paramtab->getnode(paramtab, s);
+    
     /*
+     * Do not "fix" parameters that were not exported
+     */
+
+    if (!(pm->flags & PM_EXPORTED) && !isset(ALLEXPORT))
+	return;
+
+    /*
      * Only one level of a parameter can be exported.  Unless
      * ALLEXPORT is set, this must be global.
      */
-    if (t == path)
-	cmdnamtab->emptytable(cmdnamtab);
     if (pm->flags & PM_HASHELEM)
 	return;
     u = t ? zjoin(t, ':', 1) : "";
+#if 0 /* attempt to get rid of evil replenv */
     if (findenv(s, 0)) {
 	pm->env = replenv(s, u, pm->flags);
 	return;
     }
+#endif
     if (isset(ALLEXPORT))
 	pm->flags |= PM_EXPORTED;
     if (pm->flags & PM_EXPORTED)