NetBSD Problem Report #48498

From dholland@netbsd.org  Sat Jan  4 16:24:20 2014
Return-Path: <dholland@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	(using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "Postmaster NetBSD.org" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 2321DA6464
	for <gnats-bugs@gnats.NetBSD.org>; Sat,  4 Jan 2014 16:24:20 +0000 (UTC)
Message-Id: <20140104162419.AB5A414A230@mail.netbsd.org>
Date: Sat,  4 Jan 2014 16:24:19 +0000 (UTC)
From: dholland@netbsd.org
Reply-To: dholland@netbsd.org
To: gnats-bugs@gnats.NetBSD.org
Subject: sh(1) parser oddity
X-Send-Pr-Version: 3.95

>Number:         48498
>Category:       bin
>Synopsis:       sh(1) parser oddity
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    kre
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Jan 04 16:25:00 +0000 2014
>Closed-Date:    Thu Jul 27 17:31:54 +0000 2017
>Last-Modified:  Thu Jul 27 19:05:00 +0000 2017
>Originator:     David A. Holland
>Release:        NetBSD 6.99.28 (20131210)
>Organization:
dholland@netbsd.org
>Environment:
System: NetBSD valkyrie 6.99.28 NetBSD 6.99.28 (VALKYRIE) #14: Tue Dec 10 15:15:49 EST 2013  dholland@valkyrie:/usr/src/sys/arch/amd64/compile/VALKYRIE amd64
Architecture: x86_64
Machine: amd64
>Description:

sh accepts some curious constructions; while it doesn't accept a plain
"fi", it is happy with a loose "fi" as long as it follows something else:

valkyrie% sh -c 'true; fi'
valkyrie% sh -c 'false; fi'
valkyrie% 

Similarly, it accepts "then" without "if" as long as it isn't first:

valkyrie% sh -c 'false; then echo wut'
wut
valkyrie% sh -c 'true; then echo wut'
wut
valkyrie% 

Same for "do":

valkyrie% sh -c 'true; do echo wut'
wut
valkyrie% 

For that matter you don't even need to have a command there:

valkyrie% sh -c 'true; then'
valkyrie% sh -c 'true; do'
valkyrie% 

Needless to say, ksh, bash, and zsh don't allow this sort of thing.

>How-To-Repeat:

>Fix:

torches and pitchforks

>Release-Note:

>Audit-Trail:
From: Nat Sloss <nathanialsloss@yahoo.com.au>
To: "gnats-bugs" <gnats-bugs@netbsd.org>
Cc: 
Subject: Re: bin/48498
Date: Mon, 6 Jan 2014 16:23:54 +1100

 Hi,
 I noticed this same behavior in NetBSD 6.1.1

 It seems as though if you separate commands by newlines it returns in error
 as expected.  ie sh -c "true^Jdo" will return in error as it should so I think 
 that sh should do the same for ; as it does for the new line character.

 So I have the following patch I still have to test it thoroughly though.

 Index: bin/sh/parser.c
 ===================================================================
 RCS file: /cvsroot/src/bin/sh/parser.c,v
 retrieving revision 1.80
 diff -u -r1.80 parser.c
 --- bin/sh/parser.c	31 Aug 2011 16:24:55 -0000	1.80
 +++ bin/sh/parser.c	6 Jan 2014 05:14:16 -0000
 @@ -186,7 +186,7 @@
  		switch (tok) {
  		case TBACKGND:
  		case TSEMI:
 -			tok = readtoken();
 +			tok = TNL;
  			/* fall through */
  		case TNL:
  			if (tok == TNL) {

 NB: This patch is my own work which I submit under the NetBSD license.

 Regards,

 Nat. 

Responsible-Changed-From-To: bin-bug-people->kre
Responsible-Changed-By: kre@NetBSD.org
Responsible-Changed-When: Tue, 10 May 2016 02:42:50 +0000
Responsible-Changed-Why:
I am (for now) handling this PR


State-Changed-From-To: open->analyzed
State-Changed-By: kre@NetBSD.org
State-Changed-When: Tue, 10 May 2016 02:42:50 +0000
State-Changed-Why:
I am looking into this one, and think I might have a suitable
patch (which is not anything like the one suggested, sorry)
Testing now...


From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: bin/48498: sh(1) parser oddity
Date: Tue, 25 Jul 2017 11:11:40 +0700

 Note that this PR and PR bin/52426 (issue 1) are the same bug, an
 unexpected reserved word after a ';' is simply disregared.

 It is (finally) about to be properly fixed...

From: "Robert Elz" <kre@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/48498 CVS commit: src/tests/bin/sh
Date: Wed, 26 Jul 2017 17:50:21 +0000

 Module Name:	src
 Committed By:	kre
 Date:		Wed Jul 26 17:50:21 UTC 2017

 Modified Files:
 	src/tests/bin/sh: t_syntax.sh

 Log Message:
 PR bin/48498   PR bin/52426

 Add two new sh syntax test cases to check for bug fixes for the parser
 problems (syntax errors unidentified) reported in the two PRs.

 In the latter case, also include some tests that test similar
 looking, valid, syntax - to make sure the fix for the PR does not
 break code that should not fail.  This is not needed for the earlier
 PR as other tests, and just normal parsing, is sufficient.

 These tests have been verified to fail with a current /bin/sh and to
 pass with the sh updates that are to be committed soon -- and because that
 fix is expected within hours, the tests are not marked as expected to
 fail, just let them actually fail for now.


 To generate a diff of this commit:
 cvs rdiff -u -r1.5 -r1.6 src/tests/bin/sh/t_syntax.sh

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Robert Elz" <kre@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/48498 CVS commit: src/bin/sh
Date: Wed, 26 Jul 2017 23:09:41 +0000

 Module Name:	src
 Committed By:	kre
 Date:		Wed Jul 26 23:09:41 UTC 2017

 Modified Files:
 	src/bin/sh: parser.c

 Log Message:
 PR bin/48498   PR bin/52426

 Don't ignore unexpected reserved words after ';'
 Don't allow any random token type as a case stmt pattern, only a word.
 	Those are ancient ash bugs and do not affect correct scripts.

 Don't ignore redirects in a case stmt list where the list is nothing but
 redirects (if the pattern matches, the redirects should be performed).
 	That was introduced when a redirect only case stmt list was allowed
 	(older shells had generated a syntax error.)

 Random cleanups/refactoring taken from or inspired by the FreeBSD sh
 parser ...  use makename() consistently to create a NARG node - we
 were using it in a couple of places but most NARG node creation was open
 coded.  Introduce consumetoken() (from FreeBSD) to handle the fairly
 common case where exactly one token type must come next, and we need to
 check that, and skip past it when found (or error) and linebreak() (new)
 to handle places where optional \n's are permitted.
 Both previously open coded.

 Simplify list() by removing its second arg, which was only ever used when
 handling the end of a `` (old style command substitution).  Simply move
 the code from inside list() to just after its call in the `` case (from
 FreeBSD.)


 To generate a diff of this commit:
 cvs rdiff -u -r1.141 -r1.142 src/bin/sh/parser.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: analyzed->feedback
State-Changed-By: kre@NetBSD.org
State-Changed-When: Wed, 26 Jul 2017 23:11:23 +0000
State-Changed-Why:
I believe this is fixed now, do you agree?


State-Changed-From-To: feedback->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Thu, 27 Jul 2017 17:31:54 +0000
State-Changed-Why:
yes
(I haven't tested it but I'm sure you've fixed it thoroughly and not
papered it over; so there isn't much point)


From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: bin/48498 (sh(1) parser oddity)
Date: Fri, 28 Jul 2017 02:00:36 +0700

     Date:        Thu, 27 Jul 2017 17:31:55 +0000 (UTC)
     From:        dholland@NetBSD.org
     Message-ID:  <20170727173155.1BAB57A290@mollari.NetBSD.org>

   | (I haven't tested it but I'm sure you've fixed it thoroughly and not
   | papered it over; so there isn't much point)

 I believe the fix is correct, yes, but in this case not "I" - the heavy
 lifting part of the fix came from FreeBSD (I did some cut&paste...)

>Unformatted:

Home	PR Database Search