PATCH: Fix segfaults with exec options

PATCH: Fix segfaults with exec options

[Frank Terbeck]

Hey list,

Someone on IRC noticed that zsh flakes out, if you're using the `exec'
builtin with somewhat bogus options. Like this:

% exec -a foo
% exec -l
% exec -c

Turns out that the issue is located in `execcmd()' in `exec.c'. The code
does "nextnode(firstnode(args))" a couple of times and it turns out,
that if used like above (which doesn't make sense, but crashing is
hardly the proper way to react) "firstnode(args)" can be NULL and when
that is accessed by the `nextnode' macro, the shell crashes.

There's a similar problem, when "firstnode(args)" is passed to
`uremnode()', which accesses the memory at that position without testing
for NULL, too.

I've prepared a rather hackish patch that seems to fix the issue.

I've CC:ed Phil, who appears to be the original author of the code
because I don't know the code very well and `execcmd()', being the 1000+
lines function it is, intimidates me a little...

Maybe he (or someone else) has an idea about how to fix this at a more
fundamental level. To me, my fix just plugs some holes with duct tape.

Regards, Frank


diff --git a/Src/exec.c b/Src/exec.c
index 9b3c503..e1bf881 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -2487,6 +2487,11 @@ execcmd(Estate state, int input, int output, int how, int last1)
 		 * with the zsh style.
 		 */
 		while (next && *next == '-' && strlen(next) >= 2) {
+		    if (!firstnode(args)) {
+			zerr("exec requires command to execute");
+			errflag = lastval = 1;
+			return;
+		    }
 		    uremnode(args, firstnode(args));
 		    if (!strcmp(next, "--"))
 			break;
@@ -2499,6 +2504,11 @@ execcmd(Estate state, int input, int output, int how, int last1)
 				/* position on last non-NULL character */
 				cmdopt += strlen(cmdopt+1);
 			    } else {
+				if (!firstnode(args)) {
+				    zerr("exec requires command to execute");
+				    errflag = lastval = 1;
+				    return;
+				}
 				if (!nextnode(firstnode(args))) {
 				    zerr("exec flag -a requires a parameter");
 				    errflag = lastval = 1;
@@ -2521,7 +2531,8 @@ execcmd(Estate state, int input, int output, int how, int last1)
 			    return;
 			}
 		    }
-		    next = (char *) getdata(nextnode(firstnode(args)));
+		    if (firstnode(args) && nextnode(firstnode(args)))
+			next = (char *) getdata(nextnode(firstnode(args)));
 		}
 		if (exec_argv0) {
 		    char *str, *s;

Re: PATCH: Fix segfaults with exec options

[Peter Stephenson]

On Mon, 16 Jan 2012 14:55:11 +0100
Frank Terbeck <ft@bewatermyfriend.org> wrote:
> Maybe he (or someone else) has an idea about how to fix this at a more
> fundamental level. To me, my fix just plugs some holes with duct tape.

Looks OK to me.  There isn't a more fundamental level --- it needs an
argument and you're intercepting the point where it finds out it doesn't
have one.

This is different from most "real" builtins, which have a handler that
parses options and counts arguments.  As exec is special that's done at
this point; there's no higher level parsing involved.

-- 
Peter Stephenson <pws@csr.com>            Software Engineer
Tel: +44 (0)1223 692070                   Cambridge Silicon Radio Limited
Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, UK


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog

Re: PATCH: Fix segfaults with exec options

[Phil Pennock]

On 2012-01-16 at 14:55 +0100, Frank Terbeck wrote:
> I've CC:ed Phil, who appears to be the original author of the code
> because I don't know the code very well and `execcmd()', being the 1000+
> lines function it is, intimidates me a little...

Erk.  Sorry.  :(  *blush*  Stupid mistake on my part.

> Maybe he (or someone else) has an idea about how to fix this at a more
> fundamental level. To me, my fix just plugs some holes with duct tape.

The "options for keywords" stuff is all a bit of a hack, since
fundamentally they're not supposed to take options, but bash does and
various tools assume you can ssh with certain command-line constructs,
so these were put in for interoperability.

Sorry for the lack of corner case analysis on my part.
-- 
https://twitter.com/syscomet