On Mon, 2021-08-09 at 13:19 +0200, Ingo Schwarze wrote:
> Hi,
>
> as mentioned earlier, deraadt@ reported that sftp(1) ignores Ctrl-C.
> Fixing that without longjmp(3) requires making editline(3) better
> behaved.
>
> Currently, when read(2) from the terminal gets interrupted by a
> signal, editline(3) ignores the (first) signal and unconditionally
> calls read(2) a second time. That seems wrong: if the user hits
> Ctrl-C, it is sane to assume that they meant it, not that they
> want it ignored.
>
> The following patch causes el_gets(3) and el_wgets(3) to return
> failure when read(2)ing from the terminal is interrupted by a
> signal other than SIGCONT or SIGWINCH. That allows the calling
> program to decide what to do, usually either exit the program or
> provide a fresh prompt to the user.
>
> If i know how to grep(1), the following programs in the base system
> use -ledit:
>
> * bc(1)
> It behaves well with the patch: Ctrl-C discards the current
> input line and starts a new input line.
> The reason why this already works even without the patch
> is that bc(1) does very scary stuff inside the signal handler:
> pass a file-global EditLine pointer on the heap to el_line(3)
> and access fields inside the returned struct. Needless to
> say that no signal handler should do such things...
>
> * cdio(1)
> Behaviour is acceptable and unchanged with the patch:
> Ctrl-C exits cdio(1).
>
> * ftp(1)
> It behaves well with the patch: Ctrl-C discards the current
> input line and provides a fresh prompt.
> The reason why it already works without the patch is that ftp(1)
> uses setjmp(3)/longjmp(3) to forcefully grab back control
> from el_gets(3) without waiting for it to return.
>
> * lldb(1)
> It misbehaves with or without the patch and ignores Ctrl-C.
> I freely admit that i don't feel too enthusiastic about
> debugging that beast.
>
> * sftp(1)
> Behaviour is improved with the patch: Ctrl-C now exits sftp(1).
> If desired, i can supply a very simple follow-up patch to sftp.c
> to instead behave like ftp(1) and bc(1), i.e. discard the
> current input line and provide a fresh prompt.
> Neither doing undue work in the signal handler nor longjmp(3)
> will be required for that (if this patch gets committed).
>
> * bgplgsh(8), fsdb(8)
> I have no idea how to test those. Does anyone think that testing
> either of them would be required?
>
> Regarding the patch below, note that differentiating EINTR behaviour
> by signal number is not needed because the calling code in read_char()
> already handles SIGCONT and SIGWINCH, so those two never arrive in
> read__fixio() in the first place.
>
> Also note that deraadt@ pointed out in private mail to me that the
> fact that read__fixio() clears FIONBIO is probably a symptom of
> botched editline(3) API design. That might be worth fixing, too,
> as far as that is feasible, but it is unrelated to the sftp(1)
> Ctrl-C issue; let's address one topic at a time.
>
> Any feedback is welcome.
>
> Yours,
> Ingo
>
> P.S.
> I decided to Cc: tech@ again because this patch might affect
> even people who are not specifically interested in editline(3),
> and i have no intention to cause unpleasant surprises.
>
If we're stripping down fixio to a single function, why not inline the
whole thing?
Also, as far as my brain explains things to me, it could theoretically
be possible that the signal handler kicks in right after we return a -1
from read(2), making it possible to get something like an EIO and
entering the sig switch statement. Assuming that a signal handler
doesn't clobber our errno (which libedit doesn't do, but who knows what
bad code is out there) we should probably only check the signal type
when we know we have an EINTR.
Finally, I don't understand why we only have a single retry on EAGAIN?
If the application keeps resetting the FIONBIO in such a furious way
that it happens more then once in a single call (no idea how that could
happen) it might be an uphill battle, but we just burn away cpu cycles.
It is not and should probably not be treated like something fatal.
Diff below does this. (minus 27 LoC)
The following ports use libedit (there might be a better way of finding
them, but this works):
$ find . -name Makefile | xargs grep -F '.include <bsd.port.mk>' | \
> cut -d: -f1 | while read file; do \
> (cd $(dirname $file); make show=WANTLIB | \
> grep -q '\<edit\>' && echo $(dirname $file)); \>
> done 2> /dev/null
./devel/clang-tools-extra
./devel/llvm
./mail/dcc
./mail/nmh
./emulators/gsplus
./emulators/nono
./lang/brainfuck
./lang/eltclsh
./lang/lua/5.1
./lang/lua/5.2
./lang/lua/5.3
./lang/swi-prolog
./math/gbc
./net/dnsdist
./net/honeyd
./net/icinga/core2
./net/knot
./net/ntp
./security/kc
./security/pivy
./shells/dash
./shells/nsh
./sysutils/ipmitool
So if we decide which of our interpretations should take precedence it
might be a good idea to put it into snaps for a while.
martijn@
Index: read.c
===================================================================
RCS file: /cvs/src/lib/libedit/read.c,v
retrieving revision 1.45
diff -u -p -r1.45 read.c
--- read.c 9 Aug 2021 09:11:26 -0000 1.45
+++ read.c 9 Aug 2021 12:10:42 -0000
@@ -66,7 +66,6 @@ struct el_read_t {
int read_errno;
};
-static int read__fixio(int, int);
static int read_char(EditLine *, wchar_t *);
static int read_getcmd(EditLine *, el_action_t *, wchar_t *);
static void read_clearmacros(struct macros *);
@@ -132,29 +131,6 @@ el_read_getfn(struct el_read_t *el_read)
}
-/* read__fixio():
- * Try to recover from a read error
- */
-static int
-read__fixio(int fd, int e)
-{
- int zero = 0;
-
- switch (e) {
- case EAGAIN:
- if (ioctl(fd, FIONBIO, &zero) == -1)
- return -1;
- return 0;
-
- case EINTR:
- return 0;
-
- default:
- return -1;
- }
-}
-
-
/* el_push():
* Push a macro
*/
@@ -234,33 +210,30 @@ static int
read_char(EditLine *el, wchar_t *cp)
{
ssize_t num_read;
- int tried = 0;
char cbuf[MB_LEN_MAX];
int cbp = 0;
- int save_errno = errno;
+ int zero = 0;
again:
el->el_signal->sig_no = 0;
while ((num_read = read(el->el_infd, cbuf + cbp, 1)) == -1) {
- int e = errno;
- switch (el->el_signal->sig_no) {
- case SIGCONT:
- el_set(el, EL_REFRESH);
- /*FALLTHROUGH*/
- case SIGWINCH:
- sig_set(el);
- goto again;
- default:
- break;
- }
- if (!tried && read__fixio(el->el_infd, e) == 0) {
- errno = save_errno;
- tried = 1;
- } else {
- errno = e;
- *cp = L'\0';
- return -1;
+ if (errno == EINTR) {
+ switch (el->el_signal->sig_no) {
+ case SIGCONT:
+ el_set(el, EL_REFRESH);
+ /*FALLTHROUGH*/
+ case SIGWINCH:
+ sig_set(el);
+ goto again;
+ default:
+ break;
+ }
+ } else if (errno == EAGAIN) {
+ if (ioctl(el->el_infd, FIONBIO, &zero) != -1)
+ goto again;
}
+ *cp = L'\0';
+ return -1;
}
/* Test for EOF */
