channels: Avoid reusing stack-local variables for callbacks

Previously, stack-local variables held the callback structure.
This worked as long as the call stack was not overridden by other
function calls or as long as the server closed the channel
very early. In other cases, the delayed close in libssh
could result in calling the callbacks even after we believed
the channel was freed, causing invalid memory access and crashes.

The accompanying change to libssh was merged to avoid calling
callbacks on channels the caller considers freed here:

https://gitlab.com/libssh/libssh-mirror/-/merge_requests/549/

But we will be using older libssh versions for some time so we
need a workaround in pylibssh too.

Fixes ansible#57

Signed-off-by: Jakub Jelen <[email protected]>

src/pylibsshext/channel.pxd

@@ -23,3 +23,7 @@ cdef class Channel:
     cdef  _session
     cdef libssh.ssh_channel _libssh_channel
     cdef libssh.ssh_session _libssh_session
+
+cdef class ChannelCallback:
+    cdef callbacks.ssh_channel_callbacks_struct callback
+    cdef _userdata

src/pylibsshext/channel.pyx

@@ -45,6 +45,16 @@ cdef int _process_outputs(libssh.ssh_session session,
         result.stdout += data_b
     return len
 
+cdef class ChannelCallback:
+    def __cinit__(self):
+        memset(&self.callback, 0, sizeof(self.callback))
+        callbacks.ssh_callbacks_init(&self.callback)
+        self.callback.channel_data_function = <callbacks.ssh_channel_data_callback>&_process_outputs
+
+    def set_user_data(self, userdata):
+        self._userdata = userdata
+        self.callback.userdata = <void *>self._userdata
+
 cdef class Channel:
     def __cinit__(self, session):
         self._session = session
@@ -166,12 +176,12 @@ cdef class Channel:
             raise LibsshChannelException("Failed to execute command [{0}]: [{1}]".format(command, rc))
         result = CompletedProcess(args=command, returncode=-1, stdout=b'', stderr=b'')
 
-        cdef callbacks.ssh_channel_callbacks_struct cb
-        memset(&cb, 0, sizeof(cb))
-        cb.channel_data_function = <callbacks.ssh_channel_data_callback>&_process_outputs
-        cb.userdata = <void *>result
-        callbacks.ssh_callbacks_init(&cb)
-        callbacks.ssh_set_channel_callbacks(channel, &cb)
+        cb = ChannelCallback()
+        cb.set_user_data(result)
+        callbacks.ssh_set_channel_callbacks(channel, &cb.callback)
+
+        # keep the callback around in the session object to avoid use after free
+        self._session.push_callback(cb)
 
         libssh.ssh_channel_send_eof(channel)
         result.returncode = libssh.ssh_channel_get_exit_status(channel)

src/pylibsshext/session.pxd

@@ -26,5 +26,6 @@ cdef class Session:
     cdef _hash_py
     cdef _fingerprint_py
     cdef _keytype_py
+    cdef _channel_callbacks
 
 cdef libssh.ssh_session get_libssh_session(Session session)

src/pylibsshext/session.pyx

@@ -107,6 +107,10 @@ cdef class Session(object):
         self._hash_py = None
         self._fingerprint_py = None
         self._keytype_py = None
+        # Due to delayed freeing of channels, some older libssh versions might expect
+        # the callbacks to be around even after we free the underlying channels so
+        # we should free them only when we terminate the session.
+        self._channel_callbacks = []
 
     def __cinit__(self, host=None, **kwargs):
         self._libssh_session = libssh.ssh_new()
@@ -123,6 +127,9 @@ cdef class Session(object):
             libssh.ssh_free(self._libssh_session)
             self._libssh_session = NULL
 
+    def push_callback(self, callback):
+        self._channel_callbacks.append(callback)
+
     @property
     def port(self):
         cdef unsigned int port_i