Re: Unchecked error in _libssh2_channel_write causes infinite loop

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.

-- 
  / daniel.haxx.se
_______________________________________________
libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel