Skip to contentSkip to conversation

Don't deference type-punned pointers

PATCH In review Steffan Karger 4 participants 17 messages Sep 28, 2016 – Nov 13, 2016

lore
Share & tools
lore

Steffan Karger

author
#0 lore raw

PATCH 1/1 Don't deference type-punned pointers

Fixes compiler warnings (undefined behavior) by making the copy explicit to comply to strict aliasing rules. With newer GCC the old code could actually lead to undefined behaviour.

See e.g. http://blog.regehr.org/archives/959.

Signed-off-by Steffan Karger <[email protected]...>
---
 src/openvpn/mroute.h | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
src/openvpn/mroute.h +8 -4
index 608f70b..1296285 100644
@@ -189,16 +189,20 @@ mroute_extract_in_addr_t (struct mroute_addr *dest, const in_addr_t src)
   dest->type = MR_ADDR_IPV4;
   dest->netbits = 0;
   dest->len = 4;
-  *(in_addr_t*)dest->addr = htonl (src);
+  in_addr_t tmp_addr = htonl (src);
+  memcpy(dest->addr, &tmp_addr, sizeof(uint32_t));
 }
 
 static inline in_addr_t
 in_addr_t_from_mroute_addr (const struct mroute_addr *addr)
 {
-  if ((addr->type & MR_ADDR_MASK) == MR_ADDR_IPV4 && addr->netbits == 0 && addr->len == 4)
-    return ntohl(*(in_addr_t*)addr->addr);
-  else
+  if ((addr->type & MR_ADDR_MASK) == MR_ADDR_IPV4 && addr->netbits == 0 && addr->len == 4) {
+    in_addr_t tmp = 0;
+    memcpy(&tmp, addr->addr, sizeof(tmp));
+    return ntohl(tmp);
+  } else {
     return 0;
+  }
 }
 
 static inline void
Signature · 1 line
2.7.4
#1 lore raw

Am 28.09.16 um 13:08 schrieb Steffan Karger:

Quoted 4 lines

Fixes compiler warnings (undefined behavior) by making the copy explicit to comply to strict aliasing rules. With newer GCC the old code could actually lead to undefined behaviour.

I always considered this to be legal C. But if the spec says otherwise, we should change this. The code does the same thing. So reluctant ACK.

Arne

#2 lore raw
In reply to Steffan Karger
Quoted 5 lines · Steffan Karger

Fixes compiler warnings (undefined behavior) by making the copy explicit to comply to strict aliasing rules. With newer GCC the old code could actually lead to undefined behaviour.

See e.g. http://blog.regehr.org/archives/959.

Hmm ... This is an interesting blog post. And testing the examples provided there on my own system, and I see the same results.

So I wanted to compare those findings with what happens with our own code, so I extracted the needed pieces and tried the same (source attached). But for us the result is not so clear and obvious at all.

Unless I've overlooked something bloody obvious .... but the result I get is:

-----------------------------------------
mroute_extract_in_addr_t:
.LFB29:

	.cfi_startproc
	movb	$2, 2(%rdi)
	movb	$0, 3(%rdi)
	movb	$4, (%rdi)
	bswap	%esi
	movl	%esi, 4(%rdi)
	ret
	.cfi_endproc

-----------------------------------------

To test it:

gcc -O1 -S -c mroute-test.c
gcc -O1 -S -c mroute-test.c -DPATCHED

I've also tried -O2. Without -O{1,2} I get a very different result, which is expected as that's not optimized - but it is again identical - regardless of which approach being used. The reason might be that the example which is clearly obvious in the blog post uses union.

Now ... I do recognise that the type-punned stuff is critical, in fact my compiler does warn about it with -O2. So I think moving over to memcpy() makes sense ... So I do give this a Feature ACK. But we need to have some proper testing so we're sure we don't break anything.

Signature · 3 lines
kind regards,

David Sommerseth

#include <stdio.h>
#include <stdint.h>
#include <string.h>
#include <arpa/inet.h>

#define MR_MAX_ADDR_LEN 20
#define MR_ADDR_IPV4 2
#define MR_ADDR_MASK 3

struct mroute_addr {
uint8_t len; /* length of address */
uint8_t unused;
uint8_t type; /* MR_ADDR/MR_WITH flags */
uint8_t netbits; /* number of bits in network part of address,
valid if MR_WITH_NETBITS is set */
uint8_t addr[MR_MAX_ADDR_LEN]; /* actual address */
};

void
mroute_extract_in_addr_t (struct mroute_addr *dest, const in_addr_t src) {
dest->type = MR_ADDR_IPV4;
dest->netbits = 0;
dest->len = 4;
#ifndef PATCHED
#warning Unpatched version
*(in_addr_t*)dest->addr = htonl (src);
#else
#warning PATCHED/memcpy version
in_addr_t tmp_addr = htonl (src);
memcpy(dest->addr, &tmp_addr, sizeof(uint32_t));
#endif
}

#3 lore raw
In reply to Steffan Karger
Quoted 11 lines · Steffan Karger

Fixes compiler warnings (undefined behavior) by making the copy explicit to comply to strict aliasing rules. With newer GCC the old code could actually lead to undefined behaviour.

See e.g. http://blog.regehr.org/archives/959.

Signed-off-by Steffan Karger <[email protected]...>
---
 src/openvpn/mroute.h | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

I've taken another stab on this one, read up [1,2,3] more carefully on strict aliasing , type punning and undefined behaviours in compilers. I do consider fixing this important, despite it might not be strictly needed with today's compilers [4], we don't know if and how this undefined behaviour will change in a future compiler update - despite the risk of doing so is very low in our case.

The last time I looked at this, I identified that the assembly being generated by the compiler is pretty much the same and it uses some simple and mov instructions. In addition I also have a better understanding why the code ends up as the same result, as we're just storing an "fresh" integer value to the address extracted via the dereferencing - and not manipulating data extracted from that address and putting something else back, so the compiler's optimizer won't be able to mess this up too badly.

When considering that the optimized code will be most optimal regardless if we use type punning and breaking strict alias or we will use memcpy() in the C code, it makes little sense to me to not fix this warning. Today's compilers are truly clever in this aspect when being asked to optimise variable assignments with static lengths.

So all in all, I am willing to give this a full ACK now. However, compiling this with gcc and the common CFLAGS on RHEL and Fedora, more issues related to strict aliasing appeared. With clang, there were no more warnings with this patch. I think it would be good to resolve the gcc issues as well. The issues located are found in the attachment.

[1] <http://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html>
[2] <http://blog.regehr.org/archives/213>
[3]
<http://cellperformance.beyond3d.com/articles/2006/06/understanding-strict-aliasing.html>
[4] I've tested with gcc-4.8.5 (Red Hat 4.8.5-4) and clang-3.4.2 on

    EL7.2
Signature · 4 lines
kind regards,

David Sommerseth
OpenVPN Technologies, Inc

mroute.c: In function ‘mroute_get_in_addr_t’:
mroute.c:93:7: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]

       *(in_addr_t*)ma->addr = src;
       ^
mroute.c: In function ‘mroute_get_in6_addr’:
mroute.c:105:16: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
       *(struct in6_addr *)ma->addr = src;
                ^

mroute.c: In function ‘mroute_addr_mask_host_bits’:
mroute.c:329:3: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
in_addr_t addr = ntohl(*(in_addr_t*)ma->addr);
^
mroute.c:333:7: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]

       *(in_addr_t*)ma->addr = htonl (addr);
       ^
mroute.c: In function ‘mroute_addr_print_ex’:
mroute.c:432:8: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
        print_in_addr_t( *(in_addr_t*)(&maddr.addr[12]), IA_NET_ORDER, gc));
        ^
mroute.c:437:34: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
         print_in6_addr( *(struct in6_addr*)&maddr.addr, 0, gc));

ps.c: In function ‘port_share_sendmsg’:
ps.c:226:4: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]

    *((socket_descriptor_t*)CMSG_DATA(h)) = sd_send;
    ^
ps.c:231:4: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
    *((socket_descriptor_t*)CMSG_DATA(h)) = sd_null[0];
    ^

ps.c: In function ‘control_message_from_parent’:
ps.c:505:3: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
*((socket_descriptor_t*)CMSG_DATA(h)) = SOCKET_UNDEFINED;
^
ps.c:519:4: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]

    const socket_descriptor_t received_fd = *((socket_descriptor_t*)CMSG_DATA(h));
    ^
socket.c: In function ‘setenv_sockaddr’:
socket.c:2601:4: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
    ia.s_addr = *(in_addr_t *)&addr->addr.in6.sin6_addr.s6_addr[12] ;
    ^
#4 lore raw
Quoted 43 lines · David Sommerseth
Quoted 10 lines · Steffan Karger

Fixes compiler warnings (undefined behavior) by making the copy explicit to comply to strict aliasing rules. With newer GCC the old code could actually lead to undefined behaviour.

See e.g. http://blog.regehr.org/archives/959.

Signed-off-by Steffan Karger <[email protected]...> ---

src/openvpn/mroute.h | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)

I've taken another stab on this one, read up [1,2,3] more carefully on strict aliasing , type punning and undefined behaviours in compilers. I do consider fixing this important, despite it might not be strictly needed with today's compilers [4], we don't know if and how this undefined behaviour will change in a future compiler update - despite the risk of doing so is very low in our case.

The last time I looked at this, I identified that the assembly being generated by the compiler is pretty much the same and it uses some simple and mov instructions. In addition I also have a better understanding why the code ends up as the same result, as we're just storing an "fresh" integer value to the address extracted via the dereferencing - and not manipulating data extracted from that address and putting something else back, so the compiler's optimizer won't be able to mess this up too badly.

When considering that the optimized code will be most optimal regardless if we use type punning and breaking strict alias or we will use memcpy() in the C code, it makes little sense to me to not fix this warning. Today's compilers are truly clever in this aspect when being asked to optimise variable assignments with static lengths.

So all in all, I am willing to give this a full ACK now. However, compiling this with gcc and the common CFLAGS on RHEL and Fedora, more issues related to strict aliasing appeared. With clang, there were no more warnings with this patch. I think it would be good to resolve the gcc issues as well. The issues located are found in the attachment.

Quoted 4 lines

[1]
<http://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html>

[2] <http://blog.regehr.org/archives/213>

Quoted 4 lines

[3]
<http://cellperformance.beyond3d.com/articles/2006/06/understanding-strict-aliasing.html>

[4] I've tested with gcc-4.8.5 (Red Hat 4.8.5-4) and clang-3.4.2 on

Quoted 1 line

EL7.2

Just for the record, I'm using the following CFLAGS:

    CFLAGS="-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions \
            -fstack-protector --param=ssp-buffer-size=4"
Signature · 4 lines
kind regards,

David Sommerseth
OpenVPN Technologies, Inc
author
#5 lore raw

PATCH 1/1 Don't deference type-punned pointers

Dereferencing type-punned pointers is undefined behaviour according to the C standard. We should either obey the standard, or ensure that all supported compilers deal with dereferencing type-punned pointers as we want them to. I think just obeying the standard is the easiest solution.

See e.g. http://blog.regehr.org/archives/959.

This commit refactors the offending code to use unions or memcpy() to comply to strict aliasing rules.

Note that this also slightly changes mroute_addr_mask_host_bits(), to behave as it was probably intended to: only mask the address part, not also the port part of IPv6 adresses if MR_WITH_PORT is used (ie ma->len is sizeof(struct in6_addr)+2).

v2: fix all strict aliasing occurrences, not just those in mroute.h.

Signed-off-by Steffan Karger <[email protected]...>
---
 src/openvpn/error.h  | 10 +++++++
 src/openvpn/mroute.c | 85 +++++++++++++++++++++++++---------------------------
 src/openvpn/mroute.h | 41 +++++++++++++++++++++----
 src/openvpn/multi.c  |  4 +--
 src/openvpn/ps.c     | 10 ++++---
 src/openvpn/socket.c |  3 +-
 6 files changed, 96 insertions(+), 57 deletions(-)
src/openvpn/error.h +10 -0
index ced2fdf..c90b36b 100644
@@ -27,6 +27,8 @@
 
 #include "basic.h"
 
+#include <assert.h>
+
 /* #define ABORT_ON_ERROR */
 
 #ifdef ENABLE_PKCS11

@@ -219,6 +221,14 @@ FILE *msg_fp(const unsigned int flags);
 void assert_failed (const char *filename, int line, const char *condition)
   __attribute__((__noreturn__));
 
+/* Poor-man's static_assert() for when not supplied by assert.h, taken from
+ * Linux's sys/cdefs.h under GPLv2 */
+#ifndef static_assert
+#define static_assert(expr, diagnostic) \
+    extern int (*__OpenVPN_static_assert_function (void)) \
+      [!!sizeof (struct { int __error_if_negative: (expr) ? 2 : -1; })]
+#endif
+
 #ifdef ENABLE_DEBUG
 void crash (void); /* force a segfault (debugging only) */
 #endif
src/openvpn/mroute.c +41 -44

187 lines not on this page · raw message

src/openvpn/mroute.h +35 -6

73 lines not on this page · raw message

src/openvpn/multi.c +2 -2
index a834e94..83b1e99 100644
@@ -1201,7 +1201,7 @@ multi_learn_in6_addr  (struct multi_context *m,
   addr.len = 16;
   addr.type = MR_ADDR_IPV6;
   addr.netbits = 0;
-  memcpy( &addr.addr, &a6, sizeof(a6) );
+  addr.v6.addr = a6;
 
   if (netbits >= 0)
     {

@@ -2431,7 +2431,7 @@ multi_process_incoming_link (struct multi_context *m, struct multi_instance *ins
 		{
 		  /* IPv6 link-local address (fe80::xxx)? */
 		  if ( (src.type & MR_ADDR_MASK) == MR_ADDR_IPV6 &&
-		        src.addr[0] == 0xfe && src.addr[1] == 0x80 )
+		        IN6_IS_ADDR_LINKLOCAL (&src.v6.addr) )
 		    {
 		      /* do nothing, for now.  TODO: add address learning */
 		    }
src/openvpn/ps.c +6 -4
index fe18a9d..2cb68f1 100644
@@ -223,12 +223,12 @@ port_share_sendmsg (const socket_descriptor_t sd,
 
       if (socket_defined (sd_send))
 	{
-	  *((socket_descriptor_t*)CMSG_DATA(h)) = sd_send;
+	  memcpy (CMSG_DATA(h), &sd_send, sizeof (sd_send));
 	}
       else
 	{
 	  socketpair (PF_UNIX, SOCK_DGRAM, 0, sd_null);
-	  *((socket_descriptor_t*)CMSG_DATA(h)) = sd_null[0];
+	  memcpy (CMSG_DATA(h), &sd_null[0], sizeof (sd_null[0]));
 	}
 
       status = sendmsg (sd, &mesg, MSG_NOSIGNAL);

@@ -502,7 +502,8 @@ control_message_from_parent (const socket_descriptor_t sd_control,
   h->cmsg_len = CMSG_LEN(sizeof(socket_descriptor_t));
   h->cmsg_level = SOL_SOCKET;
   h->cmsg_type = SCM_RIGHTS;
-  *((socket_descriptor_t*)CMSG_DATA(h)) = SOCKET_UNDEFINED;
+  static const socket_descriptor_t socket_undefined = SOCKET_UNDEFINED;
+  memcpy (CMSG_DATA(h), &socket_undefined, sizeof(socket_undefined));
 
   status = recvmsg (sd_control, &mesg, MSG_NOSIGNAL);
   if (status != -1)

@@ -516,7 +517,8 @@ control_message_from_parent (const socket_descriptor_t sd_control,
 	}
       else
 	{
-	  const socket_descriptor_t received_fd = *((socket_descriptor_t*)CMSG_DATA(h));
+	  socket_descriptor_t received_fd;
+	  memcpy (&received_fd, CMSG_DATA(h), sizeof(received_fd));
 	  dmsg (D_PS_PROXY_DEBUG, "PORT SHARE PROXY: RECEIVED sd=%d", (int)received_fd);
 
 	  if (status >= 2 && command == COMMAND_REDIRECT)
src/openvpn/socket.c +2 -1
index 18e6409..1fbb415 100644
@@ -2598,7 +2598,8 @@ setenv_sockaddr (struct env_set *es, const char *name_prefix, const struct openv
       if ( IN6_IS_ADDR_V4MAPPED( &addr->addr.in6.sin6_addr ))
 	{
 	  struct in_addr ia;
-	  ia.s_addr = *(in_addr_t *)&addr->addr.in6.sin6_addr.s6_addr[12] ;
+	  memcpy (&ia.s_addr, &addr->addr.in6.sin6_addr.s6_addr[12],
+	      sizeof (ia.s_addr));
 	  openvpn_snprintf (name_buf, sizeof (name_buf), "%s_ip", name_prefix);
 	  openvpn_snprintf (buf, sizeof(buf), "%s", inet_ntoa(ia) );
 	}
Signature · 1 line
2.7.4
author
#6 lore raw

PATCH v3 1/1 Don't deference type-punned pointers

Dereferencing type-punned pointers is undefined behaviour according to the C standard. We should either obey the standard, or ensure that all supported compilers deal with dereferencing type-punned pointers as we want them to. I think just obeying the standard is the easiest solution.

See e.g. http://blog.regehr.org/archives/959.

This commit refactors the offending code to use unions or memcpy() to comply to strict aliasing rules.

Note that this also slightly changes mroute_addr_mask_host_bits(), to behave as it was probably intended to: only mask the address part, not also the port part of IPv6 adresses if MR_WITH_PORT is used (ie ma->len is sizeof(struct in6_addr)+2).

v2: fix all strict aliasing occurrences, not just those in mroute.h
v3: add missing ntohs() in mroute_addr_print_ex()

Signed-off-by Steffan Karger <[email protected]...>
---
 src/openvpn/error.h  | 10 +++++++
 src/openvpn/mroute.c | 85 +++++++++++++++++++++++++---------------------------
 src/openvpn/mroute.h | 41 +++++++++++++++++++++----
 src/openvpn/multi.c  |  4 +--
 src/openvpn/ps.c     | 10 ++++---
 src/openvpn/socket.c |  3 +-
 6 files changed, 96 insertions(+), 57 deletions(-)
src/openvpn/error.h +10 -0
index ced2fdf..c90b36b 100644
@@ -27,6 +27,8 @@
 
 #include "basic.h"
 
+#include <assert.h>
+
 /* #define ABORT_ON_ERROR */
 
 #ifdef ENABLE_PKCS11

@@ -219,6 +221,14 @@ FILE *msg_fp(const unsigned int flags);
 void assert_failed (const char *filename, int line, const char *condition)
   __attribute__((__noreturn__));
 
+/* Poor-man's static_assert() for when not supplied by assert.h, taken from
+ * Linux's sys/cdefs.h under GPLv2 */
+#ifndef static_assert
+#define static_assert(expr, diagnostic) \
+    extern int (*__OpenVPN_static_assert_function (void)) \
+      [!!sizeof (struct { int __error_if_negative: (expr) ? 2 : -1; })]
+#endif
+
 #ifdef ENABLE_DEBUG
 void crash (void); /* force a segfault (debugging only) */
 #endif
src/openvpn/mroute.c +41 -44

187 lines not on this page · raw message

src/openvpn/mroute.h +35 -6

73 lines not on this page · raw message

src/openvpn/multi.c +2 -2
index a834e94..83b1e99 100644
@@ -1201,7 +1201,7 @@ multi_learn_in6_addr  (struct multi_context *m,
   addr.len = 16;
   addr.type = MR_ADDR_IPV6;
   addr.netbits = 0;
-  memcpy( &addr.addr, &a6, sizeof(a6) );
+  addr.v6.addr = a6;
 
   if (netbits >= 0)
     {

@@ -2431,7 +2431,7 @@ multi_process_incoming_link (struct multi_context *m, struct multi_instance *ins
 		{
 		  /* IPv6 link-local address (fe80::xxx)? */
 		  if ( (src.type & MR_ADDR_MASK) == MR_ADDR_IPV6 &&
-		        src.addr[0] == 0xfe && src.addr[1] == 0x80 )
+		        IN6_IS_ADDR_LINKLOCAL (&src.v6.addr) )
 		    {
 		      /* do nothing, for now.  TODO: add address learning */
 		    }
src/openvpn/ps.c +6 -4
index fe18a9d..2cb68f1 100644
@@ -223,12 +223,12 @@ port_share_sendmsg (const socket_descriptor_t sd,
 
       if (socket_defined (sd_send))
 	{
-	  *((socket_descriptor_t*)CMSG_DATA(h)) = sd_send;
+	  memcpy (CMSG_DATA(h), &sd_send, sizeof (sd_send));
 	}
       else
 	{
 	  socketpair (PF_UNIX, SOCK_DGRAM, 0, sd_null);
-	  *((socket_descriptor_t*)CMSG_DATA(h)) = sd_null[0];
+	  memcpy (CMSG_DATA(h), &sd_null[0], sizeof (sd_null[0]));
 	}
 
       status = sendmsg (sd, &mesg, MSG_NOSIGNAL);

@@ -502,7 +502,8 @@ control_message_from_parent (const socket_descriptor_t sd_control,
   h->cmsg_len = CMSG_LEN(sizeof(socket_descriptor_t));
   h->cmsg_level = SOL_SOCKET;
   h->cmsg_type = SCM_RIGHTS;
-  *((socket_descriptor_t*)CMSG_DATA(h)) = SOCKET_UNDEFINED;
+  static const socket_descriptor_t socket_undefined = SOCKET_UNDEFINED;
+  memcpy (CMSG_DATA(h), &socket_undefined, sizeof(socket_undefined));
 
   status = recvmsg (sd_control, &mesg, MSG_NOSIGNAL);
   if (status != -1)

@@ -516,7 +517,8 @@ control_message_from_parent (const socket_descriptor_t sd_control,
 	}
       else
 	{
-	  const socket_descriptor_t received_fd = *((socket_descriptor_t*)CMSG_DATA(h));
+	  socket_descriptor_t received_fd;
+	  memcpy (&received_fd, CMSG_DATA(h), sizeof(received_fd));
 	  dmsg (D_PS_PROXY_DEBUG, "PORT SHARE PROXY: RECEIVED sd=%d", (int)received_fd);
 
 	  if (status >= 2 && command == COMMAND_REDIRECT)
src/openvpn/socket.c +2 -1
index 18e6409..1fbb415 100644
@@ -2598,7 +2598,8 @@ setenv_sockaddr (struct env_set *es, const char *name_prefix, const struct openv
       if ( IN6_IS_ADDR_V4MAPPED( &addr->addr.in6.sin6_addr ))
 	{
 	  struct in_addr ia;
-	  ia.s_addr = *(in_addr_t *)&addr->addr.in6.sin6_addr.s6_addr[12] ;
+	  memcpy (&ia.s_addr, &addr->addr.in6.sin6_addr.s6_addr[12],
+	      sizeof (ia.s_addr));
 	  openvpn_snprintf (name_buf, sizeof (name_buf), "%s_ip", name_prefix);
 	  openvpn_snprintf (buf, sizeof(buf), "%s", inet_ntoa(ia) );
 	}
Signature · 1 line
2.7.4
#7 lore raw

ACK. Brilliant approach to "make the compiler happy" that is *increasing* code readability, not making it worse.

Reviewed ("STARE AT CODE!"), client tests on linux and freebsd, server tests on linux.

Your patch has been applied to the master branch.

commit 8cac9b98d58b97fbd5a23dd9f172a9843ecf5b50

Author: Steffan Karger
Date:   Sun Nov 13 14:17:27 2016 +0100
     Don't deference type-punned pointers

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg13017.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

#8 lore raw

Hi,

Quoted 7 lines · Gert Doering

ACK. Brilliant approach to "make the compiler happy" that is *increasing* code readability, not making it worse.

Reviewed ("STARE AT CODE!"), client tests on linux and freebsd, server tests on linux.

Your patch has been applied to the master branch.

This had interesting fallout. I expected some linux/BSD issues (due to generally different socket things), but instead the buildbots had massive issues on all "older" systems - namely, NetBSD 5.1, OpenBSD 4.9, CentOS 6, FreeBSD 9.3, FreeBSD 7.4.

The core of the issue seems to be this one:

gcc -DHAVE_CONFIG_H -I. -I../../../openvpn/src/openvpn -I../.. -I../../include -I../../../openvpn/include -I../../../openvpn/src/compat -g -O2 -std=c99 -MT error.o -MD -MP -MF .deps/error.Tpo -c -o error.o ../../../openvpn/src/openvpn/error.c
In file included from ../../../openvpn/src/openvpn/manage.h:33,

                 from ../../../openvpn/src/openvpn/options.h:40,
                 from ../../../openvpn/src/openvpn/ssl.h:43,
                 from ../../../openvpn/src/openvpn/ps.h:32,
                 from ../../../openvpn/src/openvpn/error.c:43:

../../../openvpn/src/openvpn/mroute.h:99: warning: declaration does not declare anything ../../../openvpn/src/openvpn/mroute.h:103: error: 'struct mroute_addr' has no member named 'v4' ../../../openvpn/src/openvpn/mroute.h:103: error: 'struct mroute_addr' has no member named 'v4' ../../../openvpn/src/openvpn/mroute.h:103: error: bit-field '__error_if_negative' width not an integer constant ../../../openvpn/src/openvpn/mroute.h:106: error: 'struct mroute_addr' has no member named 'v6' ../../../openvpn/src/openvpn/mroute.h:106: error: 'struct mroute_addr' has no member named 'v6'

which translates to

- the new union most not by anonymous ("does not declare anything")
- if I name it, like "a", the warning in line 99 goes away
- but then, all references need to be added that name

     "addr->a.v4.addr"

etc.

While I could live with abandoning FreeBSD 7.4, we need to keep supporting at least FreeBSD 9.3 - so, any ideas how to deal with that?

MinGW builds fail because there is no "in_port_t" declaration, which is something configure should be able to fix for us.

gert

Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...
author
#9 lore raw

PATCH 1/1 Fix builds on compilers without anonymous union support

The "Don't dereference type-punned pointers" patch introduced an anonymous union, which older compilers do not support (or refuse to support when -std=c99 is defined). Add a configure check, and some wrapper defines to repair builds on those compilers.

Signed-off-by Steffan Karger <[email protected]...>
---
 configure.ac         | 34 ++++++++++++++++++++++++++++------
 src/openvpn/mroute.h | 12 +++++++++++-
 2 files changed, 39 insertions(+), 7 deletions(-)
configure.ac +28 -6
index 357ba29..bba29b2 100644
@@ -390,6 +390,12 @@ AC_DEFINE_UNQUOTED([IPROUTE_PATH], ["$IPROUTE"], [Path to iproute tool])
 AC_DEFINE_UNQUOTED([ROUTE_PATH], ["$ROUTE"], [Path to route tool])
 AC_DEFINE_UNQUOTED([SYSTEMD_ASK_PASSWORD_PATH], ["$SYSTEMD_ASK_PASSWORD"], [Path to systemd-ask-password tool])
 
+# Set -std=c99 unless user already specified a -std=
+case "${CFLAGS}" in
+  *-std=*) ;;
+  *)       CFLAGS="${CFLAGS} -std=c99" ;;
+esac
+
 #
 # Libtool
 #

@@ -547,6 +553,28 @@ AC_CHECK_DECLS(
 	,
 	[[${SOCKET_INCLUDES}]]
 )
+AC_CHECKING([anonymous union support])
+AC_COMPILE_IFELSE(
+	[AC_LANG_PROGRAM(
+		[[
+			struct mystruct {
+			  union {
+			    int m1;
+			    char m2;
+			  };
+			};
+		]],
+		[[
+			struct mystruct s;
+			s.m1 = 1; s.m2 = 2;
+		]]
+	)],
+	[
+		AC_MSG_RESULT([yes])
+		AC_DEFINE([HAVE_ANONYMOUS_UNION_SUPPORT], [], [Compiler supports anonymous unions])
+	],
+	[AC_MSG_RESULT([no])]
+)
 
 dnl We emulate signals in Windows
 AC_CHECK_DECLS(

@@ -1142,12 +1170,6 @@ if test "${enable_pkcs11}" = "yes"; then
 	)
 fi
 
-# Set -std=c99 unless user already specified a -std=
-case "${CFLAGS}" in
-  *-std=*) ;;
-  *)       CFLAGS="${CFLAGS} -std=c99" ;;
-esac
-
 if test "${enable_pedantic}" = "yes"; then
 	enable_strict="yes"
 	CFLAGS="${CFLAGS} -pedantic"
src/openvpn/mroute.h +11 -1
index 5fe17e7..8f7a064 100644
@@ -96,7 +96,17 @@ struct mroute_addr {
       uint8_t prefix[12];
       in_addr_t addr;		/* _network order_ IPv4 address */
     } v4mappedv6;
-  };
+  }
+#ifndef HAVE_ANONYMOUS_UNION_SUPPORT
+/* Wrappers to support compilers that do not grok anonymous unions */
+  mroute_union
+#define raw_addr mroute_union.raw_addr
+#define eth_addr mroute_union.eth_addr
+#define v4 mroute_union.v4
+#define v6 mroute_union.v6
+#define v4mappedv6 mroute_union.v4mappedv6
+#endif
+  ;
 };
 
 /* Double-check that struct packing works as expected */
Signature · 1 line
2.7.4
#10 lore raw

[Openvpn-devel] [PATCH applied] Re: Fix builds on compilers without anonymous union support

ACK, tested on FreeBSD 7.4 ("builds, works, happiness"), FreeBSD 10, and recent Linux ("were happy already", HAVE_ANONYMOUS... properly detected).

Let's see if buildbot thinks otherwise for the remaining platforms...

Your patch has been applied to the master branch.

commit 9223336a88bc065348d0ce37621bbf2b1087ba0e

Author: Steffan Karger
Date:   Sun Nov 13 19:03:23 2016 +0100
     Fix builds on compilers without anonymous union support

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://www.mail-archive.com/search?l=mid&[email protected]...
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

#11 lore raw

PATCH 1/1 Fix compilation on MinGW with -std=c99

In reply to Steffan Karger

commit 9223336a88bc moved the CFLAGS="-std=c99" bit in configure.ac before the "socklen_t" test, which relies on #ifdef WIN32 to decide whether to include <ws2tcpip.h> or <sys/socket.h> - which is no longer defined then, and things explode in interesting ways.

Change to _WIN32, which is the "always defined on all compilers" define for this.

Signed-off-by Gert Doering <[email protected]...>
---
 m4/ax_socklen_t.m4 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
m4/ax_socklen_t.m4 +1 -1
index cd7cad8..b420a17 100644
@@ -55,7 +55,7 @@ getpeername(0,0,&len);
 		],
 		[[
 #include <sys/types.h>
-#ifdef WIN32
+#ifdef _WIN32
 #include <ws2tcpip.h>
 #else
 #include <sys/socket.h>
Signature · 1 line
1.9.1
author
#12 lore raw

Re: [Openvpn-devel] [PATCH] Fix compilation on MinGW with -std=c99

Quoted 19 lines · Gert Doering

commit 9223336a88bc moved the CFLAGS="-std=c99" bit in configure.ac before the "socklen_t" test, which relies on #ifdef WIN32 to decide whether to include <ws2tcpip.h> or <sys/socket.h> - which is no longer defined then, and things explode in interesting ways.

Change to _WIN32, which is the "always defined on all compilers" define for this.

Signed-off-by Gert Doering <[email protected]...>
---
 m4/ax_socklen_t.m4 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
m4/ax_socklen_t.m4 +0 -0
index cd7cad8..b420a17 100644

m4/ax_socklen_t.m4 getpeername(0,0,&len); · line 61

Quoted 8 lines
m4/ax_socklen_t.m4 +1 -1
@@ -55,7 +55,7 @@ getpeername(0,0,&len);
 		],
 		[[
 #include <sys/types.h>
-#ifdef WIN32
+#ifdef _WIN32
 #include <ws2tcpip.h>
 #else
 #include <sys/socket.h>

ACK. So many well-hidden bugs... Thanks for figuring this out!

-Steffan

#13 lore raw

[Openvpn-devel] [PATCH applied] Re: Fix compilation on MinGW with -std=c99

In reply to Gert Doering

Patch has been applied to the master branch. Hope buildbot likes me again :)

commit 11eedcd0071e7185fc3011cda4703f5cc75fe979

Author: Gert Doering
Date:   Sun Nov 13 20:36:45 2016 +0100
     Fix compilation on MinGW with -std=c99

     Signed-off-by: Gert Doering <[email protected]...>
     Acked-by: Steffan Karger <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg13032.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

#14 lore raw

PATCH 1/1 Add in_port_t check to configure.ac

In reply to Steffan Karger

commit 8cac9b98d58b97 introduced using in_port_t which is not available on (all?) mingw build environments.

Add configure check, falling back to uint16_t.

Signed-off-by Gert Doering <[email protected]...>
---
 configure.ac | 6 ++++++
 1 file changed, 6 insertions(+)
configure.ac +6 -0
index 357ba29..e44f619 100644
@@ -493,6 +493,12 @@ AC_CHECK_TYPES(
 	[AC_DEFINE([in_addr_t], [uint32_t], [Workaround missing in_addr_t])],
 	[[${SOCKET_INCLUDES}]]
 )
+AC_CHECK_TYPES(
+	[in_port_t],
+	,
+	[AC_DEFINE([in_port_t], [uint16_t], [Workaround missing in_port_t])],
+	[[${SOCKET_INCLUDES}]]
+)
 AC_CHECK_TYPE(
 	[struct iphdr],
 	[AC_DEFINE([HAVE_IPHDR], [1], [struct iphdr needed for IPv6 support])],
Signature · 1 line
1.9.1
author
#15 lore raw

Re: [Openvpn-devel] [PATCH] Add in_port_t check to configure.ac

Quoted 18 lines · Gert Doering

commit 8cac9b98d58b97 introduced using in_port_t which is not available on (all?) mingw build environments.

Add configure check, falling back to uint16_t.

Signed-off-by Gert Doering <[email protected]...>
---
 configure.ac | 6 ++++++
 1 file changed, 6 insertions(+)
configure.ac +0 -0
index 357ba29..e44f619 100644
@@ -493,6 +493,12 @@ AC_CHECK_TYPES(
 	[AC_DEFINE([in_addr_t], [uint32_t], [Workaround missing in_addr_t])],
 	[[${SOCKET_INCLUDES}]]

configure.ac AC_CHECK_TYPES( · line 504

Quoted 10 lines
configure.ac +6 -0
 )
+AC_CHECK_TYPES(
+	[in_port_t],
+	,
+	[AC_DEFINE([in_port_t], [uint16_t], [Workaround missing in_port_t])],
+	[[${SOCKET_INCLUDES}]]
+)
 AC_CHECK_TYPE(
 	[struct iphdr],
 	[AC_DEFINE([HAVE_IPHDR], [1], [struct iphdr needed for IPv6 support])],

ACK

-Steffan

#16 lore raw

[Openvpn-devel] [PATCH applied] Re: Add in_port_t check to configure.ac

In reply to Gert Doering

Patch has been applied to the master branch.

commit dd6714ae0a47a9401898ccc0dd7079f58ba29901

Author: Gert Doering
Date:   Sun Nov 13 16:55:35 2016 +0100
     Add in_port_t check to configure.ac

     Signed-off-by: Gert Doering <[email protected]...>
     Acked-by: Steffan Karger <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg13021.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering