Subject: Memory leaks in out-of-memory conditions

Memory leaks in out-of-memory conditions

From: Dan Fandrich <dan_at_coneharvesters.com>
Date: Thu, 6 Feb 2014 23:48:53 +0100

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

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

Received on 2014-02-06