Fix curl's excessive memory consumption during scp download

This commit is contained in:
Kamil Dudka 2014-03-28 11:56:56 +01:00
parent 343e650ca6
commit d73b199906
7 changed files with 562 additions and 1 deletions

View File

@ -0,0 +1,45 @@
From d0a263ef805244245afd9b709bdd3dc733113a6c Mon Sep 17 00:00:00 2001
From: Daniel Stenberg <daniel@haxx.se>
Date: Sat, 7 Sep 2013 13:41:14 +0200
Subject: [PATCH 06/11] _libssh2_channel_write: client spins on write when window full
When there's no window to "write to", there's no point in waiting for
the socket to become writable since it most likely just will continue to
be.
Patch-by: ncm
Fixes #258
[upstream commit e6c46cc249227de7b7cd136d72eded5dcb3f9381]
Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
src/channel.c | 10 +++++++++-
1 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/src/channel.c b/src/channel.c
index 9f2c241..74262d8 100644
--- a/src/channel.c
+++ b/src/channel.c
@@ -2039,9 +2039,17 @@ _libssh2_channel_write(LIBSSH2_CHANNEL *channel, int stream_id,
if((rc < 0) && (rc != LIBSSH2_ERROR_EAGAIN))
return rc;
- if(channel->local.window_size <= 0)
+ if(channel->local.window_size <= 0) {
/* there's no room for data so we stop */
+
+ /* Waiting on the socket to be writable would be wrong because we
+ * would be back here immediately, but a readable socket might
+ * herald an incoming window adjustment.
+ */
+ session->socket_block_directions = LIBSSH2_SESSION_BLOCK_INBOUND;
+
return (rc==LIBSSH2_ERROR_EAGAIN?rc:0);
+ }
channel->write_bufwrite = buflen;
--
1.7.1

View File

@ -0,0 +1,146 @@
From 8af6637d86b6a85e8889c286f7ff3d841fc5621c Mon Sep 17 00:00:00 2001
From: Salvador Fandino <sfandino@yahoo.com>
Date: Sat, 12 Oct 2013 02:51:46 +0200
Subject: [PATCH 07/11] window_size: redid window handling for flow control reasons
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 <sfandino@yahoo.com>
[upstream commit cdeef54967ed5b7d5bd8fa6da5851aa3d173faa0]
Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
src/channel.c | 8 +++++++-
src/libssh2_priv.h | 2 ++
src/packet.c | 35 ++++++++++++++++++++++++++++-------
3 files changed, 37 insertions(+), 8 deletions(-)
diff --git a/src/channel.c b/src/channel.c
index 74262d8..499d815 100644
--- a/src/channel.c
+++ b/src/channel.c
@@ -1411,6 +1411,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;
@@ -1868,11 +1871,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 4ec9f73..fcf4370 100644
--- a/src/libssh2_priv.h
+++ b/src/libssh2_priv.h
@@ -357,6 +357,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 bfbd56a..d2e758c 100644
--- a/src/packet.c
+++ b/src/packet.c
@@ -653,6 +653,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));
+ 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 */
@@ -684,7 +696,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
@@ -700,17 +712,26 @@ _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.7.1

View File

@ -0,0 +1,140 @@
From cae2385ba898f71038ed4dd00ddae02f85e588e7 Mon Sep 17 00:00:00 2001
From: Salvador <sfandino@yahoo.com>
Date: Tue, 15 Oct 2013 11:45:10 +0200
Subject: [PATCH 08/11] _libssh2_channel_read: fix data drop when out of window
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 commit 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 <sfandino@yahoo.com>
[upstream commit 27f9ac2549b7721cf9d857022c0e7a311830b367]
Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
src/channel.c | 75 +++++++++++++++++++--------------------------------------
1 files changed, 25 insertions(+), 50 deletions(-)
diff --git a/src/channel.c b/src/channel.c
index 499d815..82f6980 100644
--- a/src/channel.c
+++ b/src/channel.c
@@ -1751,31 +1751,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");
@@ -1857,8 +1859,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
@@ -1871,34 +1871,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.7.1

View File

@ -0,0 +1,62 @@
From 5c14f0e6ecfe73da86d3ad20edd60c4756037935 Mon Sep 17 00:00:00 2001
From: Salvador <sfandino-/E1597aS9LQAvxtiuMwx3w@public.gmane.org>
Date: Wed, 16 Oct 2013 13:31:31 +0200
Subject: [PATCH 09/11] _libssh2_channel_read: Honour window_size_initial
_libssh2_channel_read was using an arbitrary hard-coded limit to trigger
the window adjusting code. The adjustment used was also hard-coded and
arbitrary, 15MB actually, which would limit the usability of libssh2 on
systems with little RAM.
This patch, uses the window_size parameter passed to
libssh2_channel_open_ex (stored as remote.window_size_initial) plus the
buflen as the base for the trigger and the adjustment calculation.
The memory usage when using the default window size is reduced from 22MB
to 256KB per channel (actually, if compression is used, these numbers
should be incremented by ~50% to account for the errors between the
decompressed packet sizes and the predicted sizes).
My tests indicate that this change does not impact the performance of
transfers across localhost or a LAN, being it on par with that of
OpenSSH. On the other hand, it will probably slow down transfers on
networks with high bandwidth*delay when the default window size
(LIBSSH2_CHANNEL_WINDOW_DEFAULT=256KB) is used.
Signed-off-by: Salvador Fandino <sfandino@yahoo.com>
[upstream commit 1b3307dda0c58d9023a657747592ac86703b1ff4]
Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
src/channel.c | 11 +++++++----
1 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/channel.c b/src/channel.c
index 82f6980..36c75d2 100644
--- a/src/channel.c
+++ b/src/channel.c
@@ -1758,14 +1758,17 @@ ssize_t _libssh2_channel_read(LIBSSH2_CHANNEL *channel, int stream_id,
stream_id);
/* 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))) {
+ if( (channel->read_state == libssh2_NB_state_jump1) ||
+ (channel->remote.window_size < channel->remote.window_size_initial / 4 * 3 + buflen) ) {
+
+ uint32_t adjustment = channel->remote.window_size_initial + buflen - channel->remote.window_size;
+ if (adjustment < LIBSSH2_CHANNEL_MINADJUST)
+ adjustment = LIBSSH2_CHANNEL_MINADJUST;
/* 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),
+ rc = _libssh2_channel_receive_window_adjust(channel, adjustment,
0, NULL);
if (rc)
return rc;
--
1.7.1

View File

@ -0,0 +1,85 @@
From 0a758095c40ae1b32dc5052a706a16c2d9ac5742 Mon Sep 17 00:00:00 2001
From: Salvador Fandino <sfandino@yahoo.com>
Date: Mon, 21 Oct 2013 11:58:55 +0200
Subject: [PATCH 10/11] Set default window size to 2MB
The default channel window size used until now was 256KB. This value is
too small and results on a bottleneck on real-life networks where
round-trip delays can easily reach 300ms.
The issue was not visible because the configured channel window size
was being ignored and a hard-coded value of ~22MB being used instead,
but that was fixed on a previous commit.
This patch just changes the default window size
(LIBSSH2_CHANNEL_WINDOW_DEFAULT) to 2MB. It is the same value used by
OpenSSH and in our opinion represents a good compromise between memory
used and transfer speed.
Performance tests were run to determine the optimum value. The details
and related discussion are available from the following thread on the
libssh2 mailing-list:
http://www.libssh2.org/mail/libssh2-devel-archive-2013-10/0018.shtml
http://article.gmane.org/gmane.network.ssh.libssh2.devel/6543
An excerpt follows:
"I have been running some transfer test and measuring their speed.
My setup was composed of a quad-core Linux machine running Ubuntu 13.10
x86_64 with a LXC container inside. The data transfers were performed
from the container to the host (never crossing through a physical
network device).
Network delays were simulated using the tc tool. And ping was used to
verify that they worked as intended during the tests.
The operation performed was the equivalent to the following ssh command:
$ ssh container "dd bs=16K count=8K if=/dev/zero" >/dev/null
Though, establishment and closing of the SSH connection was excluded
from the timings.
I run the tests several times transferring files of sizes up to 128MB
and the results were consistent between runs.
The results corresponding to the 128MB transfer are available here:
https://docs.google.com/spreadsheet/ccc?key=0Ao1yRmX6PQQzdG5wSFlrZl9HRWNET3ZyN0hnaGo5ZFE&usp=sharing
It clearly shows that 256KB is too small as the default window size.
Moving to a 512MB generates a great improvement and after the 1MB mark
the returns rapidly diminish. Other factors (TCP window size, probably)
become more limiting than the channel window size
For comparison I also performed the same transfers using OpenSSH. Its
speed is usually on par with that of libssh2 using a window size of 1MB
(even if it uses a 2MB window, maybe it is less aggressive sending the
window adjust msgs)."
Signed-off-by: Salvador Fandino <sfandino@yahoo.com>
[upstream commit 85a827d1bceb9abd4442f225dd7c65ef5cefdc32]
Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
include/libssh2.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/libssh2.h b/include/libssh2.h
index 9b1a6e1..df873fc 100644
--- a/include/libssh2.h
+++ b/include/libssh2.h
@@ -587,7 +587,7 @@ LIBSSH2_API int libssh2_poll(LIBSSH2_POLLFD *fds, unsigned int nfds,
long timeout);
/* Channel API */
-#define LIBSSH2_CHANNEL_WINDOW_DEFAULT (256*1024)
+#define LIBSSH2_CHANNEL_WINDOW_DEFAULT (2*1024*1024)
#define LIBSSH2_CHANNEL_PACKET_DEFAULT 32768
#define LIBSSH2_CHANNEL_MINADJUST 1024
--
1.7.1

