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 <davids@openvpn.net>
Cc: Selva Nair <selva.nair@gmail.com>
Acked-by: Selva Nair <selva.nair@gmail.com>
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 <davids@openvpn.net>

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index b2547f42f10d1f9547b43280f42c538e085fe974..88518bda4578840bb20976922f60b6ba125a1ee9 100644 (file)
--- 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<<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)
+
+        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 */