Mailing List Archive

Hi Dmitry,

On 23/4/21 1:06 am, Dmitry Belyavskiy wrote:
> OpenSSH incorrectly restores the standard mode (blocking mode) on standard
> output upon exiting.

I agree that SSH should *restore* the blocking mode.  I've only spent a
small
amount of timelooking at it and I think SSH currently does not do that, and
your patch also does not.

Your patch sets blocking mode if SSH cleared it.  I think it would be more
correct to restore it tothe state that it was when SSH took over the file
because we shouldn't assume.

In channel_register_fds, don't record if we set non-blocking mode, record
what mode it currentlyhas:

c->nonblock = (get_nonblock(rfd) << 2) | (get_nonblock(wfd) << 1) |
get_nonblock(efd);

Restore in channel_close_fds:

channel_close_fd(ssh, &c->sock, 0);
if (rfd != sock) channel_close_fd(ssh, &c->rfd, c->nonblock & 4);
if (wfd != sock) channel_close_fd(ssh, &c->wfd, c->nonblock & 2);
if (efd != sock) channel_close_fd(ssh, &c->efd, c->nonblock & 1);

In channel_close_fd:

if (fd != -1) (nonblock?set_nonblock:unset_nonblock)(fd);

Regards,

David

_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

> I agree that SSH should *restore* the blocking mode.

Is that code path triggered when moving SSH to the background as well
(via EscapeKey), and then set unblock after resuming?
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Dear David,
Thank you for your recommendations!

On Fri, Apr 23, 2021 at 4:47 AM David Newall <openssh@davidnewall.com>
wrote:

> Hi Dmitry,
>
> On 23/4/21 1:06 am, Dmitry Belyavskiy wrote:
> > OpenSSH incorrectly restores the standard mode (blocking mode) on
> standard
> > output upon exiting.
>
> I agree that SSH should *restore* the blocking mode. I've only spent a
> small
> amount of timelooking at it and I think SSH currently does not do that, and
> your patch also does not.
>
> Your patch sets blocking mode if SSH cleared it. I think it would be more
> correct to restore it tothe state that it was when SSH took over the file
> because we shouldn't assume.
>
> In channel_register_fds, don't record if we set non-blocking mode, record
> what mode it currentlyhas:
>
> c->nonblock = (get_nonblock(rfd) << 2) | (get_nonblock(wfd) << 1) |
> get_nonblock(efd);
>
> Restore in channel_close_fds:
>
> channel_close_fd(ssh, &c->sock, 0);
> if (rfd != sock) channel_close_fd(ssh, &c->rfd, c->nonblock & 4);
> if (wfd != sock) channel_close_fd(ssh, &c->wfd, c->nonblock & 2);
> if (efd != sock) channel_close_fd(ssh, &c->efd, c->nonblock & 1);
>
> In channel_close_fd:
>
> if (fd != -1) (nonblock?set_nonblock:unset_nonblock)(fd);
>

Could you please take a look at the updated version of the patch?
If I understand correctly, it provides the same blocking state after
closing the channel as it was before closing.

https://github.com/openssh/openssh-portable/pull/246

Many thanks!

--
Dmitry Belyavskiy
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Hi Dmitry,

On 24/4/21 12:41 am, Dmitry Belyavskiy wrote:
> Could you please take a look at the updated version of the patch?
> If I understand correctly, it provides the same blocking state after
> closing
> the channel as it was before closing.

Yes, generally right, but, I feel you've assumed that the files are
initially
blocking which is not necessarily so.

Also, the file is closed immediately after clearing blocking mode.  I think
it's worth a comment to say that the file being set to blocking mode is an
alias for stdin, out or err.

_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Hi David,


On Sun, Apr 25, 2021 at 2:34 AM David Newall <openssh@davidnewall.com>
wrote:

> Hi Dmitry,
>
> On 24/4/21 12:41 am, Dmitry Belyavskiy wrote:
> > Could you please take a look at the updated version of the patch?
> > If I understand correctly, it provides the same blocking state after
> > closing
> > the channel as it was before closing.
>
> Yes, generally right, but, I feel you've assumed that the files are
> initially
> blocking which is not necessarily so.
>

I think I don't - see the lines 341-354 of the channels.c in the proposed
patch


> Also, the file is closed immediately after clearing blocking mode. I think
> it's worth a comment to say that the file being set to blocking mode is an
> alias for stdin, out or err.
>

Will do, thanks!

--
Dmitry Belyavskiy
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Hi Dmitry,

On 25/4/21 6:23 pm, Dmitry Belyavskiy wrote:
>> Yes, generally right, but, I feel you've assumed that the files are
>> initially
>> blocking which is not necessarily so.
>>
> I think I don't - see the lines 341-354 of the channels.c in the proposed
> patch
I do beg your pardon, you are right.  The patch looks okay to me.
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Hi Philipp,

On 23/4/21 4:03 pm, Philipp Marek wrote:
> Is that code path triggered when moving SSH to the background as well
> (via EscapeKey), and then set unblock after resuming?

I think not.  The change affects non-tty stdin, out and err at
termination of SSH.

Regards,

David

_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Hi David,

On Mon, Apr 26, 2021 at 1:59 AM David Newall <openssh@davidnewall.com>
wrote:

> Hi Dmitry,
> On 25/4/21 6:23 pm, Dmitry Belyavskiy wrote:
>
> Yes, generally right, but, I feel you've assumed that the files are
> initially
> blocking which is not necessarily so.
>
>
> I think I don't - see the lines 341-354 of the channels.c in the proposed
> patch
>
> I do beg your pardon, you are right. The patch looks okay to me.
>

Many thanks for the review!
The clarification comment is pushed.

--
Dmitry Belyavskiy
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev