Bug 169773 - [libedit] Resizing causes /bin/sh to repeat edit operations
Summary: [libedit] Resizing causes /bin/sh to repeat edit operations
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 10.0-CURRENT
Hardware: Any Any
: Normal Affects Only Me
Assignee: freebsd-bugs (Nobody)
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2012-07-10 22:30 UTC by Peter Jeremy
Modified: 2018-05-20 23:53 UTC (History)
0 users

See Also:


Attachments
libedit_error_handling.patch (928 bytes, patch)
2012-09-06 20:03 UTC, Mark Johnston
no flags Details | Diff
libedit_error_handling.patch (2.24 KB, patch)
2012-09-08 22:29 UTC, Mark Johnston
no flags Details | Diff
1.diff (5.33 KB, patch)
2012-09-10 15:56 UTC, Steffen "Daode" Nurpmeso
no flags Details | Diff
2.diff (2.76 KB, patch)
2012-09-10 15:56 UTC, Steffen "Daode" Nurpmeso
no flags Details | Diff
3.diff (1.12 KB, patch)
2012-09-10 15:56 UTC, Steffen "Daode" Nurpmeso
no flags Details | Diff
4.diff (1.17 KB, patch)
2012-09-10 15:56 UTC, Steffen "Daode" Nurpmeso
no flags Details | Diff
5.diff (4.44 KB, patch)
2012-09-10 15:56 UTC, Steffen "Daode" Nurpmeso
no flags Details | Diff
6.diff (625 bytes, patch)
2012-09-10 15:56 UTC, Steffen "Daode" Nurpmeso
no flags Details | Diff
0001-Fix-editline-3-char-read-and-errno-code-flow.patch (5.20 KB, patch)
2012-09-11 17:10 UTC, Steffen "Daode" Nurpmeso
no flags Details | Diff
0002-Add-an-EL_RESTART_READ-option-to-editline-3.patch (3.76 KB, patch)
2012-09-11 17:10 UTC, Steffen "Daode" Nurpmeso
no flags Details | Diff
0003-Set-EL_RESTART_READ-in-interactive-sh-1-sessions.patch (984 bytes, patch)
2012-09-11 17:10 UTC, Steffen "Daode" Nurpmeso
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Jeremy 2012-07-10 22:30:14 UTC
	Given an account with /bin/sh as the login shell, if you type
	some input, ending with an editing operation (eg backspace) and
	then resize the window multiple times without further input,
	the edit operation will be repeated on every second resize.

	This bug was identified during testing of r238173 but it
	predates the libedit changes in r237448.

Fix: 

Unknown
How-To-Repeat: 	Login to an account with /bin/sh as the login shell on a FreeBSD host.
	Type some text, ending with a backspace (deleting the last character
	entered).
	Resize the window (or send SIGWINCH to the shell) multiple times
	without entering additional input.
	The last character will be deleted (stortening the line by one
	character) on every second resize.
Comment 1 Mark Johnston 2012-09-06 20:03:36 UTC
The root of the problem is some questionable error-handling in libedit.

First, read_char() doesn't do the right thing if its call to
read(2) is interrupted by a signal (SIGWINCH in this case). Basically,
it retries the read once, and returns an error if the second read failed
(which is why it takes two SIGWINCHs to trigger the delete). IMHO,
read_char() should always retry the read(2) if its interrupted by a
signal. The attached patched fixes this.

The second problem has to do with how read(2) errors are handled higher
up in libedit's stack. We have read_getcmd(), which calls el_getc(),
which calls the read_char() function mentioned above. read_char()
returns -1 on error, and then -1 gets returned to read_getcmd() by
el_getc(). el_getc() returns OKCMD on success, and OKCMD is defined to
be... -1. Thus read_getcmd() has no idea that an error occured and ends
up reusing a local variable (cmdnum) which causes the second backspace.

I think the fix here is to define OKCMD to be 1 (the value returned by
read_char() on success). For the purpose of fixing the bug reported
here, the check for EINTR is enough, but this second bug should probably
be fixed too. I'm happy to test alternative fixes. =)

