Re: [Openvpn-devel] [PATCH] Fix format spec errors in Windows builds
Hi,
On Tue, Feb 20, 2018 at 3:20 AM, Gert Doering wrote:
> Hi,
>
> On Mon, Feb 19, 2018 at 03:26:34PM -0500, selva.n...@gmail.com wrote:
>> - In route.c print adapter_index as unsigned int as in the rest
>> of the code.
>
> That one confuses me, but that is most likely me vs. windows types.
>
> adapter_index is declared as "DWORD" in tun.h, and
>
> https://msdn.microsoft.com/en-us/library/cc230318.aspx
>
> says that DWORD is "typedef unsigned long DWORD".
>
> Since that particular code is only relevant on windows, why not just
> use "%lu" and avoid the cast?
You are absolutely right. But, we have several places where DWORD is
cast to (unsigned int) before printing, so I just decided to follow
that "style".
Anyway, don't want to change all those cases, but will change all
instances of adapter_index to use %lu (only ~3 locations other than
the ones touched here) and send a v2.
>
>
> The rest looks reasonable to me, and I'm not strictly opposed to the
> adapter_index one, just confused.
>
> (As a side note, I assume that this patch is "master only", and have
> not looked at which bits and pieces apply to release/2.4 - the error.c
> one does not, because the surrounding code is different, plus, I've
> already merged Steffan's patch for error.c to 2.4)
Oh, yes, a separate 2.4 version may be needed. Will check.
>
> [..]
>> @@ -3003,7 +3003,7 @@ do_route_service(const bool add, const route_message_t
>> *rt, const size_t size, H
>>
>> if (ack.error_number != NO_ERROR)
>> {
>> -msg(M_WARN, "ROUTE: route %s failed using service: %s [status=%u
>> if_index=%lu]",
>> +msg(M_WARN, "ROUTE: route %s failed using service: %s [status=%u
>> if_index=%d]",
>> (add ? "addition" : "deletion"),
>> strerror_win32(ack.error_number, &gc),
>> ack.error_number, rt->iface.index);
>> goto out;
>
> That one is correct, but arguably a bit annoying - openvpn-msg.h /
> struct interface_t uses "int index", which maybe should have been a
> DWORD to keep the windows types. Or not?
>
> I wouldn't change the message types, though - as it would introduce
> possible compatibility issues between openvpn and iservice (if one
> ends up having different versions installed and running).
This is something we better live with. That said even if we change the
msg types to use DWORD, binary compatibility with older versions would
not break as sizeof(DWORD) is 32 bits, same as int, isn't it? For the
same reason no real need to change this as no information is really
lost.
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] Fix format spec errors in Windows builds
Hi,
On Mon, Feb 19, 2018 at 03:26:34PM -0500, selva.n...@gmail.com wrote:
> - In route.c print adapter_index as unsigned int as in the rest
> of the code.
That one confuses me, but that is most likely me vs. windows types.
adapter_index is declared as "DWORD" in tun.h, and
https://msdn.microsoft.com/en-us/library/cc230318.aspx
says that DWORD is "typedef unsigned long DWORD".
Since that particular code is only relevant on windows, why not just
use "%lu" and avoid the cast?
The rest looks reasonable to me, and I'm not strictly opposed to the
adapter_index one, just confused.
(As a side note, I assume that this patch is "master only", and have
not looked at which bits and pieces apply to release/2.4 - the error.c
one does not, because the surrounding code is different, plus, I've
already merged Steffan's patch for error.c to 2.4)
[..]
> @@ -3003,7 +3003,7 @@ do_route_service(const bool add, const route_message_t
> *rt, const size_t size, H
>
> if (ack.error_number != NO_ERROR)
> {
> -msg(M_WARN, "ROUTE: route %s failed using service: %s [status=%u
> if_index=%lu]",
> +msg(M_WARN, "ROUTE: route %s failed using service: %s [status=%u
> if_index=%d]",
> (add ? "addition" : "deletion"),
> strerror_win32(ack.error_number, &gc),
> ack.error_number, rt->iface.index);
> goto out;
That one is correct, but arguably a bit annoying - openvpn-msg.h /
struct interface_t uses "int index", which maybe should have been a
DWORD to keep the windows types. Or not?
I wouldn't change the message types, though - as it would introduce
possible compatibility issues between openvpn and iservice (if one
ends up having different versions installed and running).
gert
--
now what should I write here...
Gert Doering - Munich, Germany g...@greenie.muc.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
[Openvpn-devel] [PATCH] Fix format spec errors in Windows builds
From: Selva Nair
- "%ll" is not supported by Windows run time, so use PRIi64
and cast the variable to (int64_t) in output statements
(as suggested by Steffan in patch # 234:
https://patchwork.openvpn.net/patch/234/)
- Fix an instance of wchar_t * printed using %s -- should be %ls.
- Cast variables to int or unsigned int to match the output
format spec when necessary.
- In route.c print adapter_index as unsigned int as in the rest
of the code.
Most of these errors are seen in current Windows cross-compile,
but a few are triggered only if some DEBUG options are enabled.
Some are not in Windows specific paths. But for consistency, all uses
of %llu/%lld are removed. As these only affect log output, there are
no potential side effects.
Replacing long long by int64_t also has the advantage of avoiding
size ambiguity as long long is not guaranteed to be 64 bytes.
Signed-off-by: Selva Nair
---
src/openvpn/error.c | 4 ++--
src/openvpn/event.c | 8
src/openvpn/forward.c | 2 +-
src/openvpn/misc.c| 2 +-
src/openvpn/multi.c | 4 ++--
src/openvpn/otime.c | 10 +-
src/openvpn/packet_id.c | 16
src/openvpn/reliable.c| 4 ++--
src/openvpn/route.c | 12 ++--
src/openvpn/shaper.c | 4 ++--
src/openvpn/shaper.h | 4 ++--
src/openvpn/socket.c | 6 +++---
src/openvpn/ssl_openssl.c | 8
src/openvpn/tun.c | 4 ++--
14 files changed, 44 insertions(+), 44 deletions(-)
diff --git a/src/openvpn/error.c b/src/openvpn/error.c
index 2645545..ba4e936 100644
--- a/src/openvpn/error.c
+++ b/src/openvpn/error.c
@@ -342,8 +342,8 @@ x_msg_va(const unsigned int flags, const char *format,
va_list arglist)
struct timeval tv;
gettimeofday(&tv, NULL);
-fprintf(fp, "%lld.%06ld %x %s%s%s%s",
-(long long)tv.tv_sec,
+fprintf(fp, "%"PRIi64".%06ld %x %s%s%s%s",
+(int64_t)tv.tv_sec,
(long)tv.tv_usec,
flags,
prefix,
diff --git a/src/openvpn/event.c b/src/openvpn/event.c
index 2fb112b..4178d2a 100644
--- a/src/openvpn/event.c
+++ b/src/openvpn/event.c
@@ -1041,9 +1041,9 @@ se_wait_fast(struct event_set *es, const struct timeval
*tv, struct event_set_re
struct timeval tv_tmp = *tv;
int stat;
-dmsg(D_EVENT_WAIT, "SE_WAIT_FAST maxfd=%d tv=%lld/%ld",
+dmsg(D_EVENT_WAIT, "SE_WAIT_FAST maxfd=%d tv=%"PRIi64"/%ld",
ses->maxfd,
- (long long)tv_tmp.tv_sec,
+ (int64_t)tv_tmp.tv_sec,
(long)tv_tmp.tv_usec);
stat = select(ses->maxfd + 1, &ses->readfds, &ses->writefds, NULL,
&tv_tmp);
@@ -1065,8 +1065,8 @@ se_wait_scalable(struct event_set *es, const struct
timeval *tv, struct event_se
fd_set write = ses->writefds;
int stat;
-dmsg(D_EVENT_WAIT, "SE_WAIT_SCALEABLE maxfd=%d tv=%lld/%ld",
- ses->maxfd, (long long)tv_tmp.tv_sec, (long)tv_tmp.tv_usec);
+dmsg(D_EVENT_WAIT, "SE_WAIT_SCALEABLE maxfd=%d tv=%"PRIi64"/%ld",
+ ses->maxfd, (int64_t)tv_tmp.tv_sec, (long)tv_tmp.tv_usec);
stat = select(ses->maxfd + 1, &read, &write, NULL, &tv_tmp);
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 9bf9483..af09be0 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -610,7 +610,7 @@ check_coarse_timers_dowork(struct context *c)
process_coarse_timers(c);
c->c2.coarse_timer_wakeup = now + c->c2.timeval.tv_sec;
-dmsg(D_INTERVAL, "TIMER: coarse timer wakeup %lld seconds", (long
long)c->c2.timeval.tv_sec);
+dmsg(D_INTERVAL, "TIMER: coarse timer wakeup %"PRIi64" seconds",
(int64_t)c->c2.timeval.tv_sec);
/* Is the coarse timeout NOT the earliest one? */
if (c->c2.timeval.tv_sec > save.tv_sec)
diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 76b592f..0ad8c3c 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -556,7 +556,7 @@ void
setenv_long_long(struct env_set *es, const char *name, long long value)
{
char buf[64];
-openvpn_snprintf(buf, sizeof(buf), "%lld", value);
+openvpn_snprintf(buf, sizeof(buf), "%"PRIi64, (int64_t)value);
setenv_str(es, name, buf);
}
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 25b2d09..ca5b89d 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2383,13 +2383,13 @@ multi_process_post(struct multi_context *m, struct
multi_instance *mi, const uns
multi_set_pending(m, ANY_OUT(&mi->context) ? mi : NULL);
#ifdef MULTI_DEBUG_EVENT_LOOP
-printf("POST %s[%d] to=%d lo=%d/%d w=%lld/%ld\n",
+printf("POST %s[%d] to=%d lo=%d/%d w=%"PRIi64"/%ld\n",
id(mi),
(int) (mi == m->pending),
mi ? mi->context.c2.to_tun.len : -1,
mi ? mi->context.c2.to_link.len : -1,
(mi && mi->context.c2.fragment) ?
mi-