View File

@ -0,0 +1,66 @@
From baadc811a703b9a6dec655c7afb3218d8cff51fa Mon Sep 17 00:00:00 2001
From: Daniel Stenberg <daniel@haxx.se>
Date: Sun, 16 Mar 2014 20:02:37 +0100
Subject: [PATCH 11/11] channel_receive_window_adjust: store windows size always
Avoid it sometimes returning without storing it, leaving calling
functions with unknown content!
Detected by clang-analyzer
[upstream commit fcb601da7b37c6e9bbcd264199597e2ddb7bc347]
Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
src/channel.c | 11 ++++-------
1 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/src/channel.c b/src/channel.c
index 36c75d2..1d074df 100644
--- a/src/channel.c
+++ b/src/channel.c
@@ -1,6 +1,6 @@
/* Copyright (c) 2004-2007 Sara Golemon <sarag@libssh2.org>
* Copyright (c) 2005 Mikhail Gusarov <dottedmag@dottedmag.net>
- * Copyright (c) 2008-2011 by Daniel Stenberg
+ * Copyright (c) 2008-2014 by Daniel Stenberg
*
* All rights reserved.
*
@@ -1544,6 +1544,9 @@ _libssh2_channel_receive_window_adjust(LIBSSH2_CHANNEL * channel,
{
int rc;
+ if(store)
+ *store = channel->remote.window_size;
+
if (channel->adjust_state == libssh2_NB_state_idle) {
if (!force
&& (adjustment + channel->adjust_queue <
@@ -1553,14 +1556,10 @@ _libssh2_channel_receive_window_adjust(LIBSSH2_CHANNEL * channel,
"for channel %lu/%lu",
adjustment, channel->local.id, channel->remote.id);
channel->adjust_queue += adjustment;
- if(store)
- *store = channel->remote.window_size;
return 0;
}
if (!adjustment && !channel->adjust_queue) {
- if(store)
- *store = channel->remote.window_size;
return 0;
}
@@ -1598,8 +1597,6 @@ _libssh2_channel_receive_window_adjust(LIBSSH2_CHANNEL * channel,
channel->adjust_state = libssh2_NB_state_idle;
- if(store)
- *store = channel->remote.window_size;
return 0;
}
--
1.7.1

View File

@ -12,7 +12,7 @@
Name: libssh2
Version: 1.4.3
Release: 8%{?dist}
Release: 9%{?dist}
Summary: A library implementing the SSH2 protocol
Group: System Environment/Libraries
License: BSD
@ -24,6 +24,12 @@ Patch2: 0002-sftp-statvfs-Along-error-path-reset-the-correct-stat.patch
Patch3: 0003-sftp-Add-support-for-fsync-OpenSSH-extension.patch
Patch4: 0004-partially-revert-window_size-explicit-adjustments-on.patch
Patch5: 0005-channel.c-fix-a-use-after-free.patch
Patch6: 0006-_libssh2_channel_write-client-spins-on-write-when-wi.patch
Patch7: 0007-window_size-redid-window-handling-for-flow-control-r.patch
Patch8: 0008-_libssh2_channel_read-fix-data-drop-when-out-of-wind.patch
Patch9: 0009-_libssh2_channel_read-Honour-window_size_initial.patch
Patch10: 0010-Set-default-window-size-to-2MB.patch
Patch11: 0011-channel_receive_window_adjust-store-windows-size-alw.patch
BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(id -nu)
BuildRequires: openssl-devel
BuildRequires: zlib-devel
@ -88,6 +94,14 @@ sed -i s/4711/47%{?__isa_bits}/ tests/ssh2.{c,sh}
# https://trac.libssh2.org/ticket/268
%patch5 -p1
# Fix curl's excessive memory consumption during scp download
%patch6 -p1
%patch7 -p1
%patch8 -p1
%patch9 -p1
%patch10 -p1
%patch11 -p1
# Make sshd transition appropriately if building in an SELinux environment
%if !(0%{?fedora} >= 17 || 0%{?rhel} >= 7)
chcon $(/usr/sbin/matchpathcon -n /etc/rc.d/init.d/sshd) tests/ssh2.sh || :
@ -154,6 +168,9 @@ rm -rf %{buildroot}
%{_libdir}/pkgconfig/libssh2.pc
%changelog
* Wed Apr 30 2014 Kamil Dudka <kdudka@redhat.com> 1.4.3-9
- Fix curl's excessive memory consumption during scp download
* Wed Aug 14 2013 Kamil Dudka <kdudka@redhat.com> 1.4.3-8
- fix very slow sftp upload to localhost
- fix a use after free in channel.c