From e81f313a71e548638d9e9679226ee84b3b614f13 Mon Sep 17 00:00:00 2001 From: David Sommerseth Date: Tue, 10 Jan 2017 21:34:32 +0100 Subject: [PATCH] management: >REMOTE operation would overwrite ce change indicator If the management interface on a client received a signal while waiting for input on the management channel, the "connection entry changed" status would be overwritten even though nothing was changed. Which could lead into connecting to the wrong server. This patch improves this by adding a check if a bool value was changed to false. This change happens only on signals. Further, the former 'ret' value have been renamed to 'ce_changed', to clarify what the expected return value contains. Plus adding some comments related to this. And finally do some code style cleanup, breaking up too long lines, adding some air here and there to improve the readability. Signed-off-by: David Sommerseth Cc: Selva Nair Acked-by: Selva Nair Message-Id: <1484080473-10415-1-git-send-email-davids@openvpn.net> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg13851.html Signed-off-by: David Sommerseth --- src/openvpn/init.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/src/openvpn/init.c b/src/openvpn/init.c index b2547f42f..88518bda4 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -252,31 +252,42 @@ ce_management_query_remote(struct context *c) { struct gc_arena gc = gc_new(); volatile struct connection_entry *ce = &c->options.ce; - int ret = true; + int ce_changed = true; /* presume the connection entry will be changed */ + update_time(); if (management) { struct buffer out = alloc_buf_gc(256, &gc); - buf_printf(&out, ">REMOTE:%s,%s,%s", np(ce->remote), ce->remote_port, proto2ascii(ce->proto, ce->af, false)); + + buf_printf(&out, ">REMOTE:%s,%s,%s", np(ce->remote), ce->remote_port, + proto2ascii(ce->proto, ce->af, false)); management_notify_generic(management, BSTR(&out)); - ce->flags &= ~(CE_MAN_QUERY_REMOTE_MASK<flags |= (CE_MAN_QUERY_REMOTE_QUERY<flags>>CE_MAN_QUERY_REMOTE_SHIFT) & CE_MAN_QUERY_REMOTE_MASK) == CE_MAN_QUERY_REMOTE_QUERY) + + ce->flags &= ~(CE_MAN_QUERY_REMOTE_MASK << CE_MAN_QUERY_REMOTE_SHIFT); + ce->flags |= (CE_MAN_QUERY_REMOTE_QUERY << CE_MAN_QUERY_REMOTE_SHIFT); + while (((ce->flags >> CE_MAN_QUERY_REMOTE_SHIFT) + & CE_MAN_QUERY_REMOTE_MASK) == CE_MAN_QUERY_REMOTE_QUERY) { management_event_loop_n_seconds(management, 1); if (IS_SIG(c)) { - ret = false; + ce_changed = false; /* connection entry have not been set */ break; } } } + gc_free(&gc); + + if (ce_changed) { - const int flags = ((ce->flags>>CE_MAN_QUERY_REMOTE_SHIFT) & CE_MAN_QUERY_REMOTE_MASK); - ret = (flags != CE_MAN_QUERY_REMOTE_SKIP); + /* If it is likely a connection entry was modified, + * check what changed in the flags and that it was not skipped + */ + const int flags = ((ce->flags >> CE_MAN_QUERY_REMOTE_SHIFT) + & CE_MAN_QUERY_REMOTE_MASK); + ce_changed = (flags != CE_MAN_QUERY_REMOTE_SKIP); } - gc_free(&gc); - return ret; + return ce_changed; } #endif /* ENABLE_MANAGEMENT */ -- 2.47.2