I've discovered a few memory leaks in libssh2, while running the curl torture
tests. I'm attaching a patch for review, as I'm not completely certain about
some of the error paths.
The first is in _libssh2_packet_add. This error path does not satisfy the
function's documented constraint:
* The input pointer 'data' is pointing to allocated data that this function
* is asked to deal with so on failure OR success, it must be freed fine.
i.e. it never gets freed. However, my fix for this completely drops the data
buffer. I suspect that this could result in a protocol violation or data if
libssh2 would continue to operate. If so, then it should probably be documented
that upon receipt of a LIBSSH2_ERROR_ALLOC error, the application should
immediately clean up the session. It's likely there are other OOM error paths
where this could also be a problem.
The second error results when a session is torn down prematurely, while a
transport payload is fully processed. There is currently no path to free this
buffer when the session is torn down, so I added one.
The third error isn't an OOM problem, but a potentially more serious
use-after-free one. There seems to be a path through _libssh2_transport_read
where this could occur. If the call to fullpacket results in
LIBSSH2_ERROR_EAGAIN, it must have come from _libssh2_packet_add, which as
previously discussed, always frees (or takes ownership of) its data argument.
However, the path ending in line 578 of transport.c does not reset total_num,
which is used to indicate whether payload has been allocated or not. That
means that the buffer was added to the brigade, but could still be written
into in future calls to _libssh2_transport_read. What I'm not certain about is
whether or not fullpacket() guarantees freeing or taking ownership of
p->payload. There is a path through fullpacket() where that could occur; I just
don't know if it could logically occur. If it could, then moving the
total_num=0 line up as I have isn't quite right.
>>> Dan
From 05c51895b55169c68bc0191c94d3769d09de5504 Mon Sep 17 00:00:00 2001
From: Dan Fandrich <d...@coneharvesters.com>
Date: Thu, 6 Feb 2014 23:10:53 +0100
Subject: [PATCH] Fixed a few memory leaks in out-of-memory paths
Also fixed a potential use-after-free.
---
src/packet.c | 1 +
src/session.c | 5 +++++
src/transport.c | 3 +--
3 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/packet.c b/src/packet.c
index 47bbf2b..9d937e3 100644
--- a/src/packet.c
+++ b/src/packet.c
@@ -967,6 +967,7 @@ _libssh2_packet_add(LIBSSH2_SESSION * session, unsigned
char *data,
if (!packetp) {
_libssh2_debug(session, LIBSSH2_ERROR_ALLOC,
"memory for packet");
+ LIBSSH2_FREE(session, data);
session->packAdd_state = libssh2_NB_state_idle;
return LIBSSH2_ERROR_ALLOC;
}
diff --git a/src/session.c b/src/session.c
index 9838d2b..3a52fd4 100644
--- a/src/session.c
+++ b/src/session.c
@@ -1017,6 +1017,11 @@ session_free(LIBSSH2_SESSION *session)
LIBSSH2_FREE(session, session->scpSend_command);
}
+ /* Free payload buffer */
+ if (session->packet.total_num) {
+ LIBSSH2_FREE(session, session->packet.payload);
+ }
+
/* Cleanup all remaining packets */
while ((pkg = _libssh2_list_first(&session->packets))) {
packets_left++;
diff --git a/src/transport.c b/src/transport.c
index b4ec037..8ba34e1 100644
--- a/src/transport.c
+++ b/src/transport.c
@@ -559,6 +559,7 @@ int _libssh2_transport_read(LIBSSH2_SESSION * session)
/* we have a full packet */
libssh2_transport_read_point1:
rc = fullpacket(session, encrypted);
+ p->total_num = 0; /* no packet buffer available */
if (rc == LIBSSH2_ERROR_EAGAIN) {
if (session->packAdd_state != libssh2_NB_state_idle)
@@ -578,8 +579,6 @@ int _libssh2_transport_read(LIBSSH2_SESSION * session)
return rc;
}
- p->total_num = 0; /* no packet buffer available */
-
return rc;
}
} while (1); /* loop */
--
1.8.1.5
_______________________________________________
libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel
