[Openvpn-devel] Fix format spec errors in Windows builds

Message ID 1519071994-20797-1-git-send-email-selva.nair@gmail.com
State Superseded
Delegated to: Gert Doering
Headers show
Series [Openvpn-devel] Fix format spec errors in Windows builds | expand

Commit Message

Selva Nair Feb. 19, 2018, 9:26 a.m. UTC
From: Selva Nair <selva.nair@gmail.com>

- "%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 <selva.nair@gmail.com>
---
 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(-)

Comments

Gert Doering Feb. 19, 2018, 9:20 p.m. UTC | #1
Hi,

On Mon, Feb 19, 2018 at 03:26:34PM -0500, selva.nair@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
Selva Nair Feb. 20, 2018, 4:42 a.m. UTC | #2
Hi,

On Tue, Feb 20, 2018 at 3:20 AM, Gert Doering <gert@greenie.muc.de> wrote:
> Hi,
>
> On Mon, Feb 19, 2018 at 03:26:34PM -0500, selva.nair@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

Patch

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->context.c2.fragment->outgoing.len : -1,
-               (long long)mi->context.c2.timeval.tv_sec,
+               (int64_t)mi->context.c2.timeval.tv_sec,
                (long)mi->context.c2.timeval.tv_usec);
 #endif
     }
diff --git a/src/openvpn/otime.c b/src/openvpn/otime.c
index d741ae0..048e772 100644
--- a/src/openvpn/otime.c
+++ b/src/openvpn/otime.c
@@ -88,8 +88,8 @@  const char *
 tv_string(const struct timeval *tv, struct gc_arena *gc)
 {
     struct buffer out = alloc_buf_gc(64, gc);
-    buf_printf(&out, "[%lld/%ld]",
-               (long long)tv->tv_sec,
+    buf_printf(&out, "[%"PRIi64"/%ld]",
+               (int64_t)tv->tv_sec,
                (long)tv->tv_usec);
     return BSTR(&out);
 }
@@ -198,9 +198,9 @@  time_test(void)
         t = time(NULL);
         gettimeofday(&tv, NULL);
 #if 1
-        msg(M_INFO, "t=%lld s=%lld us=%ld",
-            (long long)t,
-            (long long)tv.tv_sec,
+        msg(M_INFO, "t=%"PRIi64" s=%"PRIi64" us=%ld",
+            (int64_t)t,
+            (int64_t)tv.tv_sec,
             (long)tv.tv_usec);
 #endif
     }
diff --git a/src/openvpn/packet_id.c b/src/openvpn/packet_id.c
index 4c3696d..1560fdc 100644
--- a/src/openvpn/packet_id.c
+++ b/src/openvpn/packet_id.c
@@ -606,14 +606,14 @@  packet_id_debug_print(int msglevel,
         }
         buf_printf(&out, "%c", c);
     }
-    buf_printf(&out, "] %lld:" packet_id_format, (long long)p->time, (packet_id_print_type)p->id);
+    buf_printf(&out, "] %"PRIi64":" packet_id_format, (int64_t)p->time, (packet_id_print_type)p->id);
     if (pin)
     {
-        buf_printf(&out, " %lld:" packet_id_format, (long long)pin->time, (packet_id_print_type)pin->id);
+        buf_printf(&out, " %"PRIi64":" packet_id_format, (int64_t)pin->time, (packet_id_print_type)pin->id);
     }
 
-    buf_printf(&out, " t=%lld[%d]",
-               (long long)prev_now,
+    buf_printf(&out, " t=%"PRIi64"[%d]",
+               (int64_t)prev_now,
                (int)(prev_now - tv.tv_sec));
 
     buf_printf(&out, " r=[%d,%d,%d,%d,%d]",
@@ -666,8 +666,8 @@  packet_id_interactive_test(void)
         {
             packet_id_reap_test(&pid.rec);
             test = packet_id_test(&pid.rec, &pin);
-            printf("packet_id_test (%lld, " packet_id_format ") returned %d\n",
-                   (long long)pin.time,
+            printf("packet_id_test (%"PRIi64", " packet_id_format ") returned %d\n",
+                   (int64_t)pin.time,
                    (packet_id_print_type)pin.id,
                    test);
             if (test)
@@ -679,8 +679,8 @@  packet_id_interactive_test(void)
         {
             long_form = (count < 20);
             packet_id_alloc_outgoing(&pid.send, &pin, long_form);
-            printf("(%lld(" packet_id_format "), %d)\n",
-                   (long long)pin.time,
+            printf("(%"PRIi64"(" packet_id_format "), %d)\n",
+                   (int64_t)pin.time,
                    (packet_id_print_type)pin.id,
                    long_form);
             if (pid.send.id == 10)
diff --git a/src/openvpn/reliable.c b/src/openvpn/reliable.c
index 972af61..453fc9b 100644
--- a/src/openvpn/reliable.c
+++ b/src/openvpn/reliable.c
@@ -786,14 +786,14 @@  reliable_debug_print(const struct reliable *rel, char *desc)
     printf("********* struct reliable %s\n", desc);
     printf("  initial_timeout=%d\n", (int)rel->initial_timeout);
     printf("  packet_id=" packet_id_format "\n", rel->packet_id);
-    printf("  now=%lld\n", (long long)now);
+    printf("  now=%"PRIi64"\n", (int64_t)now);
     for (i = 0; i < rel->size; ++i)
     {
         const struct reliable_entry *e = &rel->array[i];
         if (e->active)
         {
             printf("  %d: packet_id=" packet_id_format " len=%d", i, e->packet_id, e->buf.len);
-            printf(" next_try=%lld", (long long)e->next_try);
+            printf(" next_try=%"PRIi64, (int64_t)e->next_try);
             printf("\n");
         }
     }
diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index fe541b7..09daea3 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -1969,12 +1969,12 @@  add_route_ipv6(struct route_ipv6 *r6, const struct tuntap *tt, unsigned int flag
         struct buffer out = alloc_buf_gc(64, &gc);
         if (r6->adapter_index)          /* vpn server special route */
         {
-            buf_printf(&out, "interface=%d", r6->adapter_index );
+            buf_printf(&out, "interface=%u", (unsigned int)r6->adapter_index );
             gateway_needed = true;
         }
         else
         {
-            buf_printf(&out, "interface=%d", tt->adapter_index );
+            buf_printf(&out, "interface=%u", (unsigned int)tt->adapter_index );
         }
         device = buf_bptr(&out);
 
@@ -2416,12 +2416,12 @@  delete_route_ipv6(const struct route_ipv6 *r6, const struct tuntap *tt, unsigned
         struct buffer out = alloc_buf_gc(64, &gc);
         if (r6->adapter_index)          /* vpn server special route */
         {
-            buf_printf(&out, "interface=%d", r6->adapter_index );
+            buf_printf(&out, "interface=%u", (unsigned int)r6->adapter_index );
             gateway_needed = true;
         }
         else
         {
-            buf_printf(&out, "interface=%d", tt->adapter_index );
+            buf_printf(&out, "interface=%u", (unsigned int)tt->adapter_index );
         }
         device = buf_bptr(&out);
 
@@ -2843,7 +2843,7 @@  get_default_gateway_ipv6(struct route_ipv6_gateway_info *rgi6,
     }
 
     msg( D_ROUTE, "GDG6: II=%d DP=%s/%d NH=%s",
-         BestRoute.InterfaceIndex,
+         (int)BestRoute.InterfaceIndex,
          print_in6_addr( BestRoute.DestinationPrefix.Prefix.Ipv6.sin6_addr, 0, &gc),
          BestRoute.DestinationPrefix.PrefixLength,
          print_in6_addr( BestRoute.NextHop.Ipv6.sin6_addr, 0, &gc) );
@@ -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;
diff --git a/src/openvpn/shaper.c b/src/openvpn/shaper.c
index de2da6d..344a192 100644
--- a/src/openvpn/shaper.c
+++ b/src/openvpn/shaper.c
@@ -76,8 +76,8 @@  shaper_soonest_event(struct timeval *tv, int delay)
         }
     }
 #ifdef SHAPER_DEBUG
-    dmsg(D_SHAPER_DEBUG, "SHAPER shaper_soonest_event sec=%lld usec=%ld ret=%d",
-         (long long)tv->tv_sec, (long)tv->tv_usec, (int)ret);
+    dmsg(D_SHAPER_DEBUG, "SHAPER shaper_soonest_event sec=%"PRIi64" usec=%ld ret=%d",
+         (int64_t)tv->tv_sec, (long)tv->tv_usec, (int)ret);
 #endif
     return ret;
 }
diff --git a/src/openvpn/shaper.h b/src/openvpn/shaper.h
index 34f316f..862b927 100644
--- a/src/openvpn/shaper.h
+++ b/src/openvpn/shaper.h
@@ -147,10 +147,10 @@  shaper_wrote_bytes(struct shaper *s, int nbytes)
         tv_add(&s->wakeup, &tv);
 
 #ifdef SHAPER_DEBUG
-        dmsg(D_SHAPER_DEBUG, "SHAPER shaper_wrote_bytes bytes=%d delay=%ld sec=%lld usec=%ld",
+        dmsg(D_SHAPER_DEBUG, "SHAPER shaper_wrote_bytes bytes=%d delay=%ld sec=%"PRIi64" usec=%ld",
              nbytes,
              (long)tv.tv_usec,
-             (long long)s->wakeup.tv_sec,
+             (int64_t)s->wakeup.tv_sec,
              (long)s->wakeup.tv_usec);
 #endif
     }
diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index 0fc91f2..38175ed 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -1122,7 +1122,7 @@  socket_do_accept(socket_descriptor_t sd,
 
     if (!socket_defined(new_sd))
     {
-        msg(D_LINK_ERRORS | M_ERRNO, "TCP: accept(%d) failed", sd);
+        msg(D_LINK_ERRORS | M_ERRNO, "TCP: accept(%d) failed", (int)sd);
     }
     /* only valid if we have remote_len_af!=0 */
     else if (remote_len_af && remote_len != remote_len_af)
@@ -1875,12 +1875,12 @@  phase2_inetd(struct link_socket *sock, const struct frame *frame,
                 sock->info.lsa->actual.dest.addr.sa.sa_family = local_addr.addr.sa.sa_family;
                 dmsg(D_SOCKET_DEBUG, "inetd(%s): using sa_family=%d from getsockname(%d)",
                      proto2ascii(sock->info.proto, sock->info.af, false),
-                     local_addr.addr.sa.sa_family, sock->sd);
+                     local_addr.addr.sa.sa_family, (int)sock->sd);
             }
             else
             {
                 msg(M_WARN, "inetd(%s): getsockname(%d) failed, using AF_INET",
-                    proto2ascii(sock->info.proto, sock->info.af, false), sock->sd);
+                    proto2ascii(sock->info.proto, sock->info.af, false), (int)sock->sd);
             }
         }
 #else  /* ifdef HAVE_GETSOCKNAME */
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 242b464..5bbe365 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -1448,8 +1448,8 @@  bio_debug_data(const char *mode, BIO *bio, const uint8_t *buf, int len, const ch
     if (len > 0)
     {
         open_biofp();
-        fprintf(biofp, "BIO_%s %s time=%lld bio=" ptr_format " len=%d data=%s\n",
-                mode, desc, (long long)time(NULL), (ptr_type)bio, len, format_hex(buf, len, 0, &gc));
+        fprintf(biofp, "BIO_%s %s time=%"PRIi64" bio=" ptr_format " len=%d data=%s\n",
+                mode, desc, (int64_t)time(NULL), (ptr_type)bio, len, format_hex(buf, len, 0, &gc));
         fflush(biofp);
     }
     gc_free(&gc);
@@ -1459,8 +1459,8 @@  static void
 bio_debug_oc(const char *mode, BIO *bio)
 {
     open_biofp();
-    fprintf(biofp, "BIO %s time=%lld bio=" ptr_format "\n",
-            mode, (long long)time(NULL), (ptr_type)bio);
+    fprintf(biofp, "BIO %s time=%"PRIi64" bio=" ptr_format "\n",
+            mode, (int64_t)time(NULL), (ptr_type)bio);
     fflush(biofp);
 }
 
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 2644d99..0d14f14 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -124,7 +124,7 @@  do_address_service(const bool add, const short family, const struct tuntap *tt)
 
     if (ack.error_number != NO_ERROR)
     {
-        msg(M_WARN, "TUN: %s address failed using service: %s [status=%u if_index=%lu]",
+        msg(M_WARN, "TUN: %s address failed using service: %s [status=%u if_index=%d]",
             (add ? "adding" : "deleting"), strerror_win32(ack.error_number, &gc),
             ack.error_number, addr.iface.index);
         goto out;
@@ -3790,7 +3790,7 @@  get_panel_reg(struct gc_arena *gc)
 
             if (status != ERROR_SUCCESS || name_type != REG_SZ)
             {
-                dmsg(D_REGISTRY, "Error opening registry key: %s\\%s\\%s",
+                dmsg(D_REGISTRY, "Error opening registry key: %s\\%s\\%ls",
                      NETWORK_CONNECTIONS_KEY, connection_string, name_string);
             }
             else