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

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

[Kamil Dudka]

The crash happens while running a syntax check in ksh emulation mode:

    ln -s /bin/zsh ksh
    echo > script.sh
    ./ksh -n script.sh

Originally reported at <https://bugzilla.redhat.com/1222867>.
---
 Src/params.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Src/params.c b/Src/params.c
index 045ac1e..1df97c6 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -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;
-- 
2.4.1

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

[Bart Schaefer]

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

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

[Kamil Dudka]

On Tuesday 19 May 2015 16:12:22 Bart Schaefer wrote:
> 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.");

Good catch!  Then your patch certainly looks as a better choice to me.

Kamil

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

[Bart Schaefer]

On May 20,  4:43pm, Kamil Dudka wrote:
}
} Good catch!  Then your patch certainly looks as a better choice to me.

It belatedly occurs to me that adding

    if (!value)
	value = "";

would have been a sufficient change.  Oh, well.

-- 
Barton E. Schaefer

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

[Kamil Dudka]

On Wednesday 20 May 2015 10:23:00 Bart Schaefer wrote:
> On May 20,  4:43pm, Kamil Dudka wrote:
> }
> } Good catch!  Then your patch certainly looks as a better choice to me.
> 
> It belatedly occurs to me that adding
> 
>     if (!value)
> 	value = "";
> 
> would have been a sufficient change.  Oh, well.

I am quite new to zsh code but this drives me to a question: Are the strings 
allocated in mkenvstr() freed anywhere?

Kamil

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

[Kamil Dudka]

On Wednesday 20 May 2015 19:35:13 Kamil Dudka wrote:
> On Wednesday 20 May 2015 10:23:00 Bart Schaefer wrote:
> > On May 20,  4:43pm, Kamil Dudka wrote:
> > }
> > } Good catch!  Then your patch certainly looks as a better choice to me.
> > 
> > It belatedly occurs to me that adding
> > 
> >     if (!value)
> > 	
> > 	value = "";
> > 
> > would have been a sufficient change.  Oh, well.
> 
> I am quite new to zsh code but this drives me to a question: Are the strings
> allocated in mkenvstr() freed anywhere?

Oops, it is a misleading question anyway.  It would return an allocated string 
in both cases.  Sorry for the noise.

> Kamil