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>
(cherry picked from commit e81f313a71e548638d9e9679226ee84b3b614f13)

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 9a3e29d3f74451e14792f0e25db15260d3ca25d6..4786232c2df4c56c4531071a7b7be3295c9af58d 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 */