[Openvpn-devel,v2] msvc: fix various level2 warnings

Message ID 20200625083745.211-1-lstipakov@gmail.com
State Superseded
Headers show
Series
  • [Openvpn-devel,v2] msvc: fix various level2 warnings
Related show

Commit Message

Lev Stipakov June 25, 2020, 8:37 a.m.
From: Lev Stipakov <lev@openvpn.net>

Also set warnings level to level2 and
enable "treat warnings as errors" flag.

Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
 src/compat/Debug.props       |  1 -
 src/openvpn/block_dns.c      |  3 ---
 src/openvpn/crypto_openssl.c |  2 +-
 src/openvpn/init.c           |  2 +-
 src/openvpn/mtu.c            |  4 ++--
 src/openvpn/openvpn.vcxproj  | 26 +++++++++++++++++---------
 src/openvpn/options.c        |  2 +-
 src/openvpn/route.c          |  2 +-
 src/openvpn/ssl_backend.h    |  2 +-
 src/openvpn/tun.c            |  5 +++--
 10 files changed, 27 insertions(+), 22 deletions(-)

Comments

Gert Doering June 25, 2020, 8:54 a.m. | #1
Hi,

On Thu, Jun 25, 2020 at 11:37:45AM +0300, Lev Stipakov wrote:
> @@ -315,7 +315,7 @@ show_available_ciphers(void)
>          }
>      }
>  
> -    qsort(cipher_list, num_ciphers, sizeof(*cipher_list), cipher_name_cmp);
> +    qsort((void *)cipher_list, num_ciphers, sizeof(*cipher_list), cipher_name_cmp);

I find this one questionable.  Qsort wants a void *, but every pointer type
should be convertible to void *, without a warning?

(Adding casts for things that are explicitly always allowed and well-defined 
is something I do not like very much - it just makes "more code to read")


> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index 2c8db68d..107bb4c9 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -1812,7 +1812,7 @@ do_open_tun(struct context *c)
>  #ifdef _WIN32
>      /* store (hide) interactive service handle in tuntap_options */
>      c->c1.tuntap->options.msg_channel = c->options.msg_channel;
> -    msg(D_ROUTE, "interactive service msg_channel=%u", (unsigned int) c->options.msg_channel);
> +    msg(D_ROUTE, "interactive service msg_channel=%u", (unsigned long long) c->options.msg_channel);

This gets a NAK.  "%u" is "unsigned int", not (never!) (unsigned long long) -
if you want that, because the data type is so big, use %llu or %I64u
(Windows special, according to common.h).


> index 04868cd6..8178ff06 100644
> --- a/src/openvpn/mtu.c
> +++ b/src/openvpn/mtu.c
> @@ -175,7 +175,7 @@ set_mtu_discover_type(int sd, int mtu_type, sa_family_t proto_af)
>  #if defined(HAVE_SETSOCKOPT) && defined(IP_MTU_DISCOVER)
>              case AF_INET:
>                  if (setsockopt
> -                        (sd, IPPROTO_IP, IP_MTU_DISCOVER, &mtu_type, sizeof(mtu_type)))
> +                        (sd, IPPROTO_IP, IP_MTU_DISCOVER, (const char *)&mtu_type, sizeof(mtu_type)))

I find this questionable as well.  Setsockopt() on Unix wants a 
"void *", not a "const char *" - and this cast suggest to the reader
that we're dealing with "something character based", which it is not.

>  #if defined(HAVE_SETSOCKOPT) && defined(IPV6_MTU_DISCOVER)
>              case AF_INET6:
>                  if (setsockopt
> -                        (sd, IPPROTO_IPV6, IPV6_MTU_DISCOVER, &mtu_type, sizeof(mtu_type)))
> +                        (sd, IPPROTO_IPV6, IPV6_MTU_DISCOVER, (const char *)&mtu_type, sizeof(mtu_type)))

Same here.

>  show_available_tls_ciphers_list(const char *cipher_list,
>                                  const char *tls_cert_profile,
> -                                bool tls13);
> +                                const bool tls13);

I'm not sure why this is needed or beneficial?  This const stuff is
good for pointers ("do not modify the pointed-to area") but for
call-by-value arguments, the compiler can figure this out itself.

> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -233,10 +233,11 @@ do_set_mtu_service(const struct tuntap *tt, const short family, const int mtu)
>              sizeof(set_mtu_message_t),
>              0
>          },
> -        .iface = {.index = tt->adapter_index,.name = tt->actual_name },
> +        .iface = {.index = tt->adapter_index},

Sure of that?  As in "are you sure the service does not need the name,
never"?

>          .mtu = mtu,
>          .family = family
>      };
> +    strncpynt(mtu_msg.iface.name, tt->actual_name, sizeof(mtu_msg.iface.name));

Well spotted.  This is interesting, the assignment ".name = tt->actual_name"
shouldn't be legal (as it's basically a hidden strcpy(), not a integral
type or structure assignment), but mingw is not complaining and the 
resulting binary works.  But this definitely is a necessary bugfix.


>      if (!send_msg_iservice(pipe, &mtu_msg, sizeof(mtu_msg), &ack, "Set_mtu"))
>      {
> @@ -889,7 +890,7 @@ add_route_connected_v6_net(struct tuntap *tt,
>  }
>  
>  void
> -delete_route_connected_v6_net(struct tuntap *tt,
> +delete_route_connected_v6_net(const struct tuntap *tt,
>                                const struct env_set *es)

I might be convinced that this is a useful change, but if it's needed(!)
for delete_route_connected_v6_net(), the "cost" should be added to 
add_route_connected_v6_net() as well.

gert
Arne Schwabe June 25, 2020, 9:18 a.m. | #2
>> @@ -1812,7 +1812,7 @@ do_open_tun(struct context *c)
>>  #ifdef _WIN32
>>      /* store (hide) interactive service handle in tuntap_options */
>>      c->c1.tuntap->options.msg_channel = c->options.msg_channel;
>> -    msg(D_ROUTE, "interactive service msg_channel=%u", (unsigned int) c->options.msg_channel);
>> +    msg(D_ROUTE, "interactive service msg_channel=%u", (unsigned long long) c->options.msg_channel);
> 
> This gets a NAK.  "%u" is "unsigned int", not (never!) (unsigned long long) -
> if you want that, because the data type is so big, use %llu or %I64u
> (Windows special, according to common.h).

Or use the standardised ones. E.g. PRIu64 for uint64_t. We already use
that auth-token.c.

See also https://en.cppreference.com/w/cpp/header/cinttypes (or another
reference).


> 
>> index 04868cd6..8178ff06 100644
>> --- a/src/openvpn/mtu.c
>> +++ b/src/openvpn/mtu.c
>> @@ -175,7 +175,7 @@ set_mtu_discover_type(int sd, int mtu_type, sa_family_t proto_af)
>>  #if defined(HAVE_SETSOCKOPT) && defined(IP_MTU_DISCOVER)
>>              case AF_INET:
>>                  if (setsockopt
>> -                        (sd, IPPROTO_IP, IP_MTU_DISCOVER, &mtu_type, sizeof(mtu_type)))
>> +                        (sd, IPPROTO_IP, IP_MTU_DISCOVER, (const char *)&mtu_type, sizeof(mtu_type)))
> 
> I find this questionable as well.  Setsockopt() on Unix wants a 
> "void *", not a "const char *" - and this cast suggest to the reader
> that we're dealing with "something character based", which it is not.
> 
>>  #if defined(HAVE_SETSOCKOPT) && defined(IPV6_MTU_DISCOVER)
>>              case AF_INET6:
>>                  if (setsockopt
>> -                        (sd, IPPROTO_IPV6, IPV6_MTU_DISCOVER, &mtu_type, sizeof(mtu_type)))
>> +                        (sd, IPPROTO_IPV6, IPV6_MTU_DISCOVER, (const char *)&mtu_type, sizeof(mtu_type)))
> 
> Same here.
> 
>>  show_available_tls_ciphers_list(const char *cipher_list,
>>                                  const char *tls_cert_profile,
>> -                                bool tls13);
>> +                                const bool tls13);
> 
> I'm not sure why this is needed or beneficial?  This const stuff is
> good for pointers ("do not modify the pointed-to area") but for
> call-by-value arguments, the compiler can figure this out itself.

Clang tidy actually warns against this for integral types saying that
they are passed by value always in C++. Not sure if this also applies to C.

Arne
Lev Stipakov June 25, 2020, 9:24 a.m. | #3
Hi,

> > -    qsort(cipher_list, num_ciphers, sizeof(*cipher_list), cipher_name_cmp);
> > +    qsort((void *)cipher_list, num_ciphers, sizeof(*cipher_list), cipher_name_cmp);
>
> I find this one questionable.  Qsort wants a void *, but every pointer type
> should be convertible to void *, without a warning?

Yes, but this is about "const". VS shows "'function': different
'const' qualifiers".
I'll go with this one:

    /* cast to non-const to prevent warning */
    qsort((EVP_CIPHER *)cipher_list, num_ciphers,
sizeof(*cipher_list), cipher_name_cmp);

> > -    msg(D_ROUTE, "interactive service msg_channel=%u", (unsigned int) c->options.msg_channel);
> > +    msg(D_ROUTE, "interactive service msg_channel=%u", (unsigned long long) c->options.msg_channel);
>
> This gets a NAK.  "%u" is "unsigned int", not (never!) (unsigned long long) -

Good point, thanks. The warning was about casting from HANDLE to
unsigned int. I'll change
format specifier to "%llu".

> > -                        (sd, IPPROTO_IP, IP_MTU_DISCOVER, &mtu_type, sizeof(mtu_type)))
> > +                        (sd, IPPROTO_IP, IP_MTU_DISCOVER, (const char *)&mtu_type, sizeof(mtu_type)))
>
> I find this questionable as well.  Setsockopt() on Unix wants a
> "void *", not a "const char *" - and this cast suggest to the reader
> that we're dealing with "something character based", which it is not.

That's the case in Windows:

https://docs.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-setsockopt

> int setsockopt( SOCKET s, int level, int optname, const char *optval, int optlen );

> >  show_available_tls_ciphers_list(const char *cipher_list,
> >                                  const char *tls_cert_profile,
> > -                                bool tls13);
> > +                                const bool tls13);
>
> I'm not sure why this is needed or beneficial?

Otherwise there is warning "formal parameter 3 different from declaration".

I noticed that mbedtls implementation doesn't have "const", so I'll
just remove it from openssl implementation and won't change header.

> >  void
> > -delete_route_connected_v6_net(struct tuntap *tt,
> > +delete_route_connected_v6_net(const struct tuntap *tt,
> >                                const struct env_set *es)
>
> I might be convinced that this is a useful change, but if it's needed(!)
> for delete_route_connected_v6_net(), the "cost" should be added to
> add_route_connected_v6_net() as well.

I looked closer and in fact second argument of delete_route_connected_v6_net()
is always NULL, so I'll remove that parameter.

Patch

diff --git a/src/compat/Debug.props b/src/compat/Debug.props
index e5e9f681..31bb9d91 100644
--- a/src/compat/Debug.props
+++ b/src/compat/Debug.props
@@ -15,7 +15,6 @@ 
       <PreprocessorDefinitions>_DEBUG;%(PreprocessorDefinitions)</PreprocessorDefinitions>
       <RuntimeLibrary>MultiThreadedDebugDLL</RuntimeLibrary>
       <DebugInformationFormat>EditAndContinue</DebugInformationFormat>
-      <MinimalRebuild>true</MinimalRebuild>
     </ClCompile>
   </ItemDefinitionGroup>
   <ItemGroup />
diff --git a/src/openvpn/block_dns.c b/src/openvpn/block_dns.c
index 889d6bb9..f4718fc2 100644
--- a/src/openvpn/block_dns.c
+++ b/src/openvpn/block_dns.c
@@ -109,9 +109,6 @@  DEFINE_GUID(
 
 static WCHAR *FIREWALL_NAME = L"OpenVPN";
 
-VOID NETIOAPI_API_
-InitializeIpInterfaceEntry(PMIB_IPINTERFACE_ROW Row);
-
 /*
  * Default msg handler does nothing
  */
diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index 2027d8dc..8805e25e 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -315,7 +315,7 @@  show_available_ciphers(void)
         }
     }
 
-    qsort(cipher_list, num_ciphers, sizeof(*cipher_list), cipher_name_cmp);
+    qsort((void *)cipher_list, num_ciphers, sizeof(*cipher_list), cipher_name_cmp);
 
     for (i = 0; i < num_ciphers; i++)
     {
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 2c8db68d..107bb4c9 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1812,7 +1812,7 @@  do_open_tun(struct context *c)
 #ifdef _WIN32
     /* store (hide) interactive service handle in tuntap_options */
     c->c1.tuntap->options.msg_channel = c->options.msg_channel;
-    msg(D_ROUTE, "interactive service msg_channel=%u", (unsigned int) c->options.msg_channel);
+    msg(D_ROUTE, "interactive service msg_channel=%u", (unsigned long long) c->options.msg_channel);
 #endif
 
     /* allocate route list structure */
diff --git a/src/openvpn/mtu.c b/src/openvpn/mtu.c
index 04868cd6..8178ff06 100644
--- a/src/openvpn/mtu.c
+++ b/src/openvpn/mtu.c
@@ -175,7 +175,7 @@  set_mtu_discover_type(int sd, int mtu_type, sa_family_t proto_af)
 #if defined(HAVE_SETSOCKOPT) && defined(IP_MTU_DISCOVER)
             case AF_INET:
                 if (setsockopt
-                        (sd, IPPROTO_IP, IP_MTU_DISCOVER, &mtu_type, sizeof(mtu_type)))
+                        (sd, IPPROTO_IP, IP_MTU_DISCOVER, (const char *)&mtu_type, sizeof(mtu_type)))
                 {
                     msg(M_ERR, "Error setting IP_MTU_DISCOVER type=%d on TCP/UDP socket",
                         mtu_type);
@@ -186,7 +186,7 @@  set_mtu_discover_type(int sd, int mtu_type, sa_family_t proto_af)
 #if defined(HAVE_SETSOCKOPT) && defined(IPV6_MTU_DISCOVER)
             case AF_INET6:
                 if (setsockopt
-                        (sd, IPPROTO_IPV6, IPV6_MTU_DISCOVER, &mtu_type, sizeof(mtu_type)))
+                        (sd, IPPROTO_IPV6, IPV6_MTU_DISCOVER, (const char *)&mtu_type, sizeof(mtu_type)))
                 {
                     msg(M_ERR, "Error setting IPV6_MTU_DISCOVER type=%d on TCP6/UDP6 socket",
                         mtu_type);
diff --git a/src/openvpn/openvpn.vcxproj b/src/openvpn/openvpn.vcxproj
index 53ac5482..c34733ea 100644
--- a/src/openvpn/openvpn.vcxproj
+++ b/src/openvpn/openvpn.vcxproj
@@ -28,23 +28,23 @@ 
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'" Label="Configuration">
     <ConfigurationType>Application</ConfigurationType>
     <WholeProgramOptimization>true</WholeProgramOptimization>
-    <CharacterSet>Unicode</CharacterSet>
+    <CharacterSet>NotSet</CharacterSet>
     <PlatformToolset>v142</PlatformToolset>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'" Label="Configuration">
     <ConfigurationType>Application</ConfigurationType>
     <WholeProgramOptimization>true</WholeProgramOptimization>
-    <CharacterSet>Unicode</CharacterSet>
+    <CharacterSet>NotSet</CharacterSet>
     <PlatformToolset>v142</PlatformToolset>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'" Label="Configuration">
     <ConfigurationType>Application</ConfigurationType>
-    <CharacterSet>Unicode</CharacterSet>
+    <CharacterSet>NotSet</CharacterSet>
     <PlatformToolset>v142</PlatformToolset>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'" Label="Configuration">
     <ConfigurationType>Application</ConfigurationType>
-    <CharacterSet>Unicode</CharacterSet>
+    <CharacterSet>NotSet</CharacterSet>
     <PlatformToolset>v142</PlatformToolset>
   </PropertyGroup>
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.props" />
@@ -74,7 +74,9 @@ 
     <ClCompile>
       <AdditionalIncludeDirectories>..\compat;$(TAP_WINDOWS_HOME)/include;$(OPENSSL_HOME)/include;$(LZO_HOME)/include;$(PKCS11H_HOME)/include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
       <PreprocessorDefinitions>_CONSOLE;%(PreprocessorDefinitions)</PreprocessorDefinitions>
-      <UndefinePreprocessorDefinitions>UNICODE;%(UndefinePreprocessorDefinitions)</UndefinePreprocessorDefinitions>
+      <UndefinePreprocessorDefinitions>%(UndefinePreprocessorDefinitions)</UndefinePreprocessorDefinitions>
+      <WarningLevel>Level2</WarningLevel>
+      <TreatWarningAsError>true</TreatWarningAsError>
     </ClCompile>
     <ResourceCompile />
     <Link>
@@ -87,7 +89,9 @@ 
     <ClCompile>
       <AdditionalIncludeDirectories>..\compat;$(TAP_WINDOWS_HOME)/include;$(OPENSSL_HOME)/include;$(LZO_HOME)/include;$(PKCS11H_HOME)/include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
       <PreprocessorDefinitions>_CONSOLE;%(PreprocessorDefinitions)</PreprocessorDefinitions>
-      <UndefinePreprocessorDefinitions>UNICODE;%(UndefinePreprocessorDefinitions)</UndefinePreprocessorDefinitions>
+      <UndefinePreprocessorDefinitions>%(UndefinePreprocessorDefinitions)</UndefinePreprocessorDefinitions>
+      <WarningLevel>Level2</WarningLevel>
+      <TreatWarningAsError>true</TreatWarningAsError>
     </ClCompile>
     <ResourceCompile />
     <Link>
@@ -100,7 +104,9 @@ 
     <ClCompile>
       <AdditionalIncludeDirectories>..\compat;$(TAP_WINDOWS_HOME)/include;$(OPENSSL_HOME)/include;$(LZO_HOME)/include;$(PKCS11H_HOME)/include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
       <PreprocessorDefinitions>_CONSOLE;%(PreprocessorDefinitions)</PreprocessorDefinitions>
-      <UndefinePreprocessorDefinitions>UNICODE;%(UndefinePreprocessorDefinitions)</UndefinePreprocessorDefinitions>
+      <UndefinePreprocessorDefinitions>%(UndefinePreprocessorDefinitions)</UndefinePreprocessorDefinitions>
+      <WarningLevel>Level2</WarningLevel>
+      <TreatWarningAsError>true</TreatWarningAsError>
     </ClCompile>
     <ResourceCompile />
     <Link>
@@ -113,7 +119,9 @@ 
     <ClCompile>
       <AdditionalIncludeDirectories>..\compat;$(TAP_WINDOWS_HOME)/include;$(OPENSSL_HOME)/include;$(LZO_HOME)/include;$(PKCS11H_HOME)/include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
       <PreprocessorDefinitions>_CONSOLE;%(PreprocessorDefinitions)</PreprocessorDefinitions>
-      <UndefinePreprocessorDefinitions>UNICODE;%(UndefinePreprocessorDefinitions)</UndefinePreprocessorDefinitions>
+      <UndefinePreprocessorDefinitions>%(UndefinePreprocessorDefinitions)</UndefinePreprocessorDefinitions>
+      <WarningLevel>Level2</WarningLevel>
+      <TreatWarningAsError>true</TreatWarningAsError>
     </ClCompile>
     <ResourceCompile />
     <Link>
@@ -307,4 +315,4 @@ 
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
   <ImportGroup Label="ExtensionTargets">
   </ImportGroup>
-</Project>
+</Project>
\ No newline at end of file
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 16f9da6a..3484f7d4 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -7276,7 +7276,7 @@  add_option(struct options *options,
 #ifdef _WIN32
         VERIFY_PERMISSION(OPT_P_GENERAL);
         HANDLE process = GetCurrentProcess();
-        HANDLE handle = (HANDLE) atoi(p[1]);
+        HANDLE handle = (HANDLE) atoll(p[1]);
         if (!DuplicateHandle(process, handle, process, &options->msg_channel, 0,
                              FALSE, DUPLICATE_CLOSE_SOURCE | DUPLICATE_SAME_ACCESS))
         {
diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index 9ff36359..b57da5dd 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -2833,7 +2833,7 @@  get_default_gateway_ipv6(struct route_ipv6_gateway_info *rgi6,
         DestinationAddress.Ipv6.sin6_addr = *dest;
     }
 
-    status = GetBestInterfaceEx( &DestinationAddress, &BestIfIndex );
+    status = GetBestInterfaceEx( (struct sockaddr *)&DestinationAddress, &BestIfIndex );
 
     if (status != NO_ERROR)
     {
diff --git a/src/openvpn/ssl_backend.h b/src/openvpn/ssl_backend.h
index a1770bd4..5e1055b6 100644
--- a/src/openvpn/ssl_backend.h
+++ b/src/openvpn/ssl_backend.h
@@ -534,7 +534,7 @@  void print_details(struct key_state_ssl *ks_ssl, const char *prefix);
 void
 show_available_tls_ciphers_list(const char *cipher_list,
                                 const char *tls_cert_profile,
-                                bool tls13);
+                                const bool tls13);
 
 /*
  * Show the available elliptic curves in the crypto library
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 0e6dfe72..d5ca3ce3 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -233,10 +233,11 @@  do_set_mtu_service(const struct tuntap *tt, const short family, const int mtu)
             sizeof(set_mtu_message_t),
             0
         },
-        .iface = {.index = tt->adapter_index,.name = tt->actual_name },
+        .iface = {.index = tt->adapter_index},
         .mtu = mtu,
         .family = family
     };
+    strncpynt(mtu_msg.iface.name, tt->actual_name, sizeof(mtu_msg.iface.name));
 
     if (!send_msg_iservice(pipe, &mtu_msg, sizeof(mtu_msg), &ack, "Set_mtu"))
     {
@@ -889,7 +890,7 @@  add_route_connected_v6_net(struct tuntap *tt,
 }
 
 void
-delete_route_connected_v6_net(struct tuntap *tt,
+delete_route_connected_v6_net(const struct tuntap *tt,
                               const struct env_set *es)
 {
     struct route_ipv6 r6;