On 10/01/17 09:18, Ilya Shipitsin wrote: > 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(-) > > diff --git a/src/openvpn/init.c b/src/openvpn/init.c > index b2547f4..5b39929 100644 > --- a/src/openvpn/init.c > +++ b/src/openvpn/init.c > @@ -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
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 9a3e29d..5b1248f 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -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 */
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
