On Mon, Aug 09, 2021 at 07:20:31AM -0600, Theo de Raadt wrote: > Ingo Schwarze <schwa...@usta.de> 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. >