NetBSD Problem Report #48631

From kre@munnari.OZ.AU  Sun Mar  2 14:06:53 2014
Return-Path: <kre@munnari.OZ.AU>
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" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 7A1E4A5831
	for <gnats-bugs@gnats.NetBSD.org>; Sun,  2 Mar 2014 14:06:53 +0000 (UTC)
Message-Id: <201403020548.s225mpSb003775@perseus.noi.kre.to>
Date: Sun, 2 Mar 2014 12:48:51 +0700 (ICT)
From: kre@munnari.OZ.AU
To: gnats-bugs@gnats.NetBSD.org
Subject: sh synerrs case where list to execute is entirely redirections (+FIX?)
X-Send-Pr-Version: 3.95

>Number:         48631
>Category:       bin
>Synopsis:       sh synerrs case where list to execute is entirely redirections (+FIX?)
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    bin-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Mar 02 14:10:00 +0000 2014
>Closed-Date:    Thu Jun 09 15:33:05 +0000 2016
>Last-Modified:  Thu Jun 09 15:33:05 +0000 2016
>Originator:     Robert Elz
>Release:        NetBSD -current (6.99.31 or whatever) (and all earlier?)
>Organization:
	Prince of Songkla University
>Environment:
System: NetBSD perseus.noi.kre.to 6.99.17 NetBSD 6.99.17 (GENERIC) #1: Fri Feb 22 22:09:50 ICT 2013 kre@jade.coe.psu.ac.th:/usr/obj/current/i386/sys/arch/i386/compile/GENERIC i386
Architecture: i386
Machine: i386
>Description:
	sh cannot correctly parse a case statement where the commands
	to execute (after any pattern) consist entirely of redirections.

	That is, something like
		case $var in
		create)	>$filename ;;
		esac

	If the ">$filename" is missing (empty list) all is OK, as it
	is if there is some other command there.

	Other shells (ksh, bash at least) don't have this problem, and
	I see nothing in the sh standard that would suggest this should
	not be legal syntax.

>How-To-Repeat:
	echo 'case foo in x) >/tmp/foobar ;; esac' | /bin/sh

	Also:
	echo 'case foo in x) >/tmp/foobar esac' | /bin/sh

	Expect to see:
		sh: Syntax error: ";;" unexpected
	in the first case and
		sh: Syntax error: end of file unexpected (expecting ";;")
	in the second.

	Both work if the redirect is omitted.

	This is a syntax error, so whether the case word matches the
	pattern or not is irrelevant - by forcing no match in the
	test cases above, you don't need to worry if you have a /tmp/foobar,
	it won't get clobbered anyway...

