From 0a93b5d9bc6131c0cd84395f4aa88cac0cb40f23 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Wed, 19 Jul 2017 07:22:03 +0100 Subject: [PATCH] Stop ssh2_msg_channel_response using a stale ssh_channel. When it calls through ocr->handler() to process the response to a channel request, sometimes that call ends up back in the main SSH-2 authconn coroutine, and sometimes _that_ will call bomb_out(), which closes the whole SSH connection and frees all the channels - so that when control returns back up the call stack to ssh2_msg_channel_response itself which continues working with the channel it was passed, it's using freed memory and things go badly. This is the sort of thing I'd _like_ to fix using some kind of large-scale refactoring along the lines of moving all the actual free() calls out into top-level callbacks, so that _any_ function which is holding a pointer to something can rely on that pointer still being valid after it calls a subroutine. But I haven't worked out all the details of how that system should work, and doubtless it will turn out to have problems of its own once I do, so here's a point fix which simply checks if the whole SSH session has been closed (which is easy - much easier than checking if that _channel_ structure still exists) and fixes the immediate bug. (I think this is the real fix for the problem reported by the user I mention in commit f0126dd19, because I actually got the details wrong in the log message for that previous commit: the user's SSH server wasn't rejecting the _opening_ of the main session channel, it was rejecting the "shell" channel request, so this code path was the one being exercised. Still, the other bug was real too, so no harm done!) --- ssh.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ssh.c b/ssh.c index 89041f48..aa9fd5dd 100644 --- a/ssh.c +++ b/ssh.c @@ -8105,6 +8105,8 @@ static void ssh2_msg_channel_response(Ssh ssh, struct Packet *pktin) return; } ocr->handler(c, pktin, ocr->ctx); + if (ssh->state == SSH_STATE_CLOSED) + return; /* in case the handler called bomb_out(), which some can */ c->v.v2.chanreq_head = ocr->next; sfree(ocr); /*