Subject: Re: libssh2 API brokenness

Re: libssh2 API brokenness

From: liuzl <xieepp_at_gmail.com>
Date: Thu, 1 Sep 2011 18:16:09 +0800

This problem has troubled me for a long time, as a programmer,
I decided to find it, I spent a whole day tuning process, I am glad could
help.
I think fixing the problems may take a long time, but the problem is
urgent,what
is about the following patch, it looks work fine.
@@ -1583,11 +1583,10 @@
_libssh2_channel_receive_window_adjust(LIBSSH2_CHANNEL * channel,
         _libssh2_error(channel->session, rc,
                        "Would block sending window adjust");
         return rc;
     }
     else if (rc) {
- channel->adjust_queue = adjustment;
         return _libssh2_error(channel->session, LIBSSH2_ERROR_SOCKET_SEND,
                               "Unable to send transfer-window adjustment "
                               "packet, deferring");
     }
     else {
@@ -1744,10 +1743,17 @@ ssize_t _libssh2_channel_read(LIBSSH2_CHANNEL
*channel, int stream_id,
     int bytes_want;
     int unlink_packet;
     LIBSSH2_PACKET *read_packet;
     LIBSSH2_PACKET *read_next;

+ if(channel->remote.window_size < (LIBSSH2_CHANNEL_WINDOW_DEFAULT*30)) {
+ /* the window is getting too narrow, expand it!
+ Ignore all the sending errors since we are receiving. */
+ _libssh2_channel_receive_window_adjust(channel,
+
(LIBSSH2_CHANNEL_WINDOW_DEFAULT*60), 0, NULL);
+ }
+
     if (channel->read_state == libssh2_NB_state_idle) {
         _libssh2_debug(session, LIBSSH2_TRACE_CONN,
                        "channel_read() wants %d bytes from channel %lu/%lu
"
                        "stream #%d",
                        (int) buflen, channel->local.id, channel->remote.id,
@@ -1763,19 +1769,10 @@ ssize_t _libssh2_channel_read(LIBSSH2_CHANNEL
*channel, int stream_id,
         rc = _libssh2_transport_read(session);

     if ((rc < 0) && (rc != LIBSSH2_ERROR_EAGAIN))
         return _libssh2_error(session, rc, "transport read");

- /*
- * =============================== NOTE ===============================
- * I know this is very ugly and not a really good use of "goto", but
- * this case statement would be even uglier to do it any other way
- */
- if (channel->read_state == libssh2_NB_state_jump1) {
- goto channel_read_ex_point1;
- }
-
     read_packet = _libssh2_list_first(&session->packets);
     while (read_packet && (bytes_read < (int) buflen)) {
         /* previously this loop condition also checked for
            !channel->remote.close but we cannot let it do this:

@@ -1869,30 +1866,10 @@ ssize_t _libssh2_channel_read(LIBSSH2_CHANNEL
*channel, int stream_id,
         /* make sure we remain in the created state to focus on emptying
the
            data we already have in the packet brigade before we try to read
            more off the network again */
         channel->read_state = libssh2_NB_state_created;

- if(channel->remote.window_size < (LIBSSH2_CHANNEL_WINDOW_DEFAULT*30)) {
- /* the window is getting too narrow, expand it! */
-
- channel_read_ex_point1:
- channel->read_state = libssh2_NB_state_jump1;
- /* the actual window adjusting may not finish so we need to deal
with
- this special state here */
- rc = _libssh2_channel_receive_window_adjust(channel,
-
 (LIBSSH2_CHANNEL_WINDOW_DEFAULT*60), 0, NULL);
- if (rc == LIBSSH2_ERROR_EAGAIN)
- return rc;
-
- _libssh2_debug(session, LIBSSH2_TRACE_CONN,
- "channel_read() filled %d adjusted %d",
- bytes_read, buflen);
- /* continue in 'created' state to drain the already read packages
- first before starting to empty the socket further */
- channel->read_state = libssh2_NB_state_created;
- }
-
     return bytes_read;
 }

 /*
  * libssh2_channel_read_ex
@@ -594,11 +594,16 @@ send_existing(LIBSSH2_SESSION *session, const unsigned
char *data,
         *ret = 0;
         return LIBSSH2_ERROR_NONE;
     }

     /* send as much as possible of the existing packet */
- if ((data != p->odata) || (data_len != p->olen)) {
+ /* Very common scene: We may send receive-window-adjust packet at any
time,
+ if we just blocked in the last call, will lead to
LIBSSH2_ERROR_BAD_USE.
+ So, if the caller is an internal function, we try to send normally.
This
+ behavior will be modified in the future. */
+ if (data[0] == SSH_MSG_CHANNEL_DATA && p->odata[0] ==
SSH_MSG_CHANNEL_DATA
+ && ((data != p->odata) || (data_len != p->olen))) {
         /* When we are about to complete the sending of a packet, it is
vital
            that the caller doesn't try to send a new/different packet since
            we don't add this one up until the previous one has been sent.
To
            make the caller really notice his/hers flaw, we return error for
            this case */
@@ -622,10 +627,15 @@ send_existing(LIBSSH2_SESSION *session, const unsigned
char *data,
         debugdump(session, "libssh2_transport_write send()",
                   &p->outbuf[p->osent], rc);
     }

     if (rc == length) {
+ /* The existing data have been sent completely, tell parent to
continue
+ sending the new data. */
+ if ((data != p->odata) || (data_len != p->olen))
+ *ret = 0;
+
         /* the remainder of the package was sent */
         p->ototal_num = 0;
         p->olen = 0;
         /* we leave *ret set so that the parent returns as we MUST return
back
            a send success now, so that we don't risk sending EAGAIN later

_______________________________________________
libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel
Received on 2011-09-01