* [PATCH] mkenvstr: avoid crash in case NULL is given as value @ 2015-05-19 18:24 Kamil Dudka 2015-05-19 23:12 ` Bart Schaefer 0 siblings, 1 reply; 6+ messages in thread From: Kamil Dudka @ 2015-05-19 18:24 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mkenvstr: avoid crash in case NULL is given as value 2015-05-19 18:24 [PATCH] mkenvstr: avoid crash in case NULL is given as value Kamil Dudka @ 2015-05-19 23:12 ` Bart Schaefer 2015-05-20 14:43 ` Kamil Dudka 0 siblings, 1 reply; 6+ messages in thread From: Bart Schaefer @ 2015-05-19 23:12 UTC (permalink / raw) To: Kamil Dudka, zsh-workers 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mkenvstr: avoid crash in case NULL is given as value 2015-05-19 23:12 ` Bart Schaefer @ 2015-05-20 14:43 ` Kamil Dudka 2015-05-20 17:23 ` Bart Schaefer 0 siblings, 1 reply; 6+ messages in thread From: Kamil Dudka @ 2015-05-20 14:43 UTC (permalink / raw) To: Bart Schaefer; +Cc: zsh-workers 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mkenvstr: avoid crash in case NULL is given as value 2015-05-20 14:43 ` Kamil Dudka @ 2015-05-20 17:23 ` Bart Schaefer 2015-05-20 17:35 ` Kamil Dudka 0 siblings, 1 reply; 6+ messages in thread From: Bart Schaefer @ 2015-05-20 17:23 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mkenvstr: avoid crash in case NULL is given as value 2015-05-20 17:23 ` Bart Schaefer @ 2015-05-20 17:35 ` Kamil Dudka 2015-05-20 18:17 ` Kamil Dudka 0 siblings, 1 reply; 6+ messages in thread From: Kamil Dudka @ 2015-05-20 17:35 UTC (permalink / raw) To: Bart Schaefer; +Cc: zsh-workers 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mkenvstr: avoid crash in case NULL is given as value 2015-05-20 17:35 ` Kamil Dudka @ 2015-05-20 18:17 ` Kamil Dudka 0 siblings, 0 replies; 6+ messages in thread From: Kamil Dudka @ 2015-05-20 18:17 UTC (permalink / raw) To: Bart Schaefer; +Cc: zsh-workers 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-05-20 18:17 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-05-19 18:24 [PATCH] mkenvstr: avoid crash in case NULL is given as value Kamil Dudka 2015-05-19 23:12 ` Bart Schaefer 2015-05-20 14:43 ` Kamil Dudka 2015-05-20 17:23 ` Bart Schaefer 2015-05-20 17:35 ` Kamil Dudka 2015-05-20 18:17 ` Kamil Dudka
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/zsh/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).