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.
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
Responsible Changed From-To: freebsd-bugs->jilles over to maintainer
----- 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 -----
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
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
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
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
|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
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.
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
For bugs matching the following conditions: - Status == In Progress - Assignee == "bugs@FreeBSD.org" - Last Modified Year <= 2017 Do - Set Status to "Open"
I cannot reproduce this bug anymore, I believe either jilles@ "fix" was enough, or libedit fixed things in the meantime. Please reopen if I am wrong