Re: Any comments on users/7883 ?

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

On Mon, 6 Sep 2004, Peter Stephenson wrote:

> Bart Schaefer wrote:
> > It fixes a crash bug, so it should either get committed or someone should 
> > deduce the better fix to which I alluded.
> 
> Isn't this the correct fix?

I don't think so ...

(It looks like nobody understands this code, me included.  Did Zefram 
write it, originally?)

> The problem with hn being NULL is because the later code uses is_builtin 
> which assumes it isn't NULL.

No, the problem is that hn started out valid and then was improperly 
clobbered with a NULL.  The question is whether to simply prevent the
clobbering, or to avoid the entire code path that includes clobbering.

[Oliver's reply elided because Peter already quoted it.]

On Mon, 6 Sep 2004, Peter Stephenson wrote:

> =?iso-8859-1?q?Oliver=20Kiddle?= wrote:
> > I would have thought that -v should be handled when posixbuiltins is
> > set. It is only there because of POSIX to begin with.
> 
> I think this is the answer.  It may be just an oversight.  The second
> hunk now doesn't get reached, but the change seems right anyway.

I think you found the right "if" branch test to modify, but I'm not yet 
convinced you've got the test right.  Skipping ahead to the patch:

> +	     * Quit looking for a command
> +	     * - if there was an error, or
> +	     * - if we have checked that the hash entry is suitable, or

"we have checked that the hash entry is suitable" is not AFAICT what the 
"checked" boolean means.  If we've found any hash entry, it's always 
"suitable," again AFAICT.  Rather, "checked" means that it's OK to apply 
MAGIC_EQUAL_SUBST, according to the comment up around line 1805.  (Maybe 
its semantics have been overloaded?)

> +	     * - if we are using the command prefix and either
> +	     *   - we are not using POSIXBUILTINS, or
> +	     *   - we have determined there are options which would
> +	     *     require us to use the command builtin.

This is the key bit here.  I think the right fix is to check "is_builtin"
not "hn == (HashNode)&commandbn" -- and that it's not necessary to && it
with (cflags & BINF_COMMAND) the way you have.

There are only two ways "is_builtin" could be true at that point.  One,
we've loaded a builtin from a module (line 1821); two, we've decided we
need commandbn (line 1848).  In either case it's unecessary to call
builtintab->getnode() again, although in the former case it's harmless.

(Is it just me, or is there a crash waiting to happen at line 1828 in the
event of a badly written module?)

> The test hn == (HashNode)&commandbn now gets an error message about
> `dereferencing type-punned pointer', even though it's obviously never
> dereferenced.

On some hardware architectures, even loading a pointer into a register to 
compare it to another pointer is equivalent (in terms of causing faults) 
to a dereference.  Or so says the logic behind C99.

> I still don't properly understand the logic here.

I think among the three of us we're finally getting close.  Another patch
for consideration (I've dropped your second hunk because now it's really
impossible for is_builtin to be true at that point):

Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.70
diff -u -r1.70 exec.c
--- Src/exec.c	3 Sep 2004 09:47:49 -0000	1.70
+++ Src/exec.c	7 Sep 2004 05:22:15 -0000
@@ -1825,7 +1825,7 @@
 		    load_module(((Builtin) hn)->optstr);
 		    hn = builtintab->getnode(builtintab, cmdarg);
 		}
-		assign = (hn->flags & BINF_MAGICEQUALS);
+		assign = (hn && (hn->flags & BINF_MAGICEQUALS));
 		break;
 	    }
 	    cflags &= ~BINF_BUILTIN & ~BINF_COMMAND;
@@ -1939,7 +1939,18 @@
 		return;
 	    }
 
-	    if (errflag || checked ||
+	    /*
+	     * Quit looking for a command if:
+	     * - there was an error; or
+	     * - we checked the simple cases needing MAGIC_EQUAL_SUBST; or
+	     * - we know we already found a builtin (because either:
+	     *   - we loaded a builtin from a module, or
+	     *   - we have determined there are options which would
+	     *     require us to use the "command" builtin); or
+	     * - we aren't using POSIX and so BINF_COMMAND indicates a zsh
+	     *   precommand modifier is being used in place of the builtin
+	     */
+	    if (errflag || checked || is_builtin ||
 		(unset(POSIXBUILTINS) && (cflags & BINF_COMMAND)))
 		break;