See the commit messages on the attached patches for the details.
In summary:
1) _libssh2_channel_read() could drop data under some rare circusnstances.
2) zlib inflate() usage was wrong resulting in corruption on the channel
stream. This bug only manifested when transferring highly compressible data.
My previous patch "Fix flow control" (also attached) has to be applied
before the other two.
>From f1e4bf04a05fca8f0b5f7bcbf293b9908cd74404 Mon Sep 17 00:00:00 2001
From: Salvador Fandino <sfand...@yahoo.com>
Date: Sat, 12 Oct 2013 02:51:46 +0200
Subject: [PATCH 1/3] Fix flow control
Until now, the window size (channel->remote.window_size) was being updated
just after receiving the packet from the transport layer.
That behaviour is wrong because the channel queue may grow uncontrolled
when data arrives from the network faster that the upper layer consumes it.
This patch adds a new counter, read_avail, which keeps a count of the bytes
available from the packet queue for reading. Also, now the window size is
adjusted when the data is actually read by an upper layer.
That way, if the upper layer stops reading data, the window will eventually
fill and the remote host will stop sending data. When the upper layers reads
enough data, a window adjust packet is delivered and the transfer resumes.
The read_avail counter is used to detect the situation when the remote
server tries to send data surpassing the window size. In that case, the
extra data is discarded.
Signed-off-by: Salvador <sfand...@yahoo.com>
---
src/channel.c | 8 +++++++-
src/libssh2_priv.h | 2 ++
src/packet.c | 33 +++++++++++++++++++++++++--------
3 files changed, 34 insertions(+), 9 deletions(-)
diff --git a/src/channel.c b/src/channel.c
index 128a04e..68b1857 100644
--- a/src/channel.c
+++ b/src/channel.c
@@ -1414,6 +1414,9 @@ _libssh2_channel_flush(LIBSSH2_CHANNEL *channel, int streamid)
channel->flush_state = libssh2_NB_state_created;
}
+ channel->read_avail -= channel->flush_flush_bytes;
+ channel->remote.window_size -= channel->flush_flush_bytes;
+
if (channel->flush_refund_bytes) {
int rc;
@@ -1871,11 +1874,14 @@ ssize_t _libssh2_channel_read(LIBSSH2_CHANNEL *channel, int stream_id,
/* if the transport layer said EAGAIN then we say so as well */
return _libssh2_error(session, rc, "would block");
}
- else
+ else {
+ channel->read_avail -= bytes_read;
+ channel->remote.window_size -= bytes_read;
/* 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! */
diff --git a/src/libssh2_priv.h b/src/libssh2_priv.h
index 05b1ffc..461d14c 100644
--- a/src/libssh2_priv.h
+++ b/src/libssh2_priv.h
@@ -356,6 +356,8 @@ struct _LIBSSH2_CHANNEL
libssh2_channel_data local, remote;
/* Amount of bytes to be refunded to receive window (but not yet sent) */
uint32_t adjust_queue;
+ /* Data immediately available for reading */
+ uint32_t read_avail;
LIBSSH2_SESSION *session;
diff --git a/src/packet.c b/src/packet.c
index a4887c8..7887e61 100644
--- a/src/packet.c
+++ b/src/packet.c
@@ -654,8 +654,18 @@ _libssh2_packet_add(LIBSSH2_SESSION * session, unsigned char *data,
_libssh2_debug(session, LIBSSH2_TRACE_CONN,
"Ignoring extended data and refunding %d bytes",
(int) (datalen - 13));
- session->packAdd_channelp = channelp;
+ if (channelp->read_avail + datalen - data_head >= channelp->remote.window_size)
+ datalen = channelp->remote.window_size - channelp->read_avail + data_head;
+ channelp->remote.window_size -= datalen - data_head;
+ _libssh2_debug(session, LIBSSH2_TRACE_CONN,
+ "shrinking window size by %lu bytes to %lu, read_avail %lu",
+ datalen - data_head,
+ channelp->remote.window_size,
+ channelp->read_avail);
+
+ session->packAdd_channelp = channelp;
+
/* Adjust the window based on the block we just freed */
libssh2_packet_add_jump_point1:
session->packAdd_state = libssh2_NB_state_jump1;
@@ -685,7 +695,7 @@ _libssh2_packet_add(LIBSSH2_SESSION * session, unsigned char *data,
" to receive, truncating");
datalen = channelp->remote.packet_size + data_head;
}
- if (channelp->remote.window_size <= 0) {
+ if (channelp->remote.window_size <= channelp->read_avail) {
/*
* Spec says we MAY ignore bytes sent beyond
* window_size
@@ -701,17 +711,24 @@ _libssh2_packet_add(LIBSSH2_SESSION * session, unsigned char *data,
/* Reset EOF status */
channelp->remote.eof = 0;
- if ((datalen - data_head) > channelp->remote.window_size) {
+ if (channelp->read_avail + datalen - data_head > channelp->remote.window_size) {
_libssh2_error(session,
LIBSSH2_ERROR_CHANNEL_WINDOW_EXCEEDED,
"Remote sent more data than current "
"window allows, truncating");
- datalen = channelp->remote.window_size + data_head;
- channelp->remote.window_size = 0;
+ datalen = channelp->remote.window_size - channelp->read_avail + data_head;
}
- else
- /* Now that we've received it, shrink our window */
- channelp->remote.window_size -= datalen - data_head;
+
+ /* Update the read_avail counter. The window size will be
+ * updated once the data is actually read from the queue
+ * from an upper layer */
+ channelp->read_avail += datalen - data_head;
+
+ _libssh2_debug(session, LIBSSH2_TRACE_CONN,
+ "increasing read_avail by %lu bytes to %lu/%lu",
+ (long)(datalen - data_head),
+ (long)channelp->read_avail,
+ (long)channelp->remote.window_size);
break;
--
1.8.3.2
>From 259723476e8e7e31af7ac007ca7a8bf250e76419 Mon Sep 17 00:00:00 2001
From: Salvador <sfand...@yahoo.com>
Date: Tue, 15 Oct 2013 11:45:10 +0200
Subject: [PATCH 2/3] _libssh2_channel_read was dropping data
After filling the read buffer with data from the read queue, when the
window size was too small, "libssh2_channel_receive_window_adjust" was
called to increase it. In non-blocking mode that function could return
EAGAIN and, in that case, the EAGAIN was propagated upwards and the data
already read on the buffer lost.
The function was also moving between the two read states
"libssh2_NB_state_idle" and "libssh2_NB_state_created" both of which
behave in the same way (excepting a debug statment).
This patch modifies "_libssh2_channel_read" so that the
"libssh2_channel_receive_window_adjust" call is performed first (when
required) and if everything goes well, then it reads the data from
the queued packets into the read buffer.
It also removes the useless "libssh2_NB_state_created" read state.
Some rotted comments have also been updated.
Signed-off-by: Salvador <sfand...@yahoo.com>
---
src/channel.c | 76 ++++++++++++++++++++---------------------------------------
1 file changed, 25 insertions(+), 51 deletions(-)
diff --git a/src/channel.c b/src/channel.c
index 68b1857..9df2f8d 100644
--- a/src/channel.c
+++ b/src/channel.c
@@ -1754,31 +1754,33 @@ ssize_t _libssh2_channel_read(LIBSSH2_CHANNEL *channel, int stream_id,
LIBSSH2_PACKET *read_packet;
LIBSSH2_PACKET *read_next;
- 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,
- stream_id);
- channel->read_state = libssh2_NB_state_created;
- }
+ _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,
+ stream_id);
- /*
- * =============================== 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_window_adjust;
- }
+ /* expand the receiving window first if it has become too narrow */
+ if((channel->read_state == libssh2_NB_state_jump1) ||
+ (channel->remote.window_size < (LIBSSH2_CHANNEL_WINDOW_DEFAULT*30))) {
+
+ /* the actual window adjusting may not finish so we need to deal with
+ this special state here */
+ channel->read_state = libssh2_NB_state_jump1;
+ rc = _libssh2_channel_receive_window_adjust(channel,
+ (LIBSSH2_CHANNEL_WINDOW_DEFAULT*60),
+ 0, NULL);
+ if (rc)
+ return rc;
- rc = 1; /* set to >0 to let the while loop start */
+ channel->read_state = libssh2_NB_state_idle;
+ }
- /* Process all pending incoming packets in all states in order to "even
- out" the network readings. Tests prove that this way produces faster
- transfers. */
- while (rc > 0)
+ /* Process all pending incoming packets. Tests prove that this way
+ produces faster transfers. */
+ do {
rc = _libssh2_transport_read(session);
+ } while (rc > 0);
if ((rc < 0) && (rc != LIBSSH2_ERROR_EAGAIN))
return _libssh2_error(session, rc, "transport read");
@@ -1860,8 +1862,6 @@ ssize_t _libssh2_channel_read(LIBSSH2_CHANNEL *channel, int stream_id,
}
if (!bytes_read) {
- channel->read_state = libssh2_NB_state_idle;
-
/* If the channel is already at EOF or even closed, we need to signal
that back. We may have gotten that info while draining the incoming
transport layer until EAGAIN so we must not be fooled by that
@@ -1874,35 +1874,9 @@ ssize_t _libssh2_channel_read(LIBSSH2_CHANNEL *channel, int stream_id,
/* if the transport layer said EAGAIN then we say so as well */
return _libssh2_error(session, rc, "would block");
}
- else {
- channel->read_avail -= bytes_read;
- channel->remote.window_size -= bytes_read;
- /* 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_window_adjust:
- 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)
- 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;
- }
+ channel->read_avail -= bytes_read;
+ channel->remote.window_size -= bytes_read;
return bytes_read;
}
--
1.8.3.2
>From 7e2146dc962948eb01fc335dfdc6213b9d0cfa0a Mon Sep 17 00:00:00 2001
From: Salvador <sfand...@yahoo.com>
Date: Tue, 15 Oct 2013 11:00:52 +0200
Subject: [PATCH 3/3] Fix zlib usage
Data may remain in zlib internal buffers when inflate() returns Z_OK
and avail_out == 0. In that case, inflate has to be called again.
Also, once all the data has been inflated, it returns Z_BUF_ERROR to
signal that the input buffer has been exhausted.
Until now, the way to detect that a packet payload had been completely
decompressed was to check that no data remained on the input buffer
but that didn't account for the case where data remained on the internal
zlib buffers.
That resulted in packets not being completely decompressed and the
missing data reappearing on the next packet, though the bug was masked
by the buffer allocation algorithm most of the time and only manifested
when transferring highly compressible data.
This patch fixes the zlib usage.
Signed-off-by: Salvador <sfand...@yahoo.com>
---
src/comp.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/comp.c b/src/comp.c
index 4593ce4..c9226ed 100644
--- a/src/comp.c
+++ b/src/comp.c
@@ -259,12 +259,12 @@ comp_method_zlib_decomp(LIBSSH2_SESSION * session,
status = inflate(strm, Z_PARTIAL_FLUSH);
if (status == Z_OK) {
- if (! strm->avail_in) {
- /* status is OK and input all used so we're done */
+ if (strm->avail_out > 0)
+ /* status is OK and the output buffer has not been exhausted so we're done */
break;
- }
} else if (status == Z_BUF_ERROR) {
- /* This is OK, just drop through to grow the buffer */
+ /* the input data has been exhausted so we are done */
+ break;
} else {
/* error state */
LIBSSH2_FREE(session, out);
--
1.8.3.2
_______________________________________________
libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel
