From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3368 invoked from network); 7 Sep 2004 05:24:21 -0000 Received: from news.dotsrc.org (HELO a.mx.sunsite.dk) (130.225.247.88) by ns1.primenet.com.au with SMTP; 7 Sep 2004 05:24:21 -0000 Received: (qmail 81783 invoked from network); 7 Sep 2004 05:24:15 -0000 Received: from sunsite.dk (130.225.247.90) by a.mx.sunsite.dk with SMTP; 7 Sep 2004 05:24:15 -0000 Received: (qmail 28479 invoked by alias); 7 Sep 2004 05:24:12 -0000 Mailing-List: contact zsh-workers-help@sunsite.dk; run by ezmlm Precedence: bulk X-No-Archive: yes X-Seq: 20325 Received: (qmail 28459 invoked from network); 7 Sep 2004 05:24:10 -0000 Received: from news.dotsrc.org (HELO a.mx.sunsite.dk) (130.225.247.88) by sunsite.dk with SMTP; 7 Sep 2004 05:24:10 -0000 Received: (qmail 81530 invoked from network); 7 Sep 2004 05:24:09 -0000 Received: from moonbase.zanshin.com (64.84.47.139) by a.mx.sunsite.dk with SMTP; 7 Sep 2004 05:24:07 -0000 Received: from toltec.zanshin.com (toltec.zanshin.com [64.84.47.166]) by moonbase.zanshin.com (8.13.1/8.13.1) with ESMTP id i875O5Yx023711 for ; Mon, 6 Sep 2004 22:24:05 -0700 Date: Mon, 6 Sep 2004 22:24:05 -0700 (PDT) From: Bart Schaefer Reply-To: zsh-workers@sunsite.dk To: zsh-workers@sunsite.dk Subject: Re: Any comments on users/7883 ? In-Reply-To: <200409061315.i86DFvFC028391@news01.csr.com> Message-ID: References: <20040906124533.58856.qmail@web25002.mail.ukl.yahoo.com> <200409061315.i86DFvFC028391@news01.csr.com> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Spam-Checker-Version: SpamAssassin 2.63 on a.mx.sunsite.dk X-Spam-Level: X-Spam-Status: No, hits=0.0 required=6.0 tests=none autolearn=no version=2.63 X-Spam-Hits: 0.0 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;