Fix implicit declarations when HAVE_OPENSSL_ENGINE is unset

To avoid issues like https://community.openvpn.net/openvpn/ticket/792 to happen again, I wanted to also submit a patch that adds a few -Werror= flags. However, I wonder what would be your preferred way to only add flags that are supported by the compiler.

One solution is with
https://www.gnu.org/software/autoconf-archive/ax_check_compile_flag.html
which means:
- copy ax_check_compile_flag.m4 from the autoconf archive to the openvpn repository
- include it in configure.ac
- and for example:

AX_CHECK_COMPILE_FLAG([ -Werror=implicit ], [
CFLAGS_EXTRA="$CFLAGS_EXTRA -Werror=implicit"
])

Quoted 1 line

To avoid issues like https://community.openvpn.net/openvpn/ticket/792

to happen again, I wanted to also submit a patch that adds a few -Werror= flags. However, I wonder what would be your preferred way to only add flags that are supported by the compiler.

Quoted 3 lines

One solution is with
https://www.gnu.org/software/autoconf-archive/ax_check_

compile_flag.html

Quoted 2 lines

which means:
- copy ax_check_compile_flag.m4 from the autoconf archive to the openvpn

repository

Quoted 6 lines

- include it in configure.ac
- and for example:

AX_CHECK_COMPILE_FLAG([ -Werror=implicit ], [
CFLAGS_EXTRA="$CFLAGS_EXTRA -Werror=implicit"
])

Yes, please!

You can put the macro in m4/

-Steffan

------------------------------------------------------------
------------------
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 [email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

2016-12-16 22:06 GMT+05:00 Steffan Karger <[email protected]... <mailto:[email protected]...>>:

On 16-12-16 17:42, Julien Muchembled wrote:
> To avoid issues like https://community.openvpn.net/openvpn/ticket/792
<https://community.openvpn.net/openvpn/ticket/792> to happen again, I wanted to also submit a patch that adds a few -Werror= flags. However, I wonder what would be your preferred way to only add flags that are supported by the compiler.

is it supported for clang ?

Yes, please!

You can put the macro in m4/

The bug in Trac #792 (pointer wrongly truncated as 'int', leading to segfaults) would have been understood faster with either
-Werror=implicit or -Werror=format
(and above all, before running buggy executables in production)

Debian already compiles with -Werror=format-security, and it's indeed a good idea for a sensitive software like OpenVPN to use it.
---

Quoted 4 lines

Yes, please!

You can put the macro in m4/

OK. Not sure about the license. It's GPL3+

I also don't include the latest version, which has (among other changes):
-[AC_PREREQ(2.59)dnl for _AC_LANG_PREFIX
+[AC_PREREQ(2.64)dnl for _AC_LANG_PREFIX and AS_VAR_IF

configure.ac | 7 +++++

 m4/ax_check_compile_flag.m4 | 74 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+)
 create mode 100644 m4/ax_check_compile_flag.m4

configure.ac +0 -0

index 4f086ea..3357ea0 100644

configure.ac ifdef( · line 421

Quoted 7 lines

configure.ac +7 -0

+AX_CHECK_COMPILE_FLAG([ -Werror=implicit ], [
+    CFLAGS_EXTRA="$CFLAGS_EXTRA -Werror=implicit"
+])
+AX_CHECK_COMPILE_FLAG([ -Werror=format -Werror=format-security ], [
+    CFLAGS_EXTRA="$CFLAGS_EXTRA -Werror=format -Werror=format-security"
+])
+

Shouldn't there be a CFLAGS="${CFLAGS} ${CFLAGS_EXTRA}" somewhere? Or just use CFLAGS directly, instead of _EXTRA. Without that, I don't see the flags appear in the make output.

Quoted 11 lines · Steffan Karger

configure.ac ifdef( · line 421

Quoted 7 lines

configure.ac +7 -0

+AX_CHECK_COMPILE_FLAG([ -Werror=implicit ], [
+    CFLAGS_EXTRA="$CFLAGS_EXTRA -Werror=implicit"
+])
+AX_CHECK_COMPILE_FLAG([ -Werror=format -Werror=format-security ], [
+    CFLAGS_EXTRA="$CFLAGS_EXTRA -Werror=format -Werror=format-security"
+])
+

Shouldn't there be a CFLAGS="${CFLAGS} ${CFLAGS_EXTRA}" somewhere? Or just use CFLAGS directly, instead of _EXTRA. Without that, I don't see the flags appear in the make output.

(how did I miss that?) anyway, there's also
https://fossies.org/linux/alsa-lib/m4/attributes.m4

bigger, GPL2+, and it defines macros that simply append to CFLAGS (e.g. CC_CHECK_CFLAG_APPEND)

BTW, instead of failing on these 3 specific warnings, we could go the opposite way, since -Wall is quite silent. I suggest:

-Wall -Wno-unused -Wno-pointer-sign -Werror -Wno-error=strict-aliasing -Werror=format-security

Also, is -Werror=format-security needed (instead of -Wformat-security) if we already have -Werror?

BTW, instead of failing on these 3 specific warnings, we could go the opposite way, since -Wall is quite silent. I suggest:

-Wall -Wno-unused -Wno-pointer-sign -Werror -Wno-error=strict-aliasing -Werror=format-security

Quoted 3 lines · Julien Muchembled

BTW, instead of failing on these 3 specific warnings, we could go the opposite way, since -Wall is quite silent. I suggest:

-Wall -Wno-unused -Wno-pointer-sign -Werror -Wno-error=strict-aliasing -Werror=format-security

NAK to "-Wall -Werror", unless you've tested that this does not break compilation on any of the supported platforms.

Quoted 2 lines

NAK to "-Wall -Werror", unless you've tested that this does not break compilation on any of the supported platforms.

Sure, but I think we should help out with that. Such as, do a test build on the supported platforms and see what fixes / extra flags we need to make it work. (I say we, because although I don't have access to the build systems, I am willing to help out with fixing fallout.)

I'm willing to invest in this, because I think using the compiler as a sanity checker more will save us time in the long run.

Taking 2.4_rc2 and building on AIX with -Wall -Wno-pointer-sign is actually much less bad than I thought :-) - but there are things in there which are annoying, like these

../../../git/src/openvpn/route.c: In function 'add_route_ipv6':
../../../git/src/openvpn/route.c:1834:10: warning: variable 'gateway_needed' set but not used [-Wunused-but-set-variable]

     bool gateway_needed = false;
          ^

(fixing is easy, but implies adding an #ifdef TARGET_... orgy or refactoring this whole code part - which should be done, but won't happen "over night")

More problematic are these:

../../../git/src/openvpn/socket.c: In function 'print_link_socket_actual_ex':
../../../git/src/openvpn/socket.c:2802:21: warning: implicit declaration of function 'if_indextoname' [-Wimplicit-function-declaration]

                     if_indextoname(act->pi.in6.ipi6_ifindex, ifname);
                     ^
../../../git/src/openvpn/socket.c: In function 'unix_socket_get_peer_uid_gid':
../../../git/src/openvpn/socket.c:3950:5: warning: implicit declaration of function 'getpeereid' [-Wimplicit-function-declaration]
     if (getpeereid(sd, &u, &g) == -1)

../../../git/src/openvpn/tun.c: In function 'open_tun_generic':
../../../git/src/openvpn/tun.c:1787:13: warning: implicit declaration of function 'if_nametoindex' [-Wimplicit-function-declaration]
if (if_nametoindex( dev ) > 0)

... since we're really not supposed to redeclare system prototypes (and if we do, it will break on other versions of AIX).

BTW, instead of failing on these 3 specific warnings, we could go the

-Wall -Wno-unused -Wno-pointer-sign -Werror -Wno-error=strict-aliasing

Quoted 1 line

The world is bigger than Linux.

There should be high level autoconf magic, which will add option if chosen compiler supports it.

I'm particurlarly afraid of cross compilers like mingw

The bug in Trac #792 (pointer wrongly truncated as 'int', leading to segfaults) would have been understood faster with either
-Werror=implicit or -Werror=format
(and above all, before running buggy executables in production)

Debian already compiles with -Werror=format-security, and it's indeed a good idea for a sensitive software like OpenVPN to use it.
---

Quoted 4 lines

Yes, please!

You can put the macro in m4/

OK. Not sure about the license. It's GPL3+

Am 16.12.16 um 19:26 schrieb Julien Muchembled:

Quoted 8 lines

The bug in Trac #792 (pointer wrongly truncated as 'int', leading to segfaults) would have been understood faster with either
-Werror=implicit or -Werror=format
(and above all, before running buggy executables in production)

Debian already compiles with -Werror=format-security, and it's indeed a good idea for a sensitive software like OpenVPN to use it.
---

Quoted 6 lines

Quoted 4 lines

Yes, please!

You can put the macro in m4/

OK. Not sure about the license. It's GPL3+

We cannot accept GPL3+ since it is incompatible with our license (GPL2).

This fixes a regression in commit 8e95e20913:
on amd64, an implicit declaration of ERR_error_string leads to segfaults if the returned pointer does not fit in an 'int'.

Trac: #792

Signed-off-by Julien Muchembled <[email protected]...>

---
This patch is for the 2.3 branch. In the master branch, crypto_openssl.c already includes these files.

 src/openvpn/crypto_openssl.c | 2 ++
 1 file changed, 2 insertions(+)

src/openvpn/crypto_openssl.c +0 -0

index 7b1ac6c..fe22044 100644

Quoted 6 lines · Julien Muchembled

This fixes a regression in commit 8e95e20913:
on amd64, an implicit declaration of ERR_error_string leads to segfaults if the returned pointer does not fit in an 'int'.

Trac: #792

Quoted 1 line

[...]

ACK, fix is clearly correct.

Though there must be more then just 'on amd64' to make this break - this code has run an a *lot* of amd64 systems and this is the first breakage report. (Of course that doesn't mean in any way that we shouldn't fix it, I'm just curious.)