From 99b4229abf01ea9b9ef916985e6139feed0c0bf2 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Wed, 15 Sep 2021 13:48:30 +0100 Subject: [PATCH] Make all Plugs have a log function, even if no-op. Commit 8f5e9a4f8dc796a introduced a segfault into both Windows Pageant and Windows connection sharing upstreams when they receive an incoming named-pipe connection. This occurs because the PlugVtables for those incoming connections had a null pointer in the 'log' field, because hitherto, only sockets involved with an outgoing connection expected to receive plug_log() notifications. But I added such a notification in make_handle_socket, forgetting that that function is used for both outgoing and incoming named-pipe connections (among other things). So now a Plug implementation that expects to be set up by the plug_accepting() method on a listener may still receive PLUGLOG_CONNECT_SUCCESS. I could fix that by adding a parameter to make_handle_socket telling it whether to send a notification, but that seems like more faff than is really needed. Simpler to make a rule that says _all_ Socket types must implement the log() method, even if only with a no-op function. We already have a no-op implementation of log(), in nullplug.c. So I've exposed that outside its module (in the same style as all the nullseat functions and so on), to make it really easy to add log() methods to PlugVtables that don't need one. --- network.h | 21 +++++++++++++++++++++ nullplug.c | 15 +++++++-------- pageant.c | 2 ++ ssh/sharing.c | 2 ++ 4 files changed, 32 insertions(+), 8 deletions(-) diff --git a/network.h b/network.h index 107e72c8..85c5a6d9 100644 --- a/network.h +++ b/network.h @@ -78,6 +78,13 @@ struct PlugVtable { * proxied through. This will typically be a wodge of * standard-error output from a local proxy command, so the * receiver should probably prefix it to indicate this. + * + * Note that sometimes log messages may be sent even to Socket + * types that don't involve making an outgoing connection, e.g. + * because the same core implementation (such as Windows handle + * sockets) is shared between listening and connecting sockets. So + * all Plugs must implement this method, even if only to ignore + * the logged events. */ void (*closing) (Plug *p, const char *error_msg, int error_code, bool calling_back); @@ -325,6 +332,20 @@ Socket *new_error_socket_consume_string(Plug *plug, char *errmsg); */ extern Plug *const nullplug; +/* + * Some trivial no-op plug functions, also in nullplug.c; exposed here + * so that other Plug implementations can use them too. + * + * In particular, nullplug_log is useful to Plugs that don't need to + * worry about logging. + */ +void nullplug_log(Plug *plug, PlugLogType type, SockAddr *addr, + int port, const char *err_msg, int err_code); +void nullplug_closing(Plug *plug, const char *error_msg, int error_code, + bool calling_back); +void nullplug_receive(Plug *plug, int urgent, const char *data, size_t len); +void nullplug_sent(Plug *plug, size_t bufsize); + /* ---------------------------------------------------------------------- * Functions defined outside the network code, which have to be * declared in this header file rather than the main putty.h because diff --git a/nullplug.c b/nullplug.c index 953f0348..380e4b4f 100644 --- a/nullplug.c +++ b/nullplug.c @@ -7,27 +7,26 @@ #include "putty.h" -static void nullplug_socket_log(Plug *plug, PlugLogType type, SockAddr *addr, - int port, const char *err_msg, int err_code) +void nullplug_log(Plug *plug, PlugLogType type, SockAddr *addr, + int port, const char *err_msg, int err_code) { } -static void nullplug_closing(Plug *plug, const char *error_msg, int error_code, - bool calling_back) +void nullplug_closing(Plug *plug, const char *error_msg, int error_code, + bool calling_back) { } -static void nullplug_receive( - Plug *plug, int urgent, const char *data, size_t len) +void nullplug_receive(Plug *plug, int urgent, const char *data, size_t len) { } -static void nullplug_sent(Plug *plug, size_t bufsize) +void nullplug_sent(Plug *plug, size_t bufsize) { } static const PlugVtable nullplug_plugvt = { - .log = nullplug_socket_log, + .log = nullplug_log, .closing = nullplug_closing, .receive = nullplug_receive, .sent = nullplug_sent, diff --git a/pageant.c b/pageant.c index 8ca9310b..fb0f86e1 100644 --- a/pageant.c +++ b/pageant.c @@ -1624,6 +1624,7 @@ static const PlugVtable pageant_connection_plugvt = { .closing = pageant_conn_closing, .receive = pageant_conn_receive, .sent = pageant_conn_sent, + .log = nullplug_log, }; static int pageant_listen_accepting(Plug *plug, @@ -1672,6 +1673,7 @@ static int pageant_listen_accepting(Plug *plug, static const PlugVtable pageant_listener_plugvt = { .closing = pageant_listen_closing, .accepting = pageant_listen_accepting, + .log = nullplug_log, }; struct pageant_listen_state *pageant_listener_new( diff --git a/ssh/sharing.c b/ssh/sharing.c index b5da8657..920218de 100644 --- a/ssh/sharing.c +++ b/ssh/sharing.c @@ -1905,6 +1905,7 @@ static const PlugVtable ssh_sharing_conn_plugvt = { .closing = share_closing, .receive = share_receive, .sent = share_sent, + .log = nullplug_log, }; static int share_listen_accepting(Plug *plug, @@ -2046,6 +2047,7 @@ bool ssh_share_test_for_upstream(const char *host, int port, Conf *conf) static const PlugVtable ssh_sharing_listen_plugvt = { .closing = share_listen_closing, .accepting = share_listen_accepting, + .log = nullplug_log, }; void ssh_connshare_provide_connlayer(ssh_sharing_state *sharestate,