Thanks,
-Mark
Comment 2 Eitan Adler freebsd_committer freebsd_triage 2012-09-06 20:26:57 UTC
Responsible Changed
From-To: freebsd-bugs->jilles

over to maintainer
Comment 3 Mark Linimon 2012-09-08 18:23:22 UTC
----- Forwarded message from Steffen Daode Nurpmeso <sdaoden@gmail.com> -----

Date: Sat, 08 Sep 2012 17:44:56 +0200
From: Steffen Daode Nurpmeso <sdaoden@gmail.com>
To: freebsd-bugs@FreeBSD.org
Subject: Re: bin/169773: sh(1): Resizing causes /bin/sh to repeat edit
	operations

 |Synopsis: sh(1): Resizing causes /bin/sh to repeat edit operations
 |
 |http://www.freebsd.org/cgi/query-pr.cgi?pr=169773

Oh, what a mess :)
I agree with Mark that the handling of OKCMD is simply wrong, but
it turned out to be wrong to simply use a different value for it.
Also the passing through of errno is incomplete in there.  (The
documented interface doesn't state anything about errno or useful
error handling at all, and however!)

It's a rather quick first diff for editline(3), i have no more
time but since it took almost three hours to come that far someone
else may build on top of it (or simply try it or ... wait for a
second).

I *think* it effectively results in editline(3) behaving the way
it is supposed to work (retrying once after a whatever signal,
then failing for a second one).  Since el_gets() now fails (as it
is supposed to), sh(1) will behave wrong in that the current line
gets "thrown away".  (I guess the supposed way is to temporarily
adjust PROMPT if one wants to continue what is on the line yet?
But i still have no idea of editline(3) :->)

It would be better if editline(3) could be configured to simply
restart upon EINTR, or to fixate that behaviour (for FreeBSD)?
I don't think it is acceptable to loose a line of user content due
to a simple resize?
So long and ciao,

--steffen

