NetBSD Problem Report #56972

From www@netbsd.org  Thu Aug 18 22:58:44 2022
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id BF4571A921F
	for <gnats-bugs@gnats.NetBSD.org>; Thu, 18 Aug 2022 22:58:43 +0000 (UTC)
Message-Id: <20220818225842.1F9F81A923A@mollari.NetBSD.org>
Date: Thu, 18 Aug 2022 22:58:42 +0000 (UTC)
From: uwe@stderr.spb.ru
Reply-To: uwe@stderr.spb.ru
To: gnats-bugs@NetBSD.org
Subject: sh: read builtin mishandles backslash in input
X-Send-Pr-Version: www-1.0

>Number:         56972
>Category:       bin
>Synopsis:       sh: read builtin mishandles backslash in input
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kre
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Aug 18 23:00:00 +0000 2022
>Closed-Date:    Fri Oct 28 17:58:07 +0000 2022
>Last-Modified:  Fri Oct 28 17:58:07 +0000 2022
>Originator:     Valery Ushakov
>Release:        8-STABLE
>Organization:
>Environment:
NetBSD pony 8.1_STABLE NetBSD 8.1_STABLE (GENERIC) #0: Sat Jun 15 07:30:17 MSK 2019  uwe@sampo:/home/uwe/work/netbsd/build8/obj/macppc/sys/arch/macppc/compile/GENERIC macppc

>Description:
Documentation for the read builtin says that

  By default, unless the -r option is specified, the backslash "\"
  acts as an escape character, causing the following character to be
  treated literally.  If a backslash is followed by a newline, the
  backslash and the newline will be deleted.

However a backslash in the input supplied to read is misinterpreted.

I tested this on both current and 8-stable.

>How-To-Repeat:
bash$ read x y; echo "=$x=" "=$y="
\  y
= = =y=

$ read x y; echo "=$x=" "=$y="
\  y
= y= ==

>Fix:

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: bin-bug-people->kre
Responsible-Changed-By: kre@NetBSD.org
Responsible-Changed-When: Fri, 19 Aug 2022 03:10:50 +0000
Responsible-Changed-Why:
I am looking into this PR


State-Changed-From-To: open->analyzed
State-Changed-By: kre@NetBSD.org
State-Changed-When: Fri, 19 Aug 2022 04:43:16 +0000
State-Changed-Why:
Problem understood.  Fix being tested.

This issue goes back to when read was last touched (not long before
NetBSD 3 was released -- 2005).  Before then read was so broken that
what happened in this area can be considered irrelevant.

This problem occurs when all the characters to be assigned to any
variable (except the last var) are escaped (whether they needed to be or
not).  A single unescaped char in there would hide the problem.  Processing
that was supposed to happen for chars being added to a word (the thing
assigned to any var but the last) was being skipped for escaped chars
which are processed separately.  The fix is trivial.

But while testing here I noticed a problem in this area with the
last var as well.  That is supposed to have trailing IFS whitespace
removed, which was being done ... whether or not that whitespace
had been escaped.   The fix for that is almost as trivial, and
will be done as part of this PR.


From: "Robert Elz" <kre@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56972 CVS commit: src/bin/sh
Date: Fri, 19 Aug 2022 12:17:18 +0000

 Module Name:	src
 Committed By:	kre
 Date:		Fri Aug 19 12:17:18 UTC 2022

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

 Log Message:
 PR bin/56972  Fix escape ('\') handling in sh read builtin.

 In 1.35 (March 2005) (the big read fixup), most escape handling and IFS
 processing in the read builtin was corrected.  However 2 cases were missed,
 one is a word (something to be assigned to any variable but the last) in
 which every character is escaped (the code was relying on a non-escaped char
 to set the "in a word" status), and second trailing IFS whitespace at
 the end of the line was being deleted, even if the chars had been escaped
 (the escape chars are no longer present).

 See the PR for more details (including the case that detected the problem).

 After fixing this, I looked at the FreeBSD code (normally might do it
 before, but these fixes were trivial) to check their implementation.
 Their code does similar things to ours now does, but in a completely
 different way, their read builtin is more complex than ours needs to
 be (they handle more options).   For anyone tempted to simply incorporate
 their code, note that it relies upon infrastructure changes elsewhere
 in the shell, so would not be a simple cut and drop in exercise.

 This needs pullups to -3 -4 -5 -6 -7 -8 and -9 (fortunately this is
 happening before -10 is branched, so will never be broken this way there).


 To generate a diff of this commit:
 cvs rdiff -u -r1.50 -r1.51 src/bin/sh/miscbltin.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: Fri, 19 Aug 2022 12:20:26 +0000
State-Changed-Why:
I believe this is fixed in HEAD now.   Can you test that and
confirm.   I'll submit pullup requests (assuming there isn't
more still to fix) after this has settled a little.


From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56972 CVS commit: [netbsd-9] src/bin/sh
Date: Thu, 27 Oct 2022 16:14:42 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Thu Oct 27 16:14:42 UTC 2022

 Modified Files:
 	src/bin/sh [netbsd-9]: miscbltin.c

 Log Message:
 Pull up following revision(s) (requested by kre in ticket #1549):

 	bin/sh/miscbltin.c: revision 1.51
 	bin/sh/miscbltin.c: revision 1.52

 PR bin/56972  Fix escape ('\') handling in sh read builtin.

 In 1.35 (March 2005) (the big read fixup), most escape handling and IFS
 processing in the read builtin was corrected.  However 2 cases were missed,
 one is a word (something to be assigned to any variable but the last) in
 which every character is escaped (the code was relying on a non-escaped char
 to set the "in a word" status), and second trailing IFS whitespace at
 the end of the line was being deleted, even if the chars had been escaped
 (the escape chars are no longer present).

 See the PR for more details (including the case that detected the problem).

 After fixing this, I looked at the FreeBSD code (normally might do it
 before, but these fixes were trivial) to check their implementation.

 Their code does similar things to ours now does, but in a completely
 different way, their read builtin is more complex than ours needs to
 be (they handle more options).   For anyone tempted to simply incorporate
 their code, note that it relies upon infrastructure changes elsewhere
 in the shell, so would not be a simple cut and drop in exercise.

 This needs pullups to -3 -4 -5 -6 -7 -8 and -9 (fortunately this is
 happening before -10 is branched, so will never be broken this way there).

  -

 Don't output the error for bad usage (no var name given)
 after already writing the prompt (set with the -p option).

 That results in nonsense like:
 	$ read -p foo
 	fooread: arg count

 While here, improve the error message so it means something.

 Now we will get:
 $ read -p foo
 read: variable name required
 Usage: read [-r] [-p prompt] var...

 [Detected by code reading while doing the work for the previous fix]


 To generate a diff of this commit:
 cvs rdiff -u -r1.44 -r1.44.12.1 src/bin/sh/miscbltin.c

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

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56972 CVS commit: [netbsd-8] src/bin/sh
Date: Thu, 27 Oct 2022 16:16:50 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Thu Oct 27 16:16:50 UTC 2022

 Modified Files:
 	src/bin/sh [netbsd-8]: miscbltin.c

 Log Message:
 Pull up following revision(s) (requested by kre in ticket #1779):

 	bin/sh/miscbltin.c: revision 1.51
 	bin/sh/miscbltin.c: revision 1.52

 PR bin/56972  Fix escape ('\') handling in sh read builtin.

 In 1.35 (March 2005) (the big read fixup), most escape handling and IFS
 processing in the read builtin was corrected.  However 2 cases were missed,
 one is a word (something to be assigned to any variable but the last) in
 which every character is escaped (the code was relying on a non-escaped char
 to set the "in a word" status), and second trailing IFS whitespace at
 the end of the line was being deleted, even if the chars had been escaped
 (the escape chars are no longer present).

 See the PR for more details (including the case that detected the problem).

 After fixing this, I looked at the FreeBSD code (normally might do it
 before, but these fixes were trivial) to check their implementation.

 Their code does similar things to ours now does, but in a completely
 different way, their read builtin is more complex than ours needs to
 be (they handle more options).   For anyone tempted to simply incorporate
 their code, note that it relies upon infrastructure changes elsewhere
 in the shell, so would not be a simple cut and drop in exercise.

 This needs pullups to -3 -4 -5 -6 -7 -8 and -9 (fortunately this is
 happening before -10 is branched, so will never be broken this way there).

  -

 Don't output the error for bad usage (no var name given)
 after already writing the prompt (set with the -p option).

 That results in nonsense like:
 	$ read -p foo
 	fooread: arg count

 While here, improve the error message so it means something.

 Now we will get:
 $ read -p foo
 read: variable name required
 Usage: read [-r] [-p prompt] var...

 [Detected by code reading while doing the work for the previous fix]


 To generate a diff of this commit:
 cvs rdiff -u -r1.44 -r1.44.2.1 src/bin/sh/miscbltin.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: kre@NetBSD.org
State-Changed-When: Fri, 28 Oct 2022 17:58:07 +0000
State-Changed-Why:
Feedback Timeout.

Pullups to -8 and -9 have now been made (the earlier ones miss out
unfortunately).


>Unformatted:

Home	PR Database Search