>Fix:
	The following patch (down below) corrects the first problem for me.

	Notes:

	1) I have not run sh tests, I am not sure that the patch does
	   not allow illegal syntax in cases where it should not, though
	   I have been unable to find any in a few (try stuff) tests I
	   did attempt.

	2) I have actually tested this in a sh from an older version of
	   6.99.x than is current - then I downloaded the most recent
	   version of the file affected (parser.c) and applied the same
	   fix for the purposes of sending this patch.   That (ie: this)
	   version has not even been compile tested, though I did check
	   the diffs fairly carefully, and I believe this to be as I
	   intended.

	3) This patch does not allow the ;; to be omitted (the second case
	   above), which should be legal before the esac, but I don't use
	   that (I always include the ;;) so I didn't bother looking for a
	   fix that would handle that syntax as well.   I don't feel all
	   that bad about that, neither ksh (NetBSD's /bin/ksh anyway)
	   nor bash handle this either....

	Hence: please DO NOT commit this patch without actually checking
	that it works!   (And I won't be in the slightest offended if
	someone decides there's a much better way to fix the problem than
	this one).

	Apply this patch to parser.c (the filenames in the patch won't help!)
	in src/bin/sh of current.

	When verified correct, the fix (Whatever it ends up being) for this
	should probably be pulled up to all other currently supported versions.

	ps: I have no idea what it would mean to have a ! with just a
	redirect, so I don't know if the "checkneg" goto is really
	correct, but when there's no ! it works... (or seems to).

--- parser.c-current	2014-03-01 16:17:05.000000000 +0700
+++ parser.c-current-fixed	2014-03-02 12:06:27.000000000 +0700
@@ -513,8 +513,14 @@
 	case TRP:
 		tokpushback++;
 		n1 = simplecmd(rpp, redir);
 		goto checkneg;
+	case TENDCASE:
+		if (redir) {
+			tokpushback++;
+			goto checkneg;
+		}
+		/* FALLTHROUGH */
 	default:
 		synexpect(-1);
 		/* NOTREACHED */
 	}

>Release-Note:

>Audit-Trail:
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/48631 CVS commit: src/bin/sh
Date: Mon, 22 Feb 2016 14:38:10 -0500

 Module Name:	src
 Committed By:	christos
 Date:		Mon Feb 22 19:38:10 UTC 2016

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

 Log Message:
 Finish the fix for PR/48631 - that is, make the parser correctly
 handle the token syntax it really should be handling (including
 some that posix does not require, but is right anyway.) This is
 quite similar to, and to some extent inspired by the way the FreeBSD
 sh parser.c works, but the actual implementation is quite different.
 (from kre)


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

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

From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/48631 CVS commit: src/bin/sh
Date: Mon, 22 Feb 2016 14:42:46 -0500

 Module Name:	src
 Committed By:	christos
 Date:		Mon Feb 22 19:42:46 UTC 2016

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

 Log Message:
 Fix for PR bin/48631 - allow commands controlled
 by case statements to be nothing more than redirects (from kre)


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

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

From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: PR/48631 CVS commit: src/bin/sh
Date: Tue, 23 Feb 2016 12:54:45 +0700

     Date:        Mon, 22 Feb 2016 19:40:01 +0000 (UTC)
     From:        "Christos Zoulas" <christos@netbsd.org>
     Message-ID:  <20160222194001.242077ACDA@mollari.NetBSD.org>

 Thanks for installing all of that Christos, but this one...

   | The following reply was made to PR bin/48631; it has been noted by GNATS.

   |  Modified Files:
   |  	src/bin/sh: parser.c
   |  
   |  Log Message:
   |  Finish the fix for PR/48631 - that is, make the parser correctly
   |  handle the token syntax it really should be handling [...]

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

 Really belongs to PR 50287 (which I think can be closed now, unless
 someone considers it important enough a bug (any part of the fix) to
 be worth pulling up ... if so, I'd suggest just the part that was
 applied last week, which fixes the initial issue, this one is a fairly
 significant (internal) change, and fixes truly esoteric cases, though
 it is needed for the fix for PR 43469, as it is currently implemented
 anyway, and that fix might be important enough to pull up.)

 kre

From: christos@zoulas.com (Christos Zoulas)
To: gnats-bugs@NetBSD.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org, 
	kre@munnari.OZ.AU
Cc: 
Subject: Re: PR/48631 CVS commit: src/bin/sh
Date: Tue, 23 Feb 2016 07:38:45 -0500

 On Feb 23,  5:55am, kre@munnari.OZ.AU (Robert Elz) wrote:
 -- Subject: Re: PR/48631 CVS commit: src/bin/sh

 |  Really belongs to PR 50287 (which I think can be closed now, unless
 |  someone considers it important enough a bug (any part of the fix) to
 |  be worth pulling up ... if so, I'd suggest just the part that was
 |  applied last week, which fixes the initial issue, this one is a fairly
 |  significant (internal) change, and fixes truly esoteric cases, though
 |  it is needed for the fix for PR 43469, as it is currently implemented
 |  anyway, and that fix might be important enough to pull up.)

 50287 is an X/wxGTK30 application bug which is already closed.
 Are you sure this is right?

 christos

From: Robert Elz <kre@munnari.OZ.AU>
To: christos@zoulas.com (Christos Zoulas)
Cc: gnats-bugs@NetBSD.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: PR/48631 CVS commit: src/bin/sh
Date: Tue, 23 Feb 2016 19:52:50 +0700

     Date:        Tue, 23 Feb 2016 07:38:45 -0500
     From:        christos@zoulas.com (Christos Zoulas)
     Message-ID:  <20160223123845.73CC317FDAD@rebar.astron.com>

   | 50287 is an X/wxGTK30 application bug which is already closed.
   | Are you sure this is right?

 Obviously not ... every time I hand type one of these numbers I get it wrong!

 It was meant to be (pasted this time...) 50827

 I got it wrong in the patches I sent you, and then in the e-mail earlier.
 All started from when I made a (local) sub-dir to keep the modified sources
 that would generate that particular patch, and then just got copied into
 the patch file name, the e-mail I sent you, and then the one I sent the list...

 Sigh.

 Sorry.

 kre

From: christos@zoulas.com (Christos Zoulas)
To: gnats-bugs@NetBSD.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org, 
	kre@munnari.OZ.AU
Cc: 
Subject: Re: PR/48631 CVS commit: src/bin/sh
Date: Tue, 23 Feb 2016 08:06:18 -0500

 On Feb 23, 12:55pm, kre@munnari.OZ.AU (Robert Elz) wrote:
 -- Subject: Re: PR/48631 CVS commit: src/bin/sh

 |  Obviously not ... every time I hand type one of these numbers I get it wrong!
 |  
 |  It was meant to be (pasted this time...) 50827
 |  
 |  I got it wrong in the patches I sent you, and then in the e-mail earlier.
 |  All started from when I made a (local) sub-dir to keep the modified sources
 |  that would generate that particular patch, and then just got copied into
 |  the patch file name, the e-mail I sent you, and then the one I sent the list...

 You are in good company; I've had the propensity to shuffle numbers (specially
 dates) all my life. Anyway, 50827 closed.

 christos

State-Changed-From-To: open->closed
State-Changed-By: kre@NetBSD.org
State-Changed-When: Thu, 09 Jun 2016 15:33:05 +0000
State-Changed-Why:
Fixed back in Feb in parser.c 1.97


>Unformatted:

Home	PR Database Search