Any comments on users/7883 ?

Any comments on users/7883 ?

[Bart Schaefer]

It fixes a crash bug, so it should either get committed or someone should 
deduce the better fix to which I alluded.

Re: Any comments on users/7883 ?

[Peter Stephenson]

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?  The problem with hn being NULL is because
the later code uses is_builtin which assumes it isn't NULL.  So we need
to take account of is_builtin being set (from the previous time through
the loop) at this point, too.

The error message for 'command -v blub' is now `no such builtin: -v'.
Is that correct?  I don't see any documentation for the fact that the -v
argument isn't handled when posixbuiltins is set.

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	6 Sep 2004 12:01:24 -0000
@@ -1950,7 +1950,7 @@
 		break;
 	    }
 	    if (!(hn = builtintab->getnode(builtintab, cmdarg))) {
-		if (cflags & BINF_BUILTIN) {
+		if ((cflags & BINF_BUILTIN) || is_builtin) {
 		    zwarn("no such builtin: %s", cmdarg, 0);
 		    lastval = 1;
                     opts[AUTOCONTINUE] = oautocont;

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************

Re: Any comments on users/7883 ?

[Oliver Kiddle]

 --- Peter Stephenson <pws@csr.com> wrote: 

> The error message for 'command -v blub' is now `no such builtin: -v'.
> Is that correct?  I don't see any documentation for the fact that the
> -v argument isn't handled when posixbuiltins is set.

The POSIX specification for command -v says: "Otherwise, no output
shall be written and the exit status shall reflect that the name was
not found." So it shouldn't print an error message.

I would have thought that -v should be handled when posixbuiltins is
set. It is only there because of POSIX to begin with.

Oliver


	
	
		
___________________________________________________________ALL-NEW Yahoo! Messenger - all new features - even more fun!  http://uk.messenger.yahoo.com

Re: Any comments on users/7883 ?

[Peter Stephenson]

=?iso-8859-1?q?Oliver=20Kiddle?= wrote:
>  --- Peter Stephenson <pws@csr.com> wrote: 
> 
> > The error message for 'command -v blub' is now `no such builtin: -v'.
> > Is that correct?  I don't see any documentation for the fact that the
> > -v argument isn't handled when posixbuiltins is set.
> 
> The POSIX specification for command -v says: "Otherwise, no output
> shall be written and the exit status shall reflect that the name was
> not found." So it shouldn't print an error message.

That's just because command -v wasn't recognised as a command with an
option.  It works properly when the shell gets that far.

> 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.

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

I still don't properly understand the logic here.

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	6 Sep 2004 13:15:17 -0000
@@ -1939,8 +1939,18 @@
 		return;
 	    }
 
+	    /*
+	     * Quit looking for a command
+	     * - if there was an error, or
+	     * - if we have checked that the hash entry is suitable, or
+	     * - 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.
+	     */
 	    if (errflag || checked ||
-		(unset(POSIXBUILTINS) && (cflags & BINF_COMMAND)))
+		((cflags & BINF_COMMAND) &&
+		 (unset(POSIXBUILTINS) || hn == (HashNode)&commandbn)))
 		break;
 
 	    cmdarg = (char *) peekfirst(args);
@@ -1950,7 +1960,7 @@
 		break;
 	    }
 	    if (!(hn = builtintab->getnode(builtintab, cmdarg))) {
-		if (cflags & BINF_BUILTIN) {
+		if ((cflags & BINF_BUILTIN) || is_builtin) {
 		    zwarn("no such builtin: %s", cmdarg, 0);
 		    lastval = 1;
                     opts[AUTOCONTINUE] = oautocont;

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************

Re: Any comments on users/7883 ?

[Bart Schaefer]

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;
 

Re: Any comments on users/7883 ?

[Peter Stephenson]

Bart Schaefer wrote:
> (It looks like nobody understands this code, me included.  Did Zefram 
> write it, originally?)

Probably, unless it's even older.

> 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.

This may be the best we're going to get.

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

You mean before the patch?

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************

Re: Any comments on users/7883 ?

[Bart Schaefer]

On Tue, 7 Sep 2004, Peter Stephenson wrote:

> This may be the best we're going to get.

OK, I'm going to add something to E01options and then commit it.

> Bart Schaefer wrote:
> 
> > (Is it just me, or is there a crash waiting to happen at line 1828 in the
> > event of a badly written module?)
> 
> You mean before the patch?

Yes, I did mean before.