On Mon, 2021-08-09 at 14:15 +0200, Martijn van Duren wrote:
> 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@
>
We can make it even smaller by making use of this little known construct
named while.
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:21:55 -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,29 @@ 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);
+ continue;
+ default:
+ break;
+ }
+ } else if (errno == EAGAIN) {
+ if (ioctl(el->el_infd, FIONBIO, &zero) != -1)
+ continue;
}
+ *cp = L'\0';
+ return -1;
}
/* Test for EOF */