diff --git a/lib/libedit/read.c b/lib/libedit/read.c
index 7d7f54b..5b51577 100644
--- a/lib/libedit/read.c
+++ b/lib/libedit/read.c
@@ -238,8 +238,7 @@ read_getcmd(EditLine *el, el_action_t *cmdnum, char *ch)
 	el->el_errno = 0;
 	do {
 		if ((num = el_getc(el, ch)) != 1) {	/* if EOF or error */
-			el->el_errno = num == 0 ? 0 : errno;
-			return (num);
+			return (num < 0 ? 1 : 0);
 		}
 
 #ifdef	KANJI
@@ -294,16 +293,18 @@ read_char(EditLine *el, char *cp)
 
  again:
 	el->el_signal->sig_no = 0;
-	while ((num_read = read(el->el_infd, cp, 1)) == -1) {
+	while ((num_read = read(el->el_infd, cp, 1)) < 0) {
+		int e = errno;
 		if (el->el_signal->sig_no == SIGCONT) {
 			sig_set(el);
 			el_set(el, EL_REFRESH);
 			goto again;
 		}
-		if (!tried && read__fixio(el->el_infd, errno) == 0)
+		if (! tried && read__fixio(el->el_infd, e) == 0)
 			tried = 1;
 		else {
 			*cp = '\0';
+			errno = e;
 			return (-1);
 		}
 	}
@@ -369,8 +370,10 @@ el_getc(EditLine *el, char *cp)
 	(void) fprintf(el->el_errfile, "Reading a character\n");
 #endif /* DEBUG_READ */
 	num_read = (*el->el_read.read_char)(el, cp);
+	if (num_read < 0)
+		el->el_errno = errno;
 #ifdef DEBUG_READ
-	(void) fprintf(el->el_errfile, "Got it %c\n", *cp);
+	(void) fprintf(el->el_errfile, "Got <%c> (return %d)\n", *cp, num_read);
 #endif /* DEBUG_READ */
 	return (num_read);
 }
@@ -511,6 +514,7 @@ el_gets(EditLine *el, int *nread)
 #endif /* DEBUG_EDIT */
 		/* if EOF or error */
 		if ((num = read_getcmd(el, &cmdnum, &ch)) != OKCMD) {
+			num = -1;
 #ifdef DEBUG_READ
 			(void) fprintf(el->el_errfile,
 			    "Returning from el_gets %d\n", num);

_______________________________________________
freebsd-bugs@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-bugs
To unsubscribe, send any mail to "freebsd-bugs-unsubscribe@freebsd.org"


----- End forwarded message -----
Comment 4 Mark Johnston 2012-09-08 22:29:44 UTC
On Sat, Sep 08, 2012 at 05:44:56PM +0200, Steffen Daode Nurpmeso wrote:
>  |Synopsis: sh(1): Resizing causes /bin/sh to repeat edit operations
>  |
>  |http://www.freebsd.org/cgi/query-pr.cgi?pr=169773
> 
> Oh, what a mess :)
> I agree with Mark that the handling of OKCMD is simply wrong, but
> it turned out to be wrong to simply use a different value for it.
> Also the passing through of errno is incomplete in there.  (The
> documented interface doesn't state anything about errno or useful
> error handling at all, and however!)
> 
> It's a rather quick first diff for editline(3), i have no more
> time but since it took almost three hours to come that far someone
> else may build on top of it (or simply try it or ... wait for a
> second).

I took a closer look at the patch... I think the errno handling is
mostly ok, except for a couple of places in el_gets() where the return
value of read_char() isn't stored, and the code ends up looking at an
uninitialized variable. The attached patch is your patch + the fix for
this.

> 
> I *think* it effectively results in editline(3) behaving the way
> it is supposed to work (retrying once after a whatever signal,
> then failing for a second one).  Since el_gets() now fails (as it
> is supposed to), sh(1) will behave wrong in that the current line
> gets "thrown away".  (I guess the supposed way is to temporarily
> adjust PROMPT if one wants to continue what is on the line yet?
> But i still have no idea of editline(3) :->)
> 
> It would be better if editline(3) could be configured to simply
> restart upon EINTR, or to fixate that behaviour (for FreeBSD)?
> I don't think it is acceptable to loose a line of user content due
> to a simple resize?
> So long and ciao,

Maybe we need a new option for el_set() which sets a flag in
el->el_signal that determines whether various functions return on EINTR.
libfetch for example has fetchRestartCalls for this purpose. It's not
really clear to me why anyone would want EINTR as an error and return
though.

-Mark
Comment 5 Steffen "Daode" Nurpmeso 2012-09-10 15:56:35 UTC
Mark Johnston <markjdb@gmail.com> wrote:

 |On Sat, Sep 08, 2012 at 05:44:56PM +0200, Steffen Daode Nurpmeso wrote:
 |>  |Synopsis: sh(1): Resizing causes /bin/sh to repeat edit operations
 |>  |
 |>  |http://www.freebsd.org/cgi/query-pr.cgi?pr=169773
 |> 
 [.]
 |> It's a rather quick first diff for editline(3), i have no more
 [.]
 |
 |I took a closer look at the patch... I think the errno handling is
 |mostly ok, except for a couple of places in el_gets() where the return
 |value of read_char() isn't stored, and the code ends up looking at an
 |uninitialized variable. The attached patch is your patch + the fix for
 |this.
 |
 |> I *think* it effectively results in editline(3) behaving the way
 |> it is supposed to work (retrying once after a whatever signal,
 |> then failing for a second one).  Since el_gets() now fails (as it
 |> is supposed to), sh(1) will behave wrong in that the current line
 [.]
 |> 
 |> It would be better if editline(3) could be configured to simply
 |> restart upon EINTR, or to fixate that behaviour (for FreeBSD)?
 |> I don't think it is acceptable to loose a line of user content due
 |> to a simple resize?
 |> So long and ciao,
 |
 |Maybe we need a new option for el_set() which sets a flag in
 |el->el_signal that determines whether various functions return on EINTR.
 |libfetch for example has fetchRestartCalls for this purpose. It's not
 |really clear to me why anyone would want EINTR as an error and return
 |though.

I have implemented a EL_READRESTART option for editline(3), which
seems to be the easiest approach to get around this.
Other options would have been to implement an el_gets_continue(),
which would have restarted editing with the input of the last
state (but what if that ended in a newline?), or to commit suicide
while trying to deal with signals from within sh(1).

I have also tried to extend editline.3 in respect to
EL_UNBUFFERED, which is only partially documented sofar, and the
yet completely undocumented errno handling.

It is a whole series of local commits indeed, but i don't dare to
attach a MBOX or even (horror) send a patch mail-series, and so
i'll simply attach them in order, including the PR bin/170651
patch (laziness).  It seems to work.  In reversed order:

  - 6.diff: Set EL_READRESTART in interactive sh(1) sessions
  - 5.diff: Add a new EL_READRESTART option for editline(3)
  - 4.diff: Document errno behaviour of el_getc()/el_gets()
  - 3.diff: Document EL_UNBUFFERED for el_set()
  - 2.diff: Fix editline(3) char read and errno code flow
: This simply reuses your patch.
  - 1.diff: Fix PR bin/170651
: (The plain patch.  Maybe possible to leave that off.)

 |-Mark

Bye and ciao,

--steffen
Comment 6 Mark Johnston 2012-09-10 16:58:54 UTC
On Mon, Sep 10, 2012 at 04:56:35PM +0200, Steffen Daode Nurpmeso wrote:
> 
> I have implemented a EL_READRESTART option for editline(3), which
> seems to be the easiest approach to get around this.
> Other options would have been to implement an el_gets_continue(),
> which would have restarted editing with the input of the last
> state (but what if that ended in a newline?), or to commit suicide
> while trying to deal with signals from within sh(1).
> 
> I have also tried to extend editline.3 in respect to
> EL_UNBUFFERED, which is only partially documented sofar, and the
> yet completely undocumented errno handling.
> 
> It is a whole series of local commits indeed, but i don't dare to
> attach a MBOX or even (horror) send a patch mail-series, and so
> i'll simply attach them in order, including the PR bin/170651
> patch (laziness).  It seems to work.  In reversed order:
> 
>   - 6.diff: Set EL_READRESTART in interactive sh(1) sessions
>   - 5.diff: Add a new EL_READRESTART option for editline(3)
>   - 4.diff: Document errno behaviour of el_getc()/el_gets()
>   - 3.diff: Document EL_UNBUFFERED for el_set()
>   - 2.diff: Fix editline(3) char read and errno code flow
> : This simply reuses your patch.
>   - 1.diff: Fix PR bin/170651
> : (The plain patch.  Maybe possible to leave that off.)

I didn't test 1.diff, but the rest of the patches apply and work for me.

Thanks,
-Mark
Comment 7 Steffen "Daode" Nurpmeso 2012-09-10 20:15:04 UTC
Mark Johnston <markjdb@gmail.com> wrote:

 |On Mon, Sep 10, 2012 at 04:56:35PM +0200, Steffen Daode Nurpmeso wrote:
 |> 
 |> I have implemented a EL_READRESTART option for editline(3), which
 [.]
 |>   - 6.diff: Set EL_READRESTART in interactive sh(1) sessions
 |>   - 5.diff: Add a new EL_READRESTART option for editline(3)
 |>   - 4.diff: Document errno behaviour of el_getc()/el_gets()
 |>   - 3.diff: Document EL_UNBUFFERED for el_set()
 |>   - 2.diff: Fix editline(3) char read and errno code flow
 |> : This simply reuses your patch.
 |>   - 1.diff: Fix PR bin/170651
 |> : (The plain patch.  Maybe possible to leave that off.)
 |
 |I didn't test 1.diff, but the rest of the patches apply and work for me.

Good.

 |Thanks,
 |-Mark

My pleasure.

--steffen
Comment 8 Steffen "Daode" Nurpmeso 2012-09-11 17:10:15 UTC
 |Mark Johnston <markjdb@gmail.com> wrote:
 |
 ||On Sat, Sep 08, 2012 at 05:44:56PM +0200, Steffen Daode Nurpmeso wrote:
 ||>  |Synopsis: sh(1): Resizing causes /bin/sh to repeat edit operations
 ||>  |
 ||>  |http://www.freebsd.org/cgi/query-pr.cgi?pr=169773
 ||> 
 | [.]
 ||> It's a rather quick first diff for editline(3), i have no more
 | [.]
 ||
 ||I took a closer look at the patch... I think the errno handling is
 ||mostly ok, except for a couple of places in el_gets() where the return
 ||value of read_char() isn't stored, and the code ends up looking at an
 ||uninitialized variable. The attached patch is your patch + the fix for
 ||this.
 ||
 ||> I *think* it effectively results in editline(3) behaving the way
 ||> it is supposed to work (retrying once after a whatever signal,
 ||> then failing for a second one).  Since el_gets() now fails (as it
 ||> is supposed to), sh(1) will behave wrong in that the current line
 | [.]
 ||> 
 ||> It would be better if editline(3) could be configured to simply
 ||> restart upon EINTR, or to fixate that behaviour (for FreeBSD)?
 ||> I don't think it is acceptable to loose a line of user content due
 ||> to a simple resize?
 ||> So long and ciao,
 ||
 ||Maybe we need a new option for el_set() which sets a flag in
 ||el->el_signal that determines whether various functions return on EINTR.
 ||libfetch for example has fetchRestartCalls for this purpose. It's not
 ||really clear to me why anyone would want EINTR as an error and return
 ||though.
 |
 |I have implemented a EL_READRESTART option for editline(3), which
 [.]
 |
 ||-Mark

I have posted some NetBSD problem reports and some of the patches
have been accepted upstream and are already committed. [1][2][3]
And upstream editline.3 already documented errno behaviour for
el_gets() (not for el_getc(), yet posted a PR for that [4]).

It however turned out that fixing the behaviour is even more
difficult than i thought yesterday, and the issue is also not
completed upstream.  (But at least someone who really knows is
thinking about it from now on.)

So i do attach the current state (less anything which will come in
from upstream anyway), including the new restart patch, for which
i've also posted a PR upstream [5], since i think it is a useful
flag to have.  (Though upstream editline(3) does do some EINTR.
I also saw pfg@'s commit on that itm.)

Anyway - this new restart patch rather applies 1:1 on upstream,
too, and it really seems to fix the problem now.  It is anyway
better than the patch from yesterday, just in case you use that.
Ciao,

--steffen

[1] http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=46935
[2] http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=46941
[3] http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=46942
[4] http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=46945
[5] http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=46943
Comment 9 Jilles Tjoelker freebsd_committer 2013-09-01 17:11:38 UTC
Responsible Changed
From-To: jilles->freebsd-bugs

The underlying problem is in libedit, not in sh. 

A dirty workaround is possible in sh by setting SA_RESTART for SIGWINCH. 
This prevents the repeated operations but does not apply the size change 
until the next line.
Comment 10 Jilles Tjoelker freebsd_committer 2014-01-15 22:31:57 UTC
In PR bin/169773, you wrote:
> Given an account with /bin/sh as the login shell, if you type
> some input, ending with an editing operation (eg backspace) and
> then resize the window multiple times without further input,
> the edit operation will be repeated on every second resize.

I have removed the SIGWINCH handler from sh in r260654, as I don't think
the libedit problem will be fixed properly any time soon. This is not
entirely ideal, but stops most of the erratic behaviour.

-- 
Jilles Tjoelker
Comment 11 Eitan Adler freebsd_committer freebsd_triage 2018-05-20 23:53:50 UTC
For bugs matching the following conditions:
- Status == In Progress
- Assignee == "bugs@FreeBSD.org"
- Last Modified Year <= 2017

Do
- Set Status to "Open"