Re: [PATCH] mkenvstr: avoid crash in case NULL is given as value

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

On May 19,  8:24pm, Kamil Dudka wrote:
} Subject: [PATCH] mkenvstr: avoid crash in case NULL is given as value
}
} @@ -4582,6 +4582,8 @@ mkenvstr(char *name, char *value, int flags)
}  {
}      char *str, *s;
}      int len_name, len_value;
} +    if (!value)
} +	return NULL;
}  
}      len_name = strlen(name);
}      for (len_value = 0, s = value;

Is it really safe to return NULL from mkenvstr()?  The places where it
is called would seem to imply not, e.g. here ...

                    if (pm->node.flags & PM_SPECIAL)
                        pm->env = mkenvstr (pm->node.nam,
                                            getsparam(pm->node.nam), pm->node.flags);
                    else
                        pm->env = ztrdup(*envp2);
#ifndef USE_SET_UNSET_ENV
                    *envp++ = pm->env;
#endif

... you'd get a spurious NULL in envp, and here ...

     newenv = mkenvstr(pm->node.nam, value, pm->node.flags);
     if (zputenv(newenv)) {

... you'd get an error from zputenv():

    DPUTS(!str, "Attempt to put null string into environment.");

I think rather:
 
diff --git a/Src/params.c b/Src/params.c
index 045ac1e..98541a6 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -4580,17 +4580,21 @@ addenv(Param pm, char *value)
 static char *
 mkenvstr(char *name, char *value, int flags)
 {
-    char *str, *s;
-    int len_name, len_value;
+    char *str, *s = value;
+    int len_name, len_value = 0;
 
     len_name = strlen(name);
-    for (len_value = 0, s = value;
-	 *s && (*s++ != Meta || *s++ != 32); len_value++);
+    if (s)
+	while (*s && (*s++ != Meta || *s++ != 32))
+	    len_value++;
     s = str = (char *) zalloc(len_name + len_value + 2);
     strcpy(s, name);
     s += len_name;
     *s = '=';
-    copyenvstr(s, value, flags);
+    if (value)
+	copyenvstr(s, value, flags);
+    else
+	*++s = '\0';
     return str;
 }
 

(Aside to zsh-workers:  Why is copyenvstr() a function?  It isn't
called anywhere except that one place in mkenvstr() and it has this
strange requirement that its first argument points at the '=' and
not at the end of the string.)


} The crash happens while running a syntax check in ksh emulation mode:
} 
}     ln -s /bin/zsh ksh
}     echo > script.sh
}     ./ksh -n script.sh

Here's an easier way to test:

    ARGV0=ksh zsh -n /dev/null