NetBSD Problem Report #52458

From www@NetBSD.org  Thu Aug  3 01:22:43 2017
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id A80B17A20E
	for <gnats-bugs@gnats.NetBSD.org>; Thu,  3 Aug 2017 01:22:43 +0000 (UTC)
Message-Id: <20170803012242.D37F87A26E@mollari.NetBSD.org>
Date: Thu,  3 Aug 2017 01:22:42 +0000 (UTC)
From: idleroux@fastmail.fm
Reply-To: idleroux@fastmail.fm
To: gnats-bugs@NetBSD.org
Subject: \n in prompt breaks history mechanism in /bin/sh
X-Send-Pr-Version: www-1.0

>Number:         52458
>Category:       bin
>Synopsis:       \n in prompt breaks history mechanism in /bin/sh
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    kre
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Aug 03 01:25:00 +0000 2017
>Closed-Date:    Wed Aug 09 11:12:34 +0000 2017
>Last-Modified:  Wed Aug 09 11:12:34 +0000 2017
>Originator:     Ian D. Leroux
>Release:        NetBSD 8.99.1 amd64
>Organization:
>Environment:
NetBSD scrameustache.unrouted.net 8.99.1 NetBSD 8.99.1 (GENERIC) #3: Sun Jul 30 17:08:46 EDT 2017  idleroux@scrameustache.unrouted.net:/build/src/sys/arch/amd64/compile/obj/GENERIC amd64
>Description:
In /bin/sh with command-line editing enabled (either with 'set -o vi' or 'set -o emacs', or one of the equivalent commands), one can normally cycle back through previously entered commands one by one using 'k' in vi command mode, or Ctrl-P (or up-arrow) in emacs mode.

However, if $PS1 contains a newline, the commands are not recorded separately but rather as a single block of text, and attempting to go back to a previous command reloads the entire command history as a single  script, rather than as a series of individually selectable commands.

The problem was introduced recently, in that a system from 2017-06-18 does not exhibit this behavior, while one from 2017-07-30 does.
>How-To-Repeat:
/bin/sh
PS1='
$ '
# run a few example commands, e.g.
ls
date
uname
# now attempt to view history,
# either with ESC-k in vi mode or with Ctrl-P in emacs mode,
# and note that all preceding commands appear as a block.
>Fix:

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: bin-bug-people->kre
Responsible-Changed-By: kre@NetBSD.org
Responsible-Changed-When: Thu, 03 Aug 2017 06:45:44 +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: Sat, 05 Aug 2017 10:44:46 +0000
State-Changed-Why:
This is understood now ... the problem occurred with the PS1 expansion
code (etc.)   That works by treating the prompt (PS1 etc) as an input
string, "reading" it (as if we were inside double quotes, or a here
document that needs expanding) parsing the "line" read, expanding it
(much as if it were an arg on a command line) - then the expanded prompt
is eventually output.   Normally (no \n in the prompt) there's no issue
with any of that, but when there is a \n, the shell needs to make sure
no prompt is emitted, so it just turned the prompting off (this is
a variable with 3 states, "off" "use PS1" and "use PS2").

That by itself would all be fine, but it turns out that the shell
decides after it has read each line, whether to add that line to
the history list as a new entry, or whether to append it to the last
one, by looking to see which prompt it just used, if it was PS1,
then this is a new command line, if it was PS2, then we are continuing
the previous entry (a continuation of the command).   (This only
occurs when reading from the terminal, so prompting would generally
be on - note that PS1='' does not turn off prompting, it simply causes
there to be no characters to output in the prompt when it is issued...)

Where it got complicated was where when the prompt was being re-read
to be expanded, and contained a \n (which normally would be the time
for a new (PS2) prompt to be issued) prompting was turned off.  So,
after the prompt had been issued, instead of the variable saying "PS1
needed (or really, here, just used)" (which is what it had said) it now
says "no prompt" - then when the shell comes to add the line just read
to the history, the prompt did not say "PS1" so it assumed it must have
been "PS2", and appended the line to the previous event instead of making
a new one.

This can be fixed in either of two ways (I tested both, separately).

First, rather than testing "if PS1, new entry, else, append" change it to,
"If not PS2, new entry, else, append" (or if you like, "If PS2, append,
else new entry" which is the same thing with more code churn.)

Alternatively, when the prompt is being expanded, save, and then
restore, the "which prompt" variable, so after the expansion is
done, the state is unchanged.

I will do both ... (better safe!)

While here, I noticed a (semi-related) other bug though, empty
lines are not added to history.   That's fine (and a good thing)
if you're sitting at PS1, and just banging the return key to
avoid the screen saver (or something similar), it is OK, though
probably not really correct, in a command like

	ls |


	wc

where the empty lines did not get added to the history event,
but which does not really matter, as what did get added:

	ls |
	wc

means the same thing, but is completely broken when the command is

	echo 'abc


	def'

where the history would say

	echo 'abc
	def'

which is not the same thing at all.   This is an old bug, which has
been around for a long time, but which can be fixed in a very similar
way to the above ... if the prompt used is PS2, then we will enter
empty lines into the history event (append them) just the same way as
lines that contain something (nb: here, by "empty" I mean a line containing
nothing but the terminating \n, and that \n will be appended to the
history event.)


From: "Robert Elz" <kre@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/52458 CVS commit: src/bin/sh
Date: Sat, 5 Aug 2017 11:33:05 +0000

 Module Name:	src
 Committed By:	kre
 Date:		Sat Aug  5 11:33:05 UTC 2017

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

 Log Message:
 PR bin/52458

 Avoid mangling history when editing is enabled, and the prompt contains a \n

 Also, allow empty input lines into history when they are being appended to
 a previous (partial) command (but not when they would just make an empty entry).

 For all the gory details, see the PR.

 Note nothing here actually makes prompts containing \n work correctly
 when editing is enabled, that's a libedit issue, which will be addressed
 some other time.


 To generate a diff of this commit:
 cvs rdiff -u -r1.60 -r1.61 src/bin/sh/input.c
 cvs rdiff -u -r1.142 -r1.143 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: "Ian D. Leroux" <idleroux@fastmail.fm>
To: gnats-bugs@NetBSD.org
Cc: kre@NetBSD.org, netbsd-bugs@netbsd.org, gnats-admin@netbsd.org
Subject: Re: bin/52458 (\n in prompt breaks history mechanism in /bin/sh)
Date: Sat, 5 Aug 2017 08:31:25 -0400

 Thanks for the thorough explanation of the bug.

 On Sat,  5 Aug 2017 10:44:46 +0000 (UTC) kre@NetBSD.org wrote:
 > This can be fixed in either of two ways (I tested both, separately).
 > 
 > First, rather than testing "if PS1, new entry, else, append" change
 > it to, "If not PS2, new entry, else, append" (or if you like, "If
 > PS2, append, else new entry" which is the same thing with more code
 > churn.)

 Note that this, on its own, would break if I put a '\n' in PS2.  That's
 unlikely to be a problem in practice (I routinely use a newline in PS1,
 but I've never heard of anyone putting one in PS2 and it seems like a
 silly idea).

 > Alternatively, when the prompt is being expanded, save, and then
 > restore, the "which prompt" variable, so after the expansion is
 > done, the state is unchanged.

 That sounds like a more general fix.

 Thanks,

 --
 IDL

From: "Ian D. Leroux" <idleroux@fastmail.fm>
To: gnats-bugs@NetBSD.org
Cc: "Robert Elz" <kre@netbsd.org>, kre@NetBSD.org, gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org
Subject: Re: PR/52458 CVS commit: src/bin/sh
Date: Sat, 5 Aug 2017 18:35:47 -0400

 On Sat,  5 Aug 2017 22:05:01 +0000 (UTC) "Robert Elz" <kre@netbsd.org>
 wrote:
 >  Avoid mangling history when editing is enabled, and the prompt
 > contains a \n 

 Thanks!

 --
 IDL

State-Changed-From-To: analyzed->feedback
State-Changed-By: kre@NetBSD.org
State-Changed-When: Sat, 05 Aug 2017 23:39:48 +0000
State-Changed-Why:
Can this be considered done now ... if it is OK, I will request
a pullup to -8

Nothing here actually makes \n's in prompts work as one might expect
when libedit is in use.


From: "Ian D. Leroux" <idleroux@fastmail.fm>
To: gnats-bugs@NetBSD.org
Cc: kre@NetBSD.org, netbsd-bugs@netbsd.org, gnats-admin@netbsd.org
Subject: Re: bin/52458 (\n in prompt breaks history mechanism in /bin/sh)
Date: Sat, 5 Aug 2017 19:45:35 -0400

 On Sat,  5 Aug 2017 23:39:49 +0000 (UTC) kre@NetBSD.org wrote:
 > Can this be considered done now ... if it is OK, I will request
 > a pullup to -8

 Yes, this PR can be closed.  The libedit problem(s) is, as you said,
 another battle for another day.

 --
 IDL

State-Changed-From-To: feedback->pending-pullups
State-Changed-By: kre@NetBSD.org
State-Changed-When: Sun, 06 Aug 2017 00:18:03 +0000
State-Changed-Why:
pullup-8 #199


From: "Soren Jacobsen" <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/52458 CVS commit: [netbsd-8] src/bin/sh
Date: Wed, 9 Aug 2017 05:35:19 +0000

 Module Name:	src
 Committed By:	snj
 Date:		Wed Aug  9 05:35:19 UTC 2017

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

 Log Message:
 Pull up following revision(s) (requested by kre in ticket #199):
 	bin/sh/input.c: revision 1.61
 	bin/sh/parser.c: revision 1.143
 PR bin/52458
 Avoid mangling history when editing is enabled, and the prompt contains a \n
 Also, allow empty input lines into history when they are being appended to
 a previous (partial) command (but not when they would just make an empty entry)
 .
 For all the gory details, see the PR.
 Note nothing here actually makes prompts containing \n work correctly
 when editing is enabled, that's a libedit issue, which will be addressed
 some other time.


 To generate a diff of this commit:
 cvs rdiff -u -r1.56.2.1 -r1.56.2.2 src/bin/sh/input.c
 cvs rdiff -u -r1.132.2.1 -r1.132.2.2 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: pending-pullups->closed
State-Changed-By: kre@NetBSD.org
State-Changed-When: Wed, 09 Aug 2017 11:12:34 +0000
State-Changed-Why:
Completed.


>Unformatted:

Home	PR Database Search