* [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).