Re: Unchecked error in _libssh2_channel_write causes infinite loop

On 03/13/2012 11:13 PM, Daniel Stenberg wrote:
> On Mon, 12 Mar 2012, Matthew Booth wrote:
>
>> seems to be down to an unchecked error in _libssh2_channel_write():
>>
>> /* drain the incoming flow first, mostly to make sure we get all
>> * pending window adjust packets */
>> do
>> rc = _libssh2_transport_read(session);
>> while (rc > 0);
>>
>> if(channel->local.window_size <= 0)
>> /* there's no room for data so we stop */
>> return (rc==LIBSSH2_ERROR_EAGAIN?rc:0);
>>
>> In my case, _libssh2_transport_read is returning
>> LIBSSH2_ERROR_SOCKET_RECV (don't know why yet). The result of this in
>> the above code is that _libssh2_channel_write returns 0, throwing away
>> the error information. My code spots a zero return, checks for eof,
>> doesn't find one and goes round again: infinite loop.
>>
>> I would just submit a patch which adds "if (rc<0) return rc;"
>> immediately after the read loop, but it seems like the author has
>> intentionally not done this here. Git commit
>> 3c71ad4fce745876f7e7ad33b846518f2d50edf6 and its associated mailing
>> list thread doesn't shed any light on why only EAGAIN is handled. Can
>> anybody explain?
>
> I think I only added code to handle the EAGAIN case since that was the
> particular case I was out to fix and I played safe and didn't modify the
> other cases.
>
> But I agree that it looks wrong. I would probably suggest this change as
> a slight variation of your suggestion:
>
> diff --git a/src/channel.c b/src/channel.c
> index 8d6fb0a..9e29492 100644
> --- a/src/channel.c
> +++ b/src/channel.c
> @@ -2008,6 +2008,9 @@ _libssh2_channel_write(LIBSSH2_CHANNEL *channel,
> int strea
> rc = _libssh2_transport_read(session);
> while (rc > 0);
>
> + if((rc < 0) && (rc != LIBSSH2_ERROR_EAGAIN))
> + return rc;
> +
> if(channel->local.window_size <= 0)
> /* there's no room for data so we stop */
> return (rc==LIBSSH2_ERROR_EAGAIN?rc:0);
>
> The existing code continues even if LIBSSH2_ERROR_EAGAIN was returned as
> long as there is window size to go with so we should probably continue
> doing so.

I tested the above and it works as expected. Will you push it?

Thanks,

Matt

-- 
Matthew Booth, RHCA, RHCSS
Red Hat Engineering, Virtualisation Team
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
_______________________________________________
libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel