Ingo Schwarze <[email protected]> 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.
