Re: libedit: stop ignoring SIGINT
On Mon, Aug 09, 2021 at 07:20:31AM -0600, Theo de Raadt wrote: > Ingo Schwarze wrote: > > > as mentioned earlier, deraadt@ reported that sftp(1) ignores Ctrl-C. > > Fixing that without longjmp(3) requires making editline(3) better > > behaved. > > If a library interface encourages use longjmp(), that library should be > thrown into the dustbin of history. Our src tree has very few longjmp > these days. Thank you for the effort to discourage addition of more. > > > 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. > > Looks good. > > > * 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... > > Otto -- if you are short of time, at minimum mark the variable usage > line with /* XXX signal race */ as we have done throughout the tree. I > would encourage anyone who sees such problems inside any signal handler > to show such comment-adding diffs. If these problems are documented, > they can be fixed eventually, usually through event-loop logic, but I'll > admit many of the comments are in non-event-loop programs. Added the comment. Don't know what I was thinking when I did that change. -Otto > > > * 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. > > Horrible isn't it. > > > * 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). > > I suspect dtucker will want to decide on the interactive behaviour, > but what you describe sounds right. > > > 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. > > I mentioned rarely having seen a good outcome from code mixing any of > the 3: FIONBIO, FIONREAD, and select/poll. Often the non-blocking was > added to select/poll code to hide some sort of bug, or the select/poll > code was added amateurishly to older code without removing the FIONBIO. > There are a few situations you need both approaches mixed, but it isn't > the general case, and thus FIONBIO has a "polled IO" smell for me. >
Re: libedit: stop ignoring SIGINT
On Mon, 2021-08-09 at 15:17 +0200, Ingo Schwarze wrote: > Hi Martijn, > > Martijn van Duren wrote on Mon, Aug 09, 2021 at 02:15:42PM +0200: > > > If we're stripping down fixio to a single function, why not > > inline the whole thing? > > I deliberately tried to: > > 1. Keep patches that change behaviour as small as possible to make > review as simple as possible for people who fear they might be > affected but are not specifically interested in editline(3). > > 2. Make sure that reorg / cleanup patches do not change behaviour. > > > 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. > > Yes, that looks like a race condition bug that so far escaped my > attention. But it seems unrelated to what should happen with signals > in general, so can we keep fixing that bug separate? > > > Finally, I don't understand why we only have a single retry on EAGAIN? > > Not having *any* retries inside read_char() would look like a worthy > long-term goal to me, but i'm not yet completely sure that can be > reached. I would prefer to steer into the direction of fewer magic > retries rather than more of them. Either way, EAGAIN seems unrelated > to SIGINT, so i'd prefer to keep topics separate. > > > 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. > > Actually, if the application sets FIONBIO at all (even once), chances > are quite low in the first place that stuff works as inteded by the > author of the application. So i certainly wouldn't worry about an > application setting FIONBIO in a loop, we have significantly worse > problems than that. But again, that's a separate topic. > > > Diff below does this. (minus 27 LoC) > > I do not in general object to cleaning this code up, and getting rid > of read__fixio() indeed seems to be a long-term goal. But i hope > we will be able to remove all the (mostly broken) functionality > from read__fixio() in a step-by-step manner, and once the function > is empty, it will fade away without having to disturb the code in > read_char(). Either way - separate topic... > > The most important short-term goal seems to fix sftp(1), including > the steps required to get that done in a clean way. > > > The following ports use libedit (there might be a better way of > > finding them, but this works): > > Hmm, thanks, that list feels useful. > > > 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. > > I don't think so in this case. Let's not over-use the feature of > putting stuff in snaps. I think that should be reserved for stuff > that is quite important and somewhat urgent and can't easily be > tested in a less disruptive way. But here, testing a program is > quite feasible once you know which program to test. > > Besides, *if* this patch causes a change in behaviour of a port, > the most likely change is that a program that now ignores SIGINT > exits on SIGINT afterwards. That may be worth investigating and > making a decision on in each individual case, but it's not a > super-critical change in behaviour that might require testing > in snaps. > > Yours, > Ingo > Your reasoning makes sense to me. Assuming you're confident enough with the applications linked to libedit: OK martijn@
Re: libedit: stop ignoring SIGINT
Ingo Schwarze wrote: > > 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. > > I don't think so in this case. Let's not over-use the feature of > putting stuff in snaps. I think that should be reserved for stuff > that is quite important and somewhat urgent and can't easily be > tested in a less disruptive way. But here, testing a program is > quite feasible once you know which program to test. We know what needs testing, because it links against the library. Once the first set of base programs is tested to work well with the change, it is quite likely we have complete confidence it only improves the situation in ports. Snapshot testing is reserved for discovering unknown breakage, quickly. Like "we don't know how or which programs abuse something" or "what weird machines do people have" or "we don't know how our user's fingers work". It's not like our users are going to quickly discover a weird behaviour from ^C, considering I only noticed this decade-old problem in sftp a few days ago.
Re: libedit: stop ignoring SIGINT
Ingo Schwarze wrote: > as mentioned earlier, deraadt@ reported that sftp(1) ignores Ctrl-C. > Fixing that without longjmp(3) requires making editline(3) better > behaved. If a library interface encourages use longjmp(), that library should be thrown into the dustbin of history. Our src tree has very few longjmp these days. Thank you for the effort to discourage addition of more. > 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. Looks good. > * 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... Otto -- if you are short of time, at minimum mark the variable usage line with /* XXX signal race */ as we have done throughout the tree. I would encourage anyone who sees such problems inside any signal handler to show such comment-adding diffs. If these problems are documented, they can be fixed eventually, usually through event-loop logic, but I'll admit many of the comments are in non-event-loop programs. > * 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. Horrible isn't it. > * 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). I suspect dtucker will want to decide on the interactive behaviour, but what you describe sounds right. > 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. I mentioned rarely having seen a good outcome from code mixing any of the 3: FIONBIO, FIONREAD, and select/poll. Often the non-blocking was added to select/poll code to hide some sort of bug, or the select/poll code was added amateurishly to older code without removing the FIONBIO. There are a few situations you need both approaches mixed, but it isn't the general case, and thus FIONBIO has a "polled IO" smell for me.
Re: libedit: stop ignoring SIGINT
Hi Martijn, Martijn van Duren wrote on Mon, Aug 09, 2021 at 02:15:42PM +0200: > If we're stripping down fixio to a single function, why not > inline the whole thing? I deliberately tried to: 1. Keep patches that change behaviour as small as possible to make review as simple as possible for people who fear they might be affected but are not specifically interested in editline(3). 2. Make sure that reorg / cleanup patches do not change behaviour. > 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. Yes, that looks like a race condition bug that so far escaped my attention. But it seems unrelated to what should happen with signals in general, so can we keep fixing that bug separate? > Finally, I don't understand why we only have a single retry on EAGAIN? Not having *any* retries inside read_char() would look like a worthy long-term goal to me, but i'm not yet completely sure that can be reached. I would prefer to steer into the direction of fewer magic retries rather than more of them. Either way, EAGAIN seems unrelated to SIGINT, so i'd prefer to keep topics separate. > 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. Actually, if the application sets FIONBIO at all (even once), chances are quite low in the first place that stuff works as inteded by the author of the application. So i certainly wouldn't worry about an application setting FIONBIO in a loop, we have significantly worse problems than that. But again, that's a separate topic. > Diff below does this. (minus 27 LoC) I do not in general object to cleaning this code up, and getting rid of read__fixio() indeed seems to be a long-term goal. But i hope we will be able to remove all the (mostly broken) functionality from read__fixio() in a step-by-step manner, and once the function is empty, it will fade away without having to disturb the code in read_char(). Either way - separate topic... The most important short-term goal seems to fix sftp(1), including the steps required to get that done in a clean way. > The following ports use libedit (there might be a better way of > finding them, but this works): Hmm, thanks, that list feels useful. > 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. I don't think so in this case. Let's not over-use the feature of putting stuff in snaps. I think that should be reserved for stuff that is quite important and somewhat urgent and can't easily be tested in a less disruptive way. But here, testing a program is quite feasible once you know which program to test. Besides, *if* this patch causes a change in behaviour of a port, the most likely change is that a program that now ignores SIGINT exits on SIGINT afterwards. That may be worth investigating and making a decision on in each individual case, but it's not a super-critical change in behaviour that might require testing in snaps. Yours, Ingo
Re: libedit: stop ignoring SIGINT
On Mon, Aug 09, 2021 at 01:19:08PM +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? I had a quick test with bgplgsh(8), hitting Ctrl-C exits bgplgsh. Same before with or without the diff. Not sure if anyone is actually using bgplgsh(8) -- I never used it. -- :wq Claudio
Re: libedit: stop ignoring SIGINT
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 ' | \ > > cut -d: -f1 | while read file; do \ > > (cd $(dirname $file); make show=WANTLIB | \ > > grep -q '\' && 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 >
Re: libedit: stop ignoring SIGINT
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 ' | \ > cut -d: -f1 | while read file; do \ > (cd $(dirname $file); make show=WANTLIB | \ > grep -q '\' && 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:
libedit: stop ignoring SIGINT
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.
Index: read.c
===
RCS file: /cvs/src/lib/libedit/read.c,v
retrieving revision 1.45
diff -u -U15 -r1.45 read.c
--- read.c 9 Aug 2021 09:11:26 - 1.45
+++ read.c 9 Aug 2021 10:00:18 -
@@ -134,22 +134,19 @@
/* 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, ) == -1)
return -1;
return 0;
- case EINTR:
- return 0;
-
default:
return -1;
}
}
