Re: [Openvpn-devel] [PATCH applied] Re: Print time_t as long long and suseconds_t as long
On Sat, Nov 04 2017, Gert Doering wrote: > ACK, thanks. Reviewed by "staring at the code" (changes are only what it > says on the tin), compiling with clang and gcc (no format related warnings), > and running tests, of course :-) Cool, thank you for your time. > I'm a bit undecided whether this should go to release/2.4 as well, as it > is "refactoring" and our general policy is "refactoring goes into master" > (but then it's straightforward enough). Leaving it in master for now, > but I can be convinced that we want this in 2.4... FWIW I'm fine with whatever approach you prefer. openvpn-2.5.0 will hopefully be released before 2038. :) Here's another small diff, I forgot one suseconds_t occurrence. From cbe8237ff59129501e1e92c8fd6a3488d94c4c0f Mon Sep 17 00:00:00 2001 From: Jeremie Courreges-Anglas Date: Sun, 5 Nov 2017 10:03:26 +0100 Subject: [PATCH] Cast and print another suseconds_t as long Missed in 31b5c0e --- src/openvpn/forward.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index a91a8d9a..1b7455bb 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -649,7 +649,7 @@ check_timeout_random_component_dowork(struct context *c) c->c2.timeout_random_component.tv_usec = (time_t) get_random() & 0x0003; c->c2.timeout_random_component.tv_sec = 0; -dmsg(D_INTERVAL, "RANDOM USEC=%d", (int) c->c2.timeout_random_component.tv_usec); +dmsg(D_INTERVAL, "RANDOM USEC=%ld", (long) c->c2.timeout_random_component.tv_usec); } static inline void -- 2.14.3 -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE signature.asc Description: PGP signature -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Use long long to format time_t-related environment variables
Signed-off-by: Jeremie Courreges-Anglas
---
src/openvpn/misc.c| 4 ++--
src/openvpn/misc.h| 2 +-
src/openvpn/multi.c | 7 ++-
src/openvpn/options.c | 2 +-
4 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 8c7f6116..001fe1c4 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -553,10 +553,10 @@ setenv_int(struct env_set *es, const char *name, int
value)
}
void
-setenv_unsigned(struct env_set *es, const char *name, unsigned int value)
+setenv_long_long(struct env_set *es, const char *name, long long value)
{
char buf[64];
-openvpn_snprintf(buf, sizeof(buf), "%u", value);
+openvpn_snprintf(buf, sizeof(buf), "%lld", value);
setenv_str(es, name, buf);
}
diff --git a/src/openvpn/misc.h b/src/openvpn/misc.h
index eb39ce3f..f6c810a2 100644
--- a/src/openvpn/misc.h
+++ b/src/openvpn/misc.h
@@ -98,7 +98,7 @@ void setenv_counter(struct env_set *es, const char *name,
counter_type value);
void setenv_int(struct env_set *es, const char *name, int value);
-void setenv_unsigned(struct env_set *es, const char *name, unsigned int value);
+void setenv_long_long(struct env_set *es, const char *name, long long value);
void setenv_str(struct env_set *es, const char *name, const char *value);
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 4545bce1..82a0b9d9 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -565,10 +565,7 @@ multi_client_disconnect_setenv(struct multi_context *m,
setenv_stats(&mi->context);
/* setenv connection duration */
-{
-const unsigned int duration = (unsigned int) now - mi->created;
-setenv_unsigned(mi->context.c2.es, "time_duration", duration);
-}
+setenv_long_long(mi->context.c2.es, "time_duration", now - mi->created);
}
static void
@@ -1769,7 +1766,7 @@ multi_client_connect_setenv(struct multi_context *m,
{
const char *created_ascii = time_string(mi->created, 0, false, &gc);
setenv_str(mi->context.c2.es, "time_ascii", created_ascii);
-setenv_unsigned(mi->context.c2.es, "time_unix", (unsigned
int)mi->created);
+setenv_long_long(mi->context.c2.es, "time_unix", mi->created);
}
gc_free(&gc);
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 7aa311aa..641a26e2 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -990,7 +990,7 @@ setenv_settings(struct env_set *es, const struct options *o)
setenv_int(es, "verb", o->verbosity);
setenv_int(es, "daemon", o->daemon);
setenv_int(es, "daemon_log_redirect", o->log);
-setenv_unsigned(es, "daemon_start_time", time(NULL));
+setenv_long_long(es, "daemon_start_time", time(NULL));
setenv_int(es, "daemon_pid", platform_getpid());
if (o->connection_list)
--
2.14.3
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH applied] Re: buffer_list_aggregate_separator(): add unit tests
ACK, test code is good :-) (it passes the tests now, and does not change anything else). Applying to 2.4 as well, since "test good is good", even if we might not apply the refactoring bits there. Your patch has been applied to the master and release/2.4 branch. commit 2ddb527abe38f5866ff01e91f8ee89d0f9700762 (master) commit a202f6e74b32beabad764ee4226781a7f7e9c1c6 (release/2.4) Author: Steffan Karger Date: Sat Nov 4 23:45:50 2017 +0100 buffer_list_aggregate_separator(): add unit tests Signed-off-by: Steffan Karger Acked-by: Gert Doering Message-Id: <20171104224551.3079-1-stef...@karger.me> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15748.html Signed-off-by: Gert Doering -- kind regards, Gert Doering -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] Windows installer with updated pkcs11-helper (1.22) available for testing
Hi,
On Mon, Jul 17, 2017 at 02:22:55PM +0200, Jan Just Keijser wrote:
> On 17/07/17 14:14, Gert Doering wrote:
> > Hi,
> >
> > On Mon, Jul 17, 2017 at 02:10:11PM +0200, Jan Just Keijser wrote:
> >> this problem is NOT present in OpenVPN 2.3.17; the same warning appears
> >> (route gateway is ambiguous) but the route is added
> >> anyway. This seems to be a regression in 2.4.
> > Can we have a log, please?
> attached: config and log (with hostnames anonymized)
This indeed is a regression, or a "non-handled special case in the iservice"
(waking up Selva and Heiko).
2.3 is calling route.exe, which seems to just handle this case fine
("the given gateway address is present on two different interfaces",
which I find ambiguous myself :-) ).
2.4 in your setup is using the interactive service...
> Mon Jul 17 14:18:43 2017 us=1227 TEST ROUTES: 3/3 succeeded len=2 ret=1 a=1
> u/d=up
> Mon Jul 17 14:18:43 2017 us=1227 C:\Windows\system32\route.exe ADD
> 222.222.97.13 MASK 255.255.255.255 111.111.135.254
> Mon Jul 17 14:18:43 2017 us=1227 Warning: route gateway is ambiguous:
> 111.111.135.254 (2 matches)
> Mon Jul 17 14:18:43 2017 us=1227 Route addition via service failed
... which notices that the gateway is ambiguous and refuses to cooperate.
Without checking the code, there's a few things here that are not good
- that openvpn just goes ahead, while it "should" know that adding the
"def1" default routes afterwards is going to make things explode
- that we fail, instead of just installing the route (with warning) -
which could either be "just pick an interface and log that" or "just
pick no interface at all, and let windows routing figure this out"
> Mon Jul 17 14:18:43 2017 us=16827 Recursive routing detected, drop tun packet
> to [AF_INET]222.222.97.13:1194
> Mon Jul 17 14:18:44 2017 us=108829 Recursive routing detected, drop tun
> packet to [AF_INET]222.222.97.13:1194
Now *this* is actually good news :-) - instead of blowing up your CPU,
we notice that we're stuck and log that.
As a workaround, what you might do instead...
- connect over IPv6 - the IPv6 code is different and I'm curious what
it will do :-)
- use "--ip-win32 ipapi" (+ run gui as admin) to avoid using the
interactive service
gert
--
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de
signature.asc
Description: PGP signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] Windows installer with updated pkcs11-helper (1.22) available for testing
On Sun, Nov 5, 2017 at 12:11 PM, Gert Doering wrote:
> Hi,
>
> On Mon, Jul 17, 2017 at 02:22:55PM +0200, Jan Just Keijser wrote:
> > On 17/07/17 14:14, Gert Doering wrote:
> > > Hi,
> > >
> > > On Mon, Jul 17, 2017 at 02:10:11PM +0200, Jan Just Keijser wrote:
> > >> this problem is NOT present in OpenVPN 2.3.17; the same warning
> appears (route gateway is ambiguous) but the route is added
> > >> anyway. This seems to be a regression in 2.4.
> > > Can we have a log, please?
> > attached: config and log (with hostnames anonymized)
>
> This indeed is a regression, or a "non-handled special case in the
> iservice"
> (waking up Selva and Heiko).
> 2.3 is calling route.exe, which seems to just handle this case fine
> ("the given gateway address is present on two different interfaces",
> which I find ambiguous myself :-) ).
>
When this was reported I had made a patch to handle this, somehow forgot to
send it in. Or could be that I was not totally happy with it (or not
tested). Anyway,
the next email has the patch as I have it -- will do any cleanup if needed
provided
it looks sensible.
Selva
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Use lowest metric interface when multiple interfaces match a route
From: Selva Nair
Currently a route addition using IPAPI or service is skipped if the
route gateway is reachable by multiple interfaces. This changes that
to use the interface with lowest metric.
Reported by Jan Just Keijser
Signed-off-by: Selva Nair
---
src/openvpn/route.c | 3 +--
src/openvpn/tun.c | 14 --
2 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index 8c71e6e..3937018 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -2729,7 +2729,7 @@ get_default_gateway(struct route_gateway_info *rgi)
if (rgi->gateway.addr)
{
rgi->flags |= RGI_ADDR_DEFINED;
-a_index = adapter_index_of_ip(adapters, rgi->gateway.addr, NULL,
&rgi->gateway.netmask);
+a_index = dwForwardIfIndex;
if (a_index != TUN_ADAPTER_INDEX_INVALID)
{
rgi->adapter_index = a_index;
@@ -2780,7 +2780,6 @@ windows_route_find_if_index(const struct route_ipv4 *r,
const struct tuntap *tt)
msg(M_WARN, "Warning: route gateway is ambiguous: %s (%d matches)",
print_in_addr_t(r->gateway, 0, &gc),
count);
-ret = TUN_ADAPTER_INDEX_INVALID;
}
dmsg(D_ROUTE_DEBUG, "DEBUG: route find if: on_tun=%d count=%d index=%d",
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 3639718..d0461ef 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -45,6 +45,7 @@
#include "manage.h"
#include "route.h"
#include "win32.h"
+#include "block_dns.h"
#include "memdbg.h"
@@ -4483,6 +4484,7 @@ adapter_index_of_ip(const IP_ADAPTER_INFO *list,
struct gc_arena gc = gc_new();
DWORD ret = TUN_ADAPTER_INDEX_INVALID;
in_addr_t highest_netmask = 0;
+int lowest_metric = INT_MAX;
bool first = true;
if (count)
@@ -4496,9 +4498,11 @@ adapter_index_of_ip(const IP_ADAPTER_INFO *list,
if (is_ip_in_adapter_subnet(list, ip, &hn))
{
+int metric = get_interface_metric(list->Index, AF_INET);
if (first || hn > highest_netmask)
{
highest_netmask = hn;
+lowest_metric = metric;
if (count)
{
*count = 1;
@@ -4512,16 +4516,22 @@ adapter_index_of_ip(const IP_ADAPTER_INFO *list,
{
++*count;
}
+if (metric < lowest_metric)
+{
+ret = list->Index;
+lowest_metric = metric;
+}
}
}
list = list->Next;
}
-dmsg(D_ROUTE_DEBUG, "DEBUG: IP Locate: ip=%s nm=%s index=%d count=%d",
+dmsg(D_ROUTE_DEBUG, "DEBUG: IP Locate: ip=%s nm=%s index=%d count=%d
metric=%d",
print_in_addr_t(ip, 0, &gc),
print_in_addr_t(highest_netmask, 0, &gc),
(int)ret,
- count ? *count : -1);
+ count ? *count : -1,
+ metric);
if (ret == TUN_ADAPTER_INDEX_INVALID && count)
{
--
2.1.4
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] Windows installer with updated pkcs11-helper (1.22) available for testing
Hi, And the rationale for the patch: On Mon, Jul 17, 2017 at 8:22 AM, Jan Just Keijser wrote: > On 17/07/17 14:14, Gert Doering wrote: > >> Hi, >> >> On Mon, Jul 17, 2017 at 02:10:11PM +0200, Jan Just Keijser wrote: >> >>> this problem is NOT present in OpenVPN 2.3.17; the same warning appears >>> (route gateway is ambiguous) but the route is added >>> anyway. This seems to be a regression in 2.4. >>> >> Can we have a log, please? >> >> >> attached: config and log (with hostnames anonymized) As in the logs, Mon Jul 17 14:18:43 2017 us=1227 C:\Windows\system32\route.exe ADD 222.222.97.13 MASK 255.255.255.255 111.111.135.254 Mon Jul 17 14:18:43 2017 us=1227 Warning: route gateway is ambiguous: 111.111.135.254 (2 matches) when multiple interfaces match a route, we bail out with a warning unless the route method used is exe. This code is the same in 2.3.17 and 2.4.3 except the latter defaults to using IPAPI invoked via the service. 2.3.7 also defaults to IPAPI but falls back to exe if the former fails. Looking the sources: In tun.c adapter_index_of_ip() returns the index of the first matching adapter and the number of matching adapters found. If the count is > 1 windows_route_find_if_index() in route.c sets the index to TUN_ADAPTER_INDEX_INVALID and route addition gets aborted. We could slightly modify adapter_index_of_ip() to return the matching index with smallest metric when more than one are found. Selva -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved
Hi,
On Wed, Oct 11, 2017 at 01:11:26AM +0200, si...@rozman.si wrote:
> From: Simon Rozman
>
> ---
> src/openvpn/block_dns.c | 2 +-
> src/openvpnserv/automatic.c | 2 +-
> src/openvpnserv/common.c | 2 +-
> src/openvpnserv/interactive.c | 4 ++--
> src/openvpnserv/validate.c| 2 +-
> 5 files changed, 6 insertions(+), 6 deletions(-)
So... trying to make sense of this.
> diff --git a/src/openvpn/block_dns.c b/src/openvpn/block_dns.c
> index d43cbcf..f88ba2c 100644
> --- a/src/openvpn/block_dns.c
> +++ b/src/openvpn/block_dns.c
> @@ -370,7 +370,7 @@ get_interface_metric(const NET_IFINDEX index, const
> ADDRESS_FAMILY family)
> }
> return ipiface.Metric;
> }
> -return -err;
> +return -(int)err;
> }
This, I can somewhat agree to, as "err" is an unsigned type so there
might be a hidden integer overflow if it happens to be "large". Which it
won't be, but still.
>
> /*
> diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c
> index 4123d0f..6c39aaa 100644
> --- a/src/openvpnserv/automatic.c
> +++ b/src/openvpnserv/automatic.c
> @@ -155,7 +155,7 @@ match(const WIN32_FIND_DATA *find, LPCTSTR ext)
> * Modify the extension on a filename.
> */
> static bool
> -modext(LPTSTR dest, int size, LPCTSTR src, LPCTSTR newext)
> +modext(LPTSTR dest, size_t size, LPCTSTR src, LPCTSTR newext)
> {
> int i;
Not sure why this is needed? The code verifies that size is ">0", so
a signed variable is ok here.
>
> diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c
> index b8b817b..7901fd6 100644
> --- a/src/openvpnserv/common.c
> +++ b/src/openvpnserv/common.c
> @@ -36,7 +36,7 @@ openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format,
> va_list arglist)
> len = _vsntprintf(str, size, format, arglist);
> str[size - 1] = 0;
> }
> -return (len >= 0 && len < size);
> +return (len >= 0 && (size_t)len < size);
> }
This is, if I understand right, because "len < size" will implicitly cast
size to (int), causing integer overflow if size is too big for a signed
int?
> int
> openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...)
> diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
> index 8d94197..96e0de0 100644
> --- a/src/openvpnserv/interactive.c
> +++ b/src/openvpnserv/interactive.c
> @@ -188,7 +188,7 @@ typedef enum {
> static DWORD
> AsyncPipeOp(async_op_t op, HANDLE pipe, LPVOID buffer, DWORD size, DWORD
> count, LPHANDLE events)
> {
> -int i;
> +DWORD i;
... and this totally escapes me. Why would an "int" suddenly be no longer
a legal loop variable...?
> BOOL success;
> HANDLE io_event;
> DWORD res, bytes = 0;
> @@ -1061,7 +1061,7 @@ RegisterDNS(LPVOID unused)
> { ipcfg, L"ipconfig /flushdns",timeout },
> { ipcfg, L"ipconfig /registerdns", timeout },
> };
> -int ncmds = sizeof(cmds) / sizeof(cmds[0]);
> +DWORD ncmds = sizeof(cmds) / sizeof(cmds[0]);
Same thing here...
> index 7a2d0e3..5422d33 100644
> --- a/src/openvpnserv/validate.c
> +++ b/src/openvpnserv/validate.c
> @@ -298,7 +298,7 @@ IsUserInGroup(PSID sid, const PTOKEN_GROUPS token_groups,
> const WCHAR *group_nam
> break;
> }
> /* If a match is already found, ret == TRUE and the loop is skipped
> */
> -for (int i = 0; i < nread && !ret; ++i)
> +for (DWORD i = 0; i < nread && !ret; ++i)
... and here.
Consider me not convinced...
what exactly are the warnings MSVC spits out here?
gert
--
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de
signature.asc
Description: PGP signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved
Hi, > > index 7a2d0e3..5422d33 100644 > > --- a/src/openvpnserv/validate.c > > +++ b/src/openvpnserv/validate.c > > @@ -298,7 +298,7 @@ IsUserInGroup(PSID sid, const PTOKEN_GROUPS > token_groups, const WCHAR *group_nam > > break; > > } > > /* If a match is already found, ret == TRUE and the loop is > skipped */ > > -for (int i = 0; i < nread && !ret; ++i) > > +for (DWORD i = 0; i < nread && !ret; ++i) > > ... and here. > > > Consider me not convinced... > > what exactly are the warnings MSVC spits out here? > I think these are needed to silence signed/unsigned comparison warnings (== is ok, but < and > would warn). gcc also would warn if -Wextra or -Wsign-compare is used. Simon may have more to say. Selva -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved
On 05/11/17 20:20, Gert Doering wrote:
>> -modext(LPTSTR dest, int size, LPCTSTR src, LPCTSTR newext)
>> +modext(LPTSTR dest, size_t size, LPCTSTR src, LPCTSTR newext)
>> {
>> int i;
> Not sure why this is needed? The code verifies that size is ">0", so
> a signed variable is ok here.
I would label this more as "use size_t for size/length related
variables". So I read this more as a coding style change than anything
else.
From a general point of view, I can agree to such changes. But not
convinced that it is strictly needed everywhere. If it silences
compiler warnings and don't add "anything else" (behavioural changes or
warnings) elsewhere (both code and platform wise), then I'm fairly
positive to it.
--
kind regards,
David Sommerseth
OpenVPN, Inc
signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved
Hi,
Let me explain all proposed modifications case-by-case below...
> > diff --git a/src/openvpn/block_dns.c b/src/openvpn/block_dns.c index
> > d43cbcf..f88ba2c 100644
> > --- a/src/openvpn/block_dns.c
> > +++ b/src/openvpn/block_dns.c
> > @@ -370,7 +370,7 @@ get_interface_metric(const NET_IFINDEX index,
> const ADDRESS_FAMILY family)
> > }
> > return ipiface.Metric;
> > }
> > -return -err;
> > +return -(int)err;
> > }
>
> This, I can somewhat agree to, as "err" is an unsigned type so there might be
> a hidden integer overflow if it happens to be "large". Which it won't be, but
> still.
Adding the `(int)` resolves the warning C4146: unary minus operator applied to
unsigned type, result still unsigned.
https://msdn.microsoft.com/en-us/library/4kh09110.aspx
It doesn't change much, but it shut-ups one compiler warning.
> > /*
> > diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c
> > index 4123d0f..6c39aaa 100644
> > --- a/src/openvpnserv/automatic.c
> > +++ b/src/openvpnserv/automatic.c
> > @@ -155,7 +155,7 @@ match(const WIN32_FIND_DATA *find, LPCTSTR
> ext)
> > * Modify the extension on a filename.
> > */
> > static bool
> > -modext(LPTSTR dest, int size, LPCTSTR src, LPCTSTR newext)
> > +modext(LPTSTR dest, size_t size, LPCTSTR src, LPCTSTR newext)
> > {
> > int i;
>
> Not sure why this is needed? The code verifies that size is ">0", so a signed
> variable is ok here.
warning C4018: '<=': signed/unsigned mismatch (in Win32 builds twice)
https://msdn.microsoft.com/en-us/library/y92ktdf2.aspx
Parameter size is compared to the result of _tcslen() which is size_t. Hence,
you had a signed int and unsigned size_t comparison.
However, modext() is called exactly at one location in the code:
if (!modext(log_file, _countof(log_file), find_obj.cFileName,
TEXT("log")))
...
Observe, the size parameter is _countof(log_file) which is size_t, converted to
int to be passed as parameter to modext(), and finally compared against
_tcslen() which is size_t in modext(). In other words: it had an orange,
converted it to an apple, and finally compared it to another orange.
> > diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c
> index
> > b8b817b..7901fd6 100644
> > --- a/src/openvpnserv/common.c
> > +++ b/src/openvpnserv/common.c
> > @@ -36,7 +36,7 @@ openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR
> format, va_list arglist)
> > len = _vsntprintf(str, size, format, arglist);
> > str[size - 1] = 0;
> > }
> > -return (len >= 0 && len < size);
> > +return (len >= 0 && (size_t)len < size);
> > }
>
> This is, if I understand right, because "len < size" will implicitly cast
> size to
> (int), causing integer overflow if size is too big for a signed int?
warning C4018: '<': signed/unsigned mismatch (in Win32 builds).
The MSVC is not happy if it needs to decide between signed or unsigned
comparison itself. So the programmer needs to cast either side to match the
signed/unsigned manually.
The `len >= 0` assures that len is always positive or zero before `len < size`
is evaluated. Therefore, it is no harm to cast len to size_t to force unsigned
comparison.
> > int
> > openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...) diff
> > --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
> > index 8d94197..96e0de0 100644
> > --- a/src/openvpnserv/interactive.c
> > +++ b/src/openvpnserv/interactive.c
> > @@ -188,7 +188,7 @@ typedef enum {
> > static DWORD
> > AsyncPipeOp(async_op_t op, HANDLE pipe, LPVOID buffer, DWORD size,
> > DWORD count, LPHANDLE events) {
> > -int i;
> > +DWORD i;
>
> ... and this totally escapes me. Why would an "int" suddenly be no longer a
> legal loop variable...?
This one is my favourite. :) I hope I'll manage to explain it (I am not a
native English speaker. Please, be patient.) This one is not just about C4018
again. Although, the `i < count` in `for (i = 0; i < count; i++)` does raise
the C4018. That's how it got my attention.
This particular loop variable i was used in one loop only. In that particular
loop, it iterates on [0...count) interval. The lower bound is zero (any scalar
zero). The upper bound is a DWORD. Thus, the interval is essentially a
[DWORD...DWORD) type. So the most natural selection for the loop variable is
DWORD then. This avoids all 32/64-bit issues, signed/unsigned comparison
warnings etc.
When you need to iterate from 'A' to 'Z', the most natural selection for loop
variable is char; when you count oranges, the appropriate loop variable is
orange...
> > BOOL success;
> > HANDLE io_event;
> > DWORD res, bytes = 0;
> > @@ -1061,7 +1061,7 @@ RegisterDNS(LPVOID unused)
> > { ipcfg, L"ipconfig /flushdns",timeout },
> > { ipcfg, L"ipconfig /registerdns", timeout },
> > };
> > -int ncmds = sizeof(cmds) / sizeof(cmds
[Openvpn-devel] [PATCH v2] openvpnserv: Add support for multi-instances
While openvpn.exe can run multiple concurrent processes, openvpnserv.exe
is usually only one single globally unique running process.
This patch extends openvpnserv.exe to support multiple service instances
in parallel allowing side-by-side OpenVPN installations.
Alternate instances must be installed as `SERVICE_WIN32_OWN_PROCESS`
(Type 0x10) and must use the newly introduced service command line
parameter:
-instance
can be `automatic` or `interactive`.
- The service settings will be loaded from `HKLM\Software\` registry
key.
- The automatic service will use `_exit_1` exit event.
- The interactive service will accept requests on
`\\.\pipe\\service` named pipe, and run IPC with openvpn.exe on
`\\.\pipe\\service_`.
This patch preserves backward compatibility, by defaulting to
`SERVICE_WIN32_SHARE_PROCESS` and `OpenVPN` as service ID.
---
src/openvpnserv/automatic.c | 40 +++---
src/openvpnserv/common.c | 14 +---
src/openvpnserv/interactive.c | 18 +++---
src/openvpnserv/service.c | 77 ++-
src/openvpnserv/service.h | 4 ++-
5 files changed, 100 insertions(+), 53 deletions(-)
diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c
index 4123d0f..ff5c1a2 100644
--- a/src/openvpnserv/automatic.c
+++ b/src/openvpnserv/automatic.c
@@ -44,7 +44,7 @@
#define false 0
static SERVICE_STATUS_HANDLE service;
-static SERVICE_STATUS status;
+static SERVICE_STATUS status = { SERVICE_WIN32_SHARE_PROCESS };
openvpn_service_t automatic_service = {
automatic,
@@ -60,12 +60,6 @@ struct security_attributes
SECURITY_DESCRIPTOR sd;
};
-/*
- * Which registry key in HKLM should
- * we get config info from?
- */
-#define REG_KEY "SOFTWARE\\" PACKAGE_NAME
-
static HANDLE exit_event = NULL;
/* clear an object */
@@ -91,15 +85,6 @@ init_security_attributes_allow_all(struct
security_attributes *obj)
return true;
}
-/*
- * This event is initially created in the non-signaled
- * state. It will transition to the signaled state when
- * we have received a terminate signal from the Service
- * Control Manager which will cause an asynchronous call
- * of ServiceStop below.
- */
-#define EXIT_EVENT_NAME TEXT(PACKAGE "_exit_1")
-
HANDLE
create_event(LPCTSTR name, bool allow_all, bool initial_state, bool
manual_reset)
{
@@ -212,10 +197,19 @@ ServiceCtrlAutomatic(DWORD ctrl_code, DWORD event, LPVOID
data, LPVOID ctx)
VOID WINAPI
+ServiceStartAutomaticOwn(DWORD dwArgc, LPTSTR *lpszArgv)
+{
+status.dwServiceType = SERVICE_WIN32_OWN_PROCESS;
+ServiceStartAutomatic(dwArgc, lpszArgv);
+}
+
+
+VOID WINAPI
ServiceStartAutomatic(DWORD dwArgc, LPTSTR *lpszArgv)
{
DWORD error = NO_ERROR;
settings_t settings;
+TCHAR event_name[256];
service = RegisterServiceCtrlHandlerEx(automatic_service.name,
ServiceCtrlAutomatic, &status);
if (!service)
@@ -223,7 +217,6 @@ ServiceStartAutomatic(DWORD dwArgc, LPTSTR *lpszArgv)
return;
}
-status.dwServiceType = SERVICE_WIN32_SHARE_PROCESS;
status.dwCurrentState = SERVICE_START_PENDING;
status.dwServiceSpecificExitCode = NO_ERROR;
status.dwWin32ExitCode = NO_ERROR;
@@ -237,8 +230,15 @@ ServiceStartAutomatic(DWORD dwArgc, LPTSTR *lpszArgv)
/*
* Create our exit event
+ * This event is initially created in the non-signaled
+ * state. It will transition to the signaled state when
+ * we have received a terminate signal from the Service
+ * Control Manager which will cause an asynchronous call
+ * of ServiceStop below.
*/
-exit_event = create_event(EXIT_EVENT_NAME, false, false, true);
+
+openvpn_sntprintf(event_name, _countof(event_name), TEXT("%s_exit_1"),
service_instance);
+exit_event = create_event(event_name, false, false, true);
if (!exit_event)
{
MsgToEventLog(M_ERR, TEXT("CreateEvent failed"));
@@ -327,8 +327,8 @@ ServiceStartAutomatic(DWORD dwArgc, LPTSTR *lpszArgv)
TEXT("%s\\%s"), settings.log_dir, log_file);
/* construct command line */
-openvpn_sntprintf(command_line, _countof(command_line),
TEXT(PACKAGE " --service %s 1 --config \"%s\""),
- EXIT_EVENT_NAME,
+openvpn_sntprintf(command_line, _countof(command_line),
TEXT("openvpn --service \"%s_exit_1\" 1 --config \"%s\""),
+ service_instance,
find_obj.cFileName);
/* Make security attributes struct for logfile handle so it can
diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c
index e77d7ab..dd5d058 100644
--- a/src/openvpnserv/common.c
+++ b/src/openvpnserv/common.c
@@ -24,6 +24,9 @@
#include "service.h"
#include "validate.h"
+LPCTSTR service_instance = TEXT(PACKAGE_NAME);
+
+
/*
* These are necessary due to certain buggy implementations of (v)snpr
[Openvpn-devel] [PATCH v2] Use lowest metric interface when multiple interfaces match a route
From: Selva Nair
Currently a route addition using IPAPI or service is skipped if the
route gateway is reachable by multiple interfaces. This changes that
to use the interface with lowest metric. Implemented by
(i) Do not over-write the return value with TUN_ADAPTER_INDEX_INVALID in
windows_route_find_if_index() if multiple interfaces match a route.
(ii) Select the interface with lowest metric in adapter_index_of_ip()
instead of the first one found when multiple interfaces match.
Reported by Jan Just Keijser
v2: - A private get_interface_metric() method and better error reporting
- Revert an unintented edit of route.c (a_index = ...)
- Improve the commit message
Signed-off-by: Selva Nair
---
src/openvpn/route.c | 1 -
src/openvpn/tun.c | 59 +++--
2 files changed, 57 insertions(+), 3 deletions(-)
diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index 8c71e6e..66a8ae3 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -2780,7 +2780,6 @@ windows_route_find_if_index(const struct route_ipv4 *r,
const struct tuntap *tt)
msg(M_WARN, "Warning: route gateway is ambiguous: %s (%d matches)",
print_in_addr_t(r->gateway, 0, &gc),
count);
-ret = TUN_ADAPTER_INDEX_INVALID;
}
dmsg(D_ROUTE_DEBUG, "DEBUG: route find if: on_tun=%d count=%d index=%d",
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 3639718..7603133 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -4474,6 +4474,49 @@ is_ip_in_adapter_subnet(const IP_ADAPTER_INFO *ai, const
in_addr_t ip, in_addr_t
return ret;
}
+/**
+ * Given an interface index return the interface metric.
+ *
+ * Arguments:
+ * index : The index of the interface
+ * family: AF_INET for IPv4 or AF_INET6 for IPv6
+ * On error returns -1
+ */
+
+/* function signature missing in mingw iphlpapi.h */
+VOID NETIOAPI_API_
+InitializeIpInterfaceEntry(PMIB_IPINTERFACE_ROW Row);
+
+static int
+get_interface_metric(NET_IFINDEX index, ADDRESS_FAMILY family)
+{
+DWORD err;
+int msglevel = D_ROUTE|M_WARN;
+MIB_IPINTERFACE_ROW ipiface;
+
+InitializeIpInterfaceEntry(&ipiface);
+ipiface.Family = family;
+ipiface.InterfaceIndex = index;
+
+err = GetIpInterfaceEntry(&ipiface);
+if (err == NO_ERROR)
+{
+return ipiface.Metric;
+}
+else if (err == ERROR_NOT_FOUND)
+{
+/*
+ * This happens if the address family is not enabled for the
+ * interface, which is benign -- display only at a debug level
+ */
+msglevel = D_ROUTE_DEBUG;
+}
+msg(msglevel, "Note: failed to determine metric of interface "
+ "<%lu> for %s : (error code = %lu)",
+ index, (family == AF_INET)? "ipv4" : "ipv6", err);
+return -1;
+}
+
DWORD
adapter_index_of_ip(const IP_ADAPTER_INFO *list,
const in_addr_t ip,
@@ -4483,6 +4526,7 @@ adapter_index_of_ip(const IP_ADAPTER_INFO *list,
struct gc_arena gc = gc_new();
DWORD ret = TUN_ADAPTER_INDEX_INVALID;
in_addr_t highest_netmask = 0;
+int lowest_metric = INT_MAX;
bool first = true;
if (count)
@@ -4496,9 +4540,14 @@ adapter_index_of_ip(const IP_ADAPTER_INFO *list,
if (is_ip_in_adapter_subnet(list, ip, &hn))
{
+int metric = get_interface_metric(list->Index, AF_INET);
if (first || hn > highest_netmask)
{
highest_netmask = hn;
+if (metric >= 0)
+{
+lowest_metric = metric;
+}
if (count)
{
*count = 1;
@@ -4512,16 +4561,22 @@ adapter_index_of_ip(const IP_ADAPTER_INFO *list,
{
++*count;
}
+if (metric >= 0 && metric < lowest_metric)
+{
+ret = list->Index;
+lowest_metric = metric;
+}
}
}
list = list->Next;
}
-dmsg(D_ROUTE_DEBUG, "DEBUG: IP Locate: ip=%s nm=%s index=%d count=%d",
+dmsg(D_ROUTE_DEBUG, "DEBUG: IP Locate: ip=%s nm=%s index=%d count=%d
metric=%d",
print_in_addr_t(ip, 0, &gc),
print_in_addr_t(highest_netmask, 0, &gc),
(int)ret,
- count ? *count : -1);
+ count ? *count : -1,
+ lowest_metric);
if (ret == TUN_ADAPTER_INDEX_INVALID && count)
{
--
2.1.4
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved
Hi, I understand the intent here and its nice to get rid of compiler warnings, when it makes sense. But be careful while silencing by just a cast. On Sun, Nov 5, 2017 at 5:06 PM, Simon Rozman wrote: > > Hi, > > Let me explain all proposed modifications case-by-case below... > > > > diff --git a/src/openvpn/block_dns.c b/src/openvpn/block_dns.c index > > > d43cbcf..f88ba2c 100644 > > > --- a/src/openvpn/block_dns.c > > > +++ b/src/openvpn/block_dns.c > > > @@ -370,7 +370,7 @@ get_interface_metric(const NET_IFINDEX index, > > const ADDRESS_FAMILY family) > > > } > > > return ipiface.Metric; > > > } > > > -return -err; > > > +return -(int)err; > > > } > > > > This, I can somewhat agree to, as "err" is an unsigned type so there might be > > a hidden integer overflow if it happens to be "large". Which it won't be, but > > still. > > Adding the `(int)` resolves the warning C4146: unary minus operator > applied to unsigned type, result still unsigned. > https://msdn.microsoft.com/en-us/library/4kh09110.aspx > > It doesn't change much, but it shut-ups one compiler warning. The original code could be argued to be logically wrong (though perfectly legal C) if err becomes larger than a signed int. This change does not fix that. So what's the point? As an example, consider this err = 2147483649 <(214)%20748-3649> (2 more than INT_MAX) Original will return 2147483647 <(214)%20748-3647> (not a -ve int) The new code will return the same 2147483647 <(214)%20748-3647> That is, the return value is +ve int which the caller will wrongly interpret as valid result. This is hypothetical as Windows system err codes do not get that large. But then the original is as good as the replacement except for a C++-trained compiler being silenced. While most of the MSVC compiler warnings are false-alarms, one out of many > does represent a potential threat. But you don't know which one that is. > Therefore it is best to address them all. :) > To repeat, cast does not eliminate a potential for error, it just just hides it. Selva -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] LZ4 library version printing
> Hi, > > returning to this patchset about printing the LZ4 library version... > > On Sat, Sep 23, 2017 at 10:52:16AM +0200, Simon Matter wrote: >> > On Thu, Sep 07, 2017 at 10:40:21PM +0200, Simon Matter wrote: >> >> While we are at it, I found it useful to see the used LZ4 version at >> >> runtime as it is done with LZO and other libraries. > > I still like this :-) > > [..] >> > Feature-ACK (I think this is a useful idea, for the same reasons we >> have >> > LZO and OpenSSL versions :-) ), but that will not work if we use >> > compat-lz4.h, so "code NAK". >> >> IIRC at the time of writing this, the embedded lz4 was too old and >> didn't >> have the LZ4_versionString() function. So the code won't work with the >> patch. If the embedded code has LZ4_versionString() now, it should be >> ok. > > It now has... (1.7.5). > > I'm not sure where LZ4_versionString() got introduced, though - their > git repo does not have tags for 1.7.1 or 1.7.2, it is in the 1.7.3 tag, > but not in the "last of the old numbering scheme" version. > > Our configure.ac requires 1.7.1 - so if LZ4_versionString() is not in > there yet, we might want to bump to 1.7.3 then. Hi Gert, according to the NEWS file in lz4 there is no such thing as release 1.7.1 or 1.7.2, it goes from r131 -> v1.7.3. I therefore suggest to set the minimum version to 1.7.3 as I'm also unable to locate any official 1.7.1 or 1.7.2 release. BTW, for 2.4.4 I've changed to patch to the one attached. Regards, Simon--- openvpn-2.4.4/src/openvpn/options.c.orig 2017-09-26 11:27:37.0 +0200 +++ openvpn-2.4.4/src/openvpn/options.c 2017-09-28 06:21:57.977919621 +0200 @@ -61,6 +61,10 @@ #include "memdbg.h" +#if defined(ENABLE_LZ4) +#include "lz4.h" +#endif + const char title_string[] = PACKAGE_STRING #ifdef CONFIGURE_GIT_REVISION @@ -4155,11 +4159,17 @@ #else #define LZO_LIB_VER_STR "", "" #endif +#ifdef ENABLE_LZ4 +#define LZ4_LIB_VER_STR ", LZ4 ", LZ4_versionString() +#else +#define LZ4_LIB_VER_STR "", "" +#endif -msg(flags, "library versions: %s%s%s", SSL_LIB_VER_STR, LZO_LIB_VER_STR); +msg(flags, "library versions: %s%s%s%s%s", SSL_LIB_VER_STR, LZO_LIB_VER_STR, LZ4_LIB_VER_STR); #undef SSL_LIB_VER_STR #undef LZO_LIB_VER_STR +#undef LZ4_LIB_VER_STR } static void-- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
