* Any comments on users/7883 ? @ 2004-09-05 18:13 Bart Schaefer 2004-09-06 12:08 ` Peter Stephenson 0 siblings, 1 reply; 7+ messages in thread From: Bart Schaefer @ 2004-09-05 18:13 UTC (permalink / raw) To: zsh-workers It fixes a crash bug, so it should either get committed or someone should deduce the better fix to which I alluded. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Any comments on users/7883 ? 2004-09-05 18:13 Any comments on users/7883 ? Bart Schaefer @ 2004-09-06 12:08 ` Peter Stephenson 2004-09-06 12:45 ` Oliver Kiddle 0 siblings, 1 reply; 7+ messages in thread From: Peter Stephenson @ 2004-09-06 12:08 UTC (permalink / raw) To: zsh-workers 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 ********************************************************************** ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Any comments on users/7883 ? 2004-09-06 12:08 ` Peter Stephenson @ 2004-09-06 12:45 ` Oliver Kiddle 2004-09-06 13:15 ` Peter Stephenson 0 siblings, 1 reply; 7+ messages in thread From: Oliver Kiddle @ 2004-09-06 12:45 UTC (permalink / raw) To: zsh-workers --- 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Any comments on users/7883 ? 2004-09-06 12:45 ` Oliver Kiddle @ 2004-09-06 13:15 ` Peter Stephenson 2004-09-07 5:24 ` Bart Schaefer 0 siblings, 1 reply; 7+ messages in thread From: Peter Stephenson @ 2004-09-06 13:15 UTC (permalink / raw) To: zsh-workers =?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 ********************************************************************** ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Any comments on users/7883 ? 2004-09-06 13:15 ` Peter Stephenson @ 2004-09-07 5:24 ` Bart Schaefer 2004-09-07 10:25 ` Peter Stephenson 0 siblings, 1 reply; 7+ messages in thread From: Bart Schaefer @ 2004-09-07 5:24 UTC (permalink / raw) To: zsh-workers 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; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Any comments on users/7883 ? 2004-09-07 5:24 ` Bart Schaefer @ 2004-09-07 10:25 ` Peter Stephenson 2004-09-08 7:37 ` Bart Schaefer 0 siblings, 1 reply; 7+ messages in thread From: Peter Stephenson @ 2004-09-07 10:25 UTC (permalink / raw) To: zsh-workers 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 ********************************************************************** ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Any comments on users/7883 ? 2004-09-07 10:25 ` Peter Stephenson @ 2004-09-08 7:37 ` Bart Schaefer 0 siblings, 0 replies; 7+ messages in thread From: Bart Schaefer @ 2004-09-08 7:37 UTC (permalink / raw) To: zsh-workers 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2004-09-08 7:37 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2004-09-05 18:13 Any comments on users/7883 ? Bart Schaefer 2004-09-06 12:08 ` Peter Stephenson 2004-09-06 12:45 ` Oliver Kiddle 2004-09-06 13:15 ` Peter Stephenson 2004-09-07 5:24 ` Bart Schaefer 2004-09-07 10:25 ` Peter Stephenson 2004-09-08 7:37 ` Bart Schaefer
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).