NetBSD Problem Report #48489

From dholland@netbsd.org  Thu Jan  2 04:22:04 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 19930A6465
	for <gnats-bugs@gnats.NetBSD.org>; Thu,  2 Jan 2014 04:22:04 +0000 (UTC)
Message-Id: <20140102042203.9F6AC14A1E4@mail.netbsd.org>
Date: Thu,  2 Jan 2014 04:22:03 +0000 (UTC)
From: dholland@netbsd.org
Reply-To: dholland@netbsd.org
To: gnats-bugs@gnats.NetBSD.org
Subject: sh(1) allows trailing && and ||
X-Send-Pr-Version: 3.95

>Number:         48489
>Category:       bin
>Synopsis:       sh(1) allows trailing && and ||
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    kre
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Jan 02 04:25:00 +0000 2014
>Closed-Date:    Sun Jun 12 03:47:21 +0000 2016
>Last-Modified:  Sun Jun 12 03:47:21 +0000 2016
>Originator:     David A. Holland
>Release:        NetBSD 6.99.28 (20131210)
>Organization:
>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 trailing && or ||, which ought to be a syntax error.

>How-To-Repeat:

sh -c 'true &&'
sh -c 'true ||'

Both ksh and bash report a syntax error. For that matter, so do csh
and tcsh.

zsh does not, but zsh can't really be taken as an authoritative
reference.

>Fix:

dunno, the sh parser is an eldritch horror

>Release-Note:

>Audit-Trail:
From: Miwa Susumu <miwarin@gmail.com>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: bin/48489
Date: Sun, 12 Oct 2014 20:19:06 +0900

 as a specification, I think that the newline of zero or more are
 present in the back of the '&&' and are allowed.
 What about?

 IEEE Std 1003.1, 2004 Edition - 2.10 Shell Grammar
 http://pubs.opengroup.org/onlinepubs/009604599/utilities/xcu_chap02.html#tag_02_10

 '&&' is AND_IF. '||' is OR_IF.

   :
 and_or           :                         pipeline
                  | and_or AND_IF linebreak pipeline
                  | and_or OR_IF  linebreak pipeline
   :
 linebreak        : newline_list
                  | /* empty */


 -- 
 miwarin

From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@www.NetBSD.org
Cc: 
Subject: Re: bin/48489 (sh(1) allows trailing && and ||)
Date: Wed, 24 Feb 2016 23:08:28 +0700

  | sh accepts trailing && or ||, which ought to be a syntax error.

 Yes, it should be, but does it really matter?

 Miwa Susumu's reading of the grammar in the reply in the PR
 was mistaken - it is certainly true that an && or || may be followed
 by a \n, but that (whether that appears or not) they must be followed by
 a pipeline, and that can't be empty.

 Now (since the past hour or so) our sh(1) documents this odd behaviour,
 describes it as useless (which it is) - but at least it should not
 be a surprise.

 sh also allows an empty command before && or || - which it also
 shouldn't, but again, does anyone really care.  This one has been
 documented in sh(1) for ages.

 And yes, taking these two oddities together means that
 	sh -c '&&'
 does not generate any errors, and returns an exit code of 0.

 No-one should rely upon this, it is not portable, but it is also harmless,
 I think.

 If anyone really believes it is worth spending time on, I could look and
 see if I can adjust the parser to avoid this, but unless there is some
 good reason ("comply with the std" is not) to do it, I think there are
 other bugs, that actually affect rational operation, that I can better
 spend time on.

 Is the doc of this sufficient for this PR to be closed?

 kre

State-Changed-From-To: open->feedback
State-Changed-By: wiz@NetBSD.org
State-Changed-When: Mon, 04 Apr 2016 12:38:01 +0000
State-Changed-Why:
Man page was updated, is that enough?


State-Changed-From-To: feedback->open
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Wed, 06 Apr 2016 02:32:47 +0000
State-Changed-Why:
No. Accepting invalid syntax is still a bug -- sooner or later it causes
a problem where an invalid script is accidentally accepted and then breaks
elsewhere later.

There's a reason this was marked non-critical/low though.


From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: bin/48489 (sh(1) allows trailing && and ||)
Date: Wed, 06 Apr 2016 17:10:45 +0700

     Date:        Wed,  6 Apr 2016 02:32:47 +0000 (UTC)
     From:        dholland@NetBSD.org
     Message-ID:  <20160406023247.DA6AA7AAA0@mollari.NetBSD.org>

   | No. Accepting invalid syntax is still a bug

 OK, I will keep it on my list as something to eventually fix.

   | There's a reason this was marked non-critical/low though.

 So it won't necessarily be high up the list...

 kre

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: bin/48489 (sh(1) allows trailing && and ||)
Date: Mon, 2 May 2016 01:03:17 +0000

 On Wed, Apr 06, 2016 at 02:32:47AM +0000, dholland@NetBSD.org wrote:
  > No. Accepting invalid syntax is still a bug -- sooner or later it causes
  > a problem where an invalid script is accidentally accepted and then breaks
  > elsewhere later.

 btw, as I had vaguely thought, this isn't hypothetical. The motivation
 for filing the PR was this:

 http://mail-index.netbsd.org/pkgsrc-changes/2014/01/02/msg099418.html

 -- 
 David A. Holland
 dholland@netbsd.org

Responsible-Changed-From-To: bin-bug-people->kre
Responsible-Changed-By: kre@NetBSD.org
Responsible-Changed-When: Sun, 08 May 2016 09:46:49 +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: Sun, 08 May 2016 09:46:49 +0000
State-Changed-Why:
The parser will soon be modified to forbid empty simple commands
(ones containing absolutely nothing).   Patch being tested now.


From: "Robert Elz" <kre@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/48489 CVS commit: src/bin/sh
Date: Mon, 9 May 2016 20:36:07 +0000

 Module Name:	src
 Committed By:	kre
 Date:		Mon May  9 20:36:07 UTC 2016

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

 Log Message:
 PR bin/48489 -- Shell "simple commands" are now not allowed to be
 completely empty, they must have something (var assignment, redirect,
 or command, or multiple of those) to avoid a syntax error.  This
 matches the requirements of the grammar in the standard.   Correct the
 parser (using FreeBSD's sh as a guide) and update the man page to
 remove text that noted a couple of places this was previously OK.

 OK christos@


 To generate a diff of this commit:
 cvs rdiff -u -r1.118 -r1.119 src/bin/sh/parser.c
 cvs rdiff -u -r1.121 -r1.122 src/bin/sh/sh.1

 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: Mon, 09 May 2016 22:06:55 +0000
State-Changed-Why:
Is this fixed to your satisfaction in current now?


From: "Robert Elz" <kre@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/48489 CVS commit: src/bin/sh
Date: Wed, 1 Jun 2016 02:47:06 +0000

 Module Name:	src
 Committed By:	kre
 Date:		Wed Jun  1 02:47:05 UTC 2016

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

 Log Message:
 PR bin/51145 PR bin/48489
 More fixes to the shell parser to prevent empty simple commands (where
 empty means no significant text at all) - as discussed on tech-userlevel
 this still allows { } (which can be useful in function definitions, not
 really anywhere else though) except in posix mode.  ( ) now generates
 a syntax error, as should any other place where commands are required but
 nothing is present.  (nb, redirections, var assignments, even var expansions
 that expand to nothing, are all OK, and avoid the error - just comments, or
 whits space, are not.)    This is (aside from allowing { } at all) all in
 accordance with the posix spec.


 To generate a diff of this commit:
 cvs rdiff -u -r1.119 -r1.120 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: feedback->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Sun, 12 Jun 2016 03:47:21 +0000
State-Changed-Why:
yes, seems fine


>Unformatted:

Home	PR Database Search