resolving trivial issue found by clang static analyzer variable "ret" is assigned a value that is redefined later

PATCH In review Ilya Shipitsin 4 participants 12 messages Jan 10, 2017 – Jan 12, 2017

Share & tools

Ilya Shipitsin

author

2017-01-10 08:18 UTC #0 lore raw

PATCH 1/1 resolving trivial issue found by clang static analyzer variable "ret" is assigned a value that is redefined later

details can be found here: https://delft.syzzer.nl/openvpn-scan-build/2017-01-10-000020-23912-1/report-856cea.html#EndPath

---
 src/openvpn/init.c | 1 -
 1 file changed, 1 deletion(-)

src/openvpn/init.c +0 -1

index b2547f4..5b39929 100644
@@ -266,7 +266,6 @@ ce_management_query_remote(struct context *c)
             management_event_loop_n_seconds(management, 1);
             if (IS_SIG(c))
             {
-                ret = false;
                 break;
             }
         }

Signature · 1 line

2.5.5

Selva Nair

2017-01-10 15:20 UTC #1 lore raw

src/openvpn/init.c ce_management_query_remote(struct context *c) · line 271

Quoted 13 lines · Ilya Shipitsin

src/openvpn/init.c +0 -1

index b2547f4..5b39929 100644
@@ -266,7 +266,6 @@ ce_management_query_remote(struct context *c)
             management_event_loop_n_seconds(management, 1);
             if (IS_SIG(c))
             {
-                ret = false;
                 break;
             }
         }

This looks like an arbitrary fix to silence the analyzer. Something else is likely wrong here. Why "ret" is over-written later (line 276) as

ret = (flags != CE_MAN_QUERY_REMOTE_SKIP);

is the real question. Looks like a bug that causes signal to be ignored.

Selva

Илья Шипицин

author

2017-01-10 15:47 UTC #2 lore raw

2017-01-10 20:20 GMT+05:00 Selva Nair <[email protected]...>:

Quoted 21 lines

src/openvpn/init.c ce_management_query_remote(struct context *c) · line 271
Quoted 13 lines · Ilya Shipitsin
src/openvpn/init.c +0 -1
index b2547f4..5b39929 100644
@@ -266,7 +266,6 @@ ce_management_query_remote(struct context *c)
             management_event_loop_n_seconds(management, 1);
             if (IS_SIG(c))
             {
-                ret = false;
                 break;
             }
         }
This looks like an arbitrary fix to silence the analyzer. Something else is likely wrong here. Why "ret" is over-written later (line 276) as

Quoted 4 lines

ret = (flags != CE_MAN_QUERY_REMOTE_SKIP);
is the real question. Looks like a bug that causes signal to be ignored.

hmm

Quoted 4 lines

Selva

David Sommerseth

2017-01-10 19:12 UTC #3 lore raw

In reply to Ilya Shipitsin

Quoted 11 lines · Ilya Shipitsin

details can be found here: https://delft.syzzer.nl/openvpn-scan-build/2017-01-10-000020-23912-1/report-856cea.html#EndPath
---
 src/openvpn/init.c | 1 -
 1 file changed, 1 deletion(-)
src/openvpn/init.c +0 -0
index b2547f4..5b39929 100644

src/openvpn/init.c ce_management_query_remote(struct context *c) · line 271

Quoted 7 lines

src/openvpn/init.c +0 -1

@@ -266,7 +266,6 @@ ce_management_query_remote(struct context *c)
             management_event_loop_n_seconds(management, 1);
             if (IS_SIG(c))
             {
-                ret = false;
                 break;
             }
         }

Hmm ... After having looked through a little bit of the code paths in init.c, this seems very funky and odd.

First of all, the ret variable will be overwritten no doubt about that. And the IS_SIG(c) check is "duplicated" where ce_management_query_remote() is called [init.c:401]. But not without a reason.

The break statement in ce_management_query_remote() is needed in the IS_SIG(c) check to exit a local while() loop. This is crucial so that OpenVPN is not caught in an inescapable loop on signals. And then a similar check happens where ce_management_query_remote() is called, also to escape a while() loop. Without these, OpenVPN can end up in a lock until the local "management client" have done its duties. Which is not ideal.

The attached patch is cleaning up this a bit, avoiding some of the scoping and the not really useful 'bool ret' variable. If someone can do a thorough test where the management interface is used and the management interface is used to overwrite --remote the option, then we might have a confirmation this fix is right. See commit 54561af63699e7408 and doc/management-notes.txt (look for 'remote ') for more info. Once we have a confirmation on the fix, I can produce a proper patch.

--
kind regards,

David Sommerseth
OpenVPN Technologies, Inc

src/openvpn/init.c +4 -7

index 9a3e29d..5b1248f 100644
@@ -252,7 +252,7 @@ ce_management_query_remote(struct context *c)
 {
     struct gc_arena gc = gc_new();
     volatile struct connection_entry *ce = &c->options.ce;
-    int ret = true;
+
     update_time();
     if (management)
     {

@@ -266,17 +266,14 @@ ce_management_query_remote(struct context *c)
             management_event_loop_n_seconds(management, 1);
             if (IS_SIG(c))
             {
-                ret = false;
                 break;
             }
         }
     }
-    {
-        const int flags = ((ce->flags>>CE_MAN_QUERY_REMOTE_SHIFT) & CE_MAN_QUERY_REMOTE_MASK);
-        ret = (flags != CE_MAN_QUERY_REMOTE_SKIP);
-    }
     gc_free(&gc);
-    return ret;
+
+    const int flags = ((ce->flags>>CE_MAN_QUERY_REMOTE_SHIFT) & CE_MAN_QUERY_REMOTE_MASK);
+    return (flags != CE_MAN_QUERY_REMOTE_SKIP);;
 }
 #endif /* ENABLE_MANAGEMENT */

Selva Nair

2017-01-10 19:50 UTC #4 lore raw

Quoted 3 lines · David Sommerseth

The attached patch is cleaning up this a bit, avoiding some of the scoping and the not really useful 'bool ret' variable.

In the patch you correctly break out of the while() on when man-even-loop prematurely returns due to signal but do not return false on signal. This could potentially cause the current remote to be accepted if the UI sends a signal instead of ACCEPT in answer to the >REMOTE query.

Here is my suggestion which involves only a very small change:

src/openvpn/init.c ce_management_query_remote(struct context *c) · line 255

Quoted 4 lines

    volatile struct connection_entry *ce = &c->options.ce;

src/openvpn/init.c +1 -1

-    int ret = true;
+

leave that as in original

src/openvpn/init.c ce_management_query_remote(struct context *c) · line 269

Quoted 10 lines

     update_time();
     if (management)
     {

src/openvpn/init.c +0 -1

@@ -266,17 +266,14 @@ ce_management_query_remote(struct context *c)
             management_event_loop_n_seconds(management, 1);
             if (IS_SIG(c))
             {
-                ret = false;

leave this as in original

src/openvpn/init.c ce_management_query_remote(struct context *c) · line 279

Quoted 10 lines

src/openvpn/init.c +0 -4

                 break;
             }
         }
     }
-    {
-        const int flags = ((ce->flags>>CE_MAN_QUERY_REMOTE_SHIFT) &
 CE_MAN_QUERY_REMOTE_MASK);
-        ret = (flags != CE_MAN_QUERY_REMOTE_SKIP);
-    }

Make the above conditional on ret = true -- i.e., if ret has turned false due to signal do not change it.

src/openvpn/init.c ce_management_query_remote(struct context *c) · line 279

Quoted 3 lines

src/openvpn/init.c +0 -1

     gc_free(&gc);
-    return ret;

Leave as in original

Quoted 5 lines

(quoted diff) +2 -0
+
+    const int flags = ((ce->flags>>CE_MAN_QUERY_REMOTE_SHIFT) &
CE_MAN_QUERY_REMOTE_MASK);
+ return (flags != CE_MAN_QUERY_REMOTE_SKIP);;

delete these

Quoted 1 line

}

#endif /* ENABLE_MANAGEMENT */

Selva

David Sommerseth

2017-01-10 20:33 UTC #5 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 10 lines · Selva Nair

On Tue, Jan 10, 2017 at 2:12 PM, David Sommerseth
<[email protected]...
<mailto:[email protected]...>> wrote:
The attached patch is cleaning up this a bit, avoiding some of the scoping and the not really useful 'bool ret' variable.

Quoted 6 lines

In the patch you correctly break out of the while() on when man-even-loop prematurely returns due to signal but do not return false on signal. This could potentially cause the current remote to be accepted if the UI sends a signal instead of ACCEPT in answer to the
Quoted 1 line
REMOTE query.

Ahh! Good catch. I did consider this approach, but presumed that James had the variable overwritten on purpose. But I believe you are right, I see this have a potential to backfire if overwritten.

I'll attach two patches to this mail thread. The first one fixes this issue, as suggested - but where I improve the comments and add a few code style fixes as well. The second patch just removes a redundant #ifdef ENABLE_MANAGEMENT block, where these ce_managmenet_remote_*() functions are used.

- --
kind regards,

David Sommerseth
OpenVPN Technologies, Inc

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJYdUUhAAoJEIbPlEyWcf3yxU8P/1jZy2IaUB1IgVQLQk+oqEqS 5FJb7gMYzpOxborF3y3O5vUhjboU4wNnL4wYbymarOyZ3V83fxlfZ1LHAnQh8cVZ PCKvRMXFfnbwLk40Rf8Bkv2WQw5EjXUsfEREy/Pg7DdIcGkCl8ckumV6aOLkCTfv 71oObTAZMF9tQPDxDvgySJyt8A4sRYNEaJxowu0iguIVVgl+IVPp/wToV7txuD0c kvjMee03NxtTh1HZgFqvBvfaCJ+1FZpvEf5DZ+WYwhFpWxgn1shw4XDYafgWpU9Z v5cd//y7RY3o8m7NdgNnJH68/lSu9LrIQPhj9YARPFfrte5QTkk5E597Ws+7x12O gCJlyHLTQKMlRNTLX6mtZHrY957xFdTlBuMsRNKF3s0bzLRP8b3L7UPYgtt4/UPz 5AETZGywCk4ij0dSsctJQcy1ejVqPXrEAw+hYli3tEJd5mpP6b0XUc5+0pcTJpfr zR01okfOAW2uc7LCt/Upn7td1nVuyxvmNeJdrnqTF5ZNEpTDRcGHzZxzpXG5ECIT vjFRY9+GmxJGFNVlYTlgUtZN3H1NuK2wGyP3pzuI0FKyFgiRMiGV8VUkFpOypXsA Dd8NXd0MZW2C+R5W7bUwi6ma3UcIPGs61yHn6KfCFMJLen23fLY9AZFEtAaXTIE9 jFOSt6NkJuNVAkkrhzGP
=jXw0
-----END PGP SIGNATURE-----

David Sommerseth

2017-01-10 20:34 UTC #6 lore raw

PATCH 1/2 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 <[email protected]...>

Cc Selva Nair <[email protected]...>

---
 src/openvpn/init.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

src/openvpn/init.c +22 ~~-11~~

index 9a3e29d..4786232 100644
@@ -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;
             }
         }
     }
-    {
-        const int flags = ((ce->flags>>CE_MAN_QUERY_REMOTE_SHIFT) & CE_MAN_QUERY_REMOTE_MASK);
-        ret = (flags != CE_MAN_QUERY_REMOTE_SKIP);
-    }
     gc_free(&gc);
-    return ret;
+
+    if (ce_changed)
+    {
+        /* 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);
+    }
+    return ce_changed;
 }
 #endif /* ENABLE_MANAGEMENT */

Signature · 1 line

1.8.3.1

David Sommerseth

2017-01-10 20:34 UTC #7 lore raw

PATCH 2/2 management: Remove a redundant #ifdef block

Bascially removes two independent #ifdef ENABLE_MANAGEMENT blocks into a single block, which makes the logic flow more easy to read.

Signed-off-by David Sommerseth <[email protected]...>

Cc Selva Nair <[email protected]...>

---
 src/openvpn/init.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

src/openvpn/init.c +1 -5

index 4786232..5296345 100644
@@ -414,11 +414,7 @@ next_connection_entry(struct context *c)
                 break;
             }
         }
-        else
-#endif
-
-#ifdef ENABLE_MANAGEMENT
-        if (ce_defined && management && management_query_proxy_enabled(management))
+        else if (ce_defined && management && management_query_proxy_enabled(management))
         {
             ce_defined = ce_management_query_proxy(c);
             if (IS_SIG(c))

Signature · 1 line

1.8.3.1

Selva Nair

2017-01-11 19:52 UTC #8 lore raw

Re: [Openvpn-devel] [PATCH 2/2] management: Remove a redundant #ifdef block

Quoted 26 lines · David Sommerseth

Bascially removes two independent #ifdef ENABLE_MANAGEMENT blocks into a single block, which makes the logic flow more easy to read.

Signed-off-by David Sommerseth <[email protected]...>

Cc Selva Nair <[email protected]...>

---
 src/openvpn/init.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

src/openvpn/init.c +1 -5

index 4786232..5296345 100644
@@ -414,11 +414,7 @@ next_connection_entry(struct context *c)
                 break;
             }
         }
-        else
-#endif
-
-#ifdef ENABLE_MANAGEMENT
-        if (ce_defined && management && management_query_proxy_enabled
 (management))
+        else if (ce_defined && management &&
 management_query_proxy_enabled(management))
         {

Quoted 3 lines

             ce_defined = ce_management_query_proxy(c);
             if (IS_SIG(c))

One more ifdef gone, and good riddance!

ACK.

David Sommerseth

2017-01-12 02:03 UTC #9 lore raw

Re: [Openvpn-devel] [PATCH applied] management: Remove a redundant #ifdef block

In reply to David Sommerseth

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Your patch has been applied to the following branches

commit 7b02cc2aa8318dc8f2677064dadcbec295b2f937 (master) commit 977f0b9ff4b600212b603279153ff1b1b10cf527 (release/2.4)

Author: David Sommerseth
Date:   Tue Jan 10 21:34:33 2017 +0100

     management: Remove a redundant #ifdef block

     Signed-off-by: David Sommerseth <[email protected]...>
     Acked-by: Selva Nair <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg13852.html
     Signed-off-by: David Sommerseth <[email protected]...>

- --
kind regards,

David Sommerseth

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJYduP4AAoJEIbPlEyWcf3yhSEP/16NtW0V/gWe6jwW7RJcCcHA 9wmMBaE9JVPahcUcx4i4jQbgjdSAoa1/Zm9TY47YX/ZW8Axs0xcYvkxiAkYMxyo9 +oZKdScNBS2B6hSv+Cl9Um/VcwWx6Nli0U1NaKAed+i63iXxV6WEAmL0EKx19Afg xIPS3Etep/4wFAgJkzqIb3H6EAHvGHKuCGt5Sp1KnF9faeGyAlm/9wGJ4rl4q0+7 bcI5U4c+33XIlM2OZ2QYO1qz9bK+sh7cfVuT1qbZ129UocMyOlgt0QTA42xqr0kh C1b36x83DttIsqd1Tz7UbJUrth4ABYP9dka6nV7bzs/3XvMjkhzFP5aWBlKHCV3r +6jV2zMAt5oYMUlDpk6b0BnyHd2FcU9sz688/1OhfmvoRXKof+XT1rlSxbvfHS+n 6izZowaa3kLV4jbv8DV6KJRxHPAJQ5NK4fuU/p/YFby3I/4qYR93MvT/LA2eEl+E /d7plBke3TrnAqjzQgU0Cwqnfsb0/SsS6HVFy/xpsuOwAZCOHTsvqCRhLfViefQs OIrsrOkNfgx5onOIT5hTGfhGUp6FsZOiKOHOPd2MHz6d8xOh6px1R/l2LYTPG+YG egOh8w+7q08kit9Pa9wrjg6i+tHDZCjpm/9iXu9jh0mQV0Fa9jfX/SPQmRk1LrRE kjWvn/TlxGahZRqp85gc
=CvFt
-----END PGP SIGNATURE-----

Selva Nair

2017-01-11 03:21 UTC #10 lore raw

Re: [Openvpn-devel] [PATCH 1/2] management: >REMOTE operation would overwrite ce change indicator

In reply to David Sommerseth

Quoted 6 lines · David Sommerseth

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.

Actually your earlier comment is right: that is, IS_SIG(c) repeated in next_connection_entry() makes what is returned from here irrelevant once a signal has triggered. But I still think this is the "correct" approach.

ACK from me.

Selva

David Sommerseth

2017-01-12 02:03 UTC #11 lore raw

Re: [Openvpn-devel] [PATCH applied] management: >REMOTE operation would overwrite ce change indicator

In reply to David Sommerseth

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Your patch has been applied to the following branches

commit e81f313a71e548638d9e9679226ee84b3b614f13 (master) commit a853cd060eb61df77055cbb92e97ad7f245f2316 (release/2.4)

Author: David Sommerseth
Date:   Tue Jan 10 21:34:32 2017 +0100

     management: >REMOTE operation would overwrite ce change indicator

     Signed-off-by: David Sommerseth <[email protected]...>
     Acked-by: Selva Nair <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg13851.html
     Signed-off-by: David Sommerseth <[email protected]...>

- --
kind regards,

David Sommerseth

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJYduPjAAoJEIbPlEyWcf3yns0P/RNsveEAvleUcBQNlJ9rzKL4 XYojnRhbdGzuuNQhgWj+49+HG9gYgCy4XrRRcMEaw/trwBZVOSF9viI10HNuUSnT fHO3sIFNHMgEUPNiP2GGzEiD1EEgE2hAKLjaCjTzvfLKvOiC9ZaH+WTj71Q0ppax XLsM/B5g6OiD4X5ZPsgJ+ZY2yyb7Mxe/DOFI05pkZ59CA2t0vMP1kKSjf2le+51+ iCcvfDV0Fx9px5JGOtsrkI/K0Wp59ip3N18xqYp15Wd2wj3ANv9zA0mfp1ZK/jDz cIRoA/qG2pS7txgyVvbtDyfmYJnWTnmnBP+hhR859ykhUSBUyQN6zliwhCislqv9 36glnIkbbmyXCYIAPIxx7Fh6ZKnh+whu+RQ+GR1cPR5rl08qofZPR7JygdJDx/as i4JR51fFpFBd94zsQpf1NYBPoORccgsbzyl8y6dNOkkdyCl5pxkOmScUuIUzCMwP UFalNUH2kopEGwwZCXf+aTvpkQ7Q8UMhctCsX/EqcGcOzBzJ9eZV7mzvhwknkYMV QPgmR+YpWki4CGDnHSv43mTs87yRbY7xcn+u4002/Yv0F1HxRzL8Lz2RfvDD5eJA ppvOaVCDvEIObsdCIP29Fa0g7YA0F1h8e1BV98qF12WEC1v+S8d4qiAAHyN4aeQt r4MDwMpTCh1tFH2I9vj5
=X0eo
-----END PGP SIGNATURE-----