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