On Sun, 2021-08-08 at 13:42 +0200, Ingo Schwarze wrote: > Hi, > > deraadt@ recently reported to me that the editline(3) library, while > line editing is active - for example inside el_gets(3) - ignores > the first SIGINT it receives, for example the the first Ctrl-C the > user presses. I consider that a bug in the editline(3) library. > Some programs, for example our old ftp(1) implementation, work > around the bug in a horrible way by using setjmp(3)/longjmp(3). > > The root cause of the problem is in libedit/read.c, in the interaction > of the functions read_char() and read__fixio(). Before fixing the bug > can reasonably be considered, the function read__fixio() direly needs > cleanup. As it stands, it is utterly unreadable. > > So here is a patch to make it clear what the function does, with > no functional change intended yet (-37 +5 LOC). > > There will be one or more follow-up patches. If you want to receive > them, please reply to me, i won't necessarily send them all to > tech@. > > I see some value in avoiding gratuitious divergence from NetBSD, > but getting rid of this horrible mess is not gratuitious. > > Rationale: > * Do not mark an argument as unused that is actually used. > * errno cannot possibly be -1. Even is it were, treating it as > EAGAIN makes no sense, treating it as the most severe error > imaginable makes more sense to me. > * We define EWOULDBLOCK to be the same as EAGAIN, so no need > to handle it separately. > * No need to #define TRY_AGAIN to use it just once. > * Don't do the same thing twice. We do support the FIONBIO ioctl(2), > so the the indirection using the F_GETFL fcntl(2) can be deleted. > * No need to play confusing games with the "e" variable. > Just return -1 for error or 0 for success in a straightforward > manner. > > OK? > Ingo > > P.S. > I also considered whether this FIONBIO dance should better be done > at the initialization stage rather than after EAGAIN already happened. > But maybe not. This is a library. The application program might > set the fd to non-blocking mode at any time and then call el_gets(3) > again, in which case the library needs to restore blocking I/O to > do its work. > Hopefully without getting dragged too much into this one: Your reasoning and diff look fine to me.
OK martijn@ > > Index: read.c > =================================================================== > RCS file: /cvs/src/lib/libedit/read.c,v > retrieving revision 1.44 > diff -u -p -r1.44 read.c > --- read.c 25 May 2016 09:36:21 -0000 1.44 > +++ read.c 8 Aug 2021 10:30:06 -0000 > @@ -39,9 +39,10 @@ > * read.c: Clean this junk up! This is horrible code. > * Terminal read functions > */ > +#include <sys/ioctl.h> > + > #include <ctype.h> > #include <errno.h> > -#include <fcntl.h> > #include <limits.h> > #include <stdlib.h> > #include <string.h> > @@ -134,55 +135,16 @@ el_read_getfn(struct el_read_t *el_read) > /* read__fixio(): > * Try to recover from a read error > */ > -/* ARGSUSED */ > static int > -read__fixio(int fd __attribute__((__unused__)), int e) > +read__fixio(int fd, int e) > { > + int zero = 0; > > switch (e) { > - case -1: /* Make sure that the code is reachable */ > - > -#ifdef EWOULDBLOCK > - case EWOULDBLOCK: > -#ifndef TRY_AGAIN > -#define TRY_AGAIN > -#endif > -#endif /* EWOULDBLOCK */ > - > -#if defined(POSIX) && defined(EAGAIN) > -#if defined(EWOULDBLOCK) && EWOULDBLOCK != EAGAIN > case EAGAIN: > -#ifndef TRY_AGAIN > -#define TRY_AGAIN > -#endif > -#endif /* EWOULDBLOCK && EWOULDBLOCK != EAGAIN */ > -#endif /* POSIX && EAGAIN */ > - > - e = 0; > -#ifdef TRY_AGAIN > -#if defined(F_SETFL) && defined(O_NDELAY) > - if ((e = fcntl(fd, F_GETFL)) == -1) > + if (ioctl(fd, FIONBIO, &zero) == -1) > return -1; > - > - if (fcntl(fd, F_SETFL, e & ~O_NDELAY) == -1) > - return -1; > - else > - e = 1; > -#endif /* F_SETFL && O_NDELAY */ > - > -#ifdef FIONBIO > - { > - int zero = 0; > - > - if (ioctl(fd, FIONBIO, &zero) == -1) > - return -1; > - else > - e = 1; > - } > -#endif /* FIONBIO */ > - > -#endif /* TRY_AGAIN */ > - return e ? 0 : -1; > + return 0; > > case EINTR: > return 0; >