[Openvpn-devel,v2] msvc: fix various level2 warnings
Commit Message
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
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
>> @@ -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
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.
@@ -15,7 +15,6 @@
<PreprocessorDefinitions>_DEBUG;%(PreprocessorDefinitions)</PreprocessorDefinitions>
<RuntimeLibrary>MultiThreadedDebugDLL</RuntimeLibrary>
<DebugInformationFormat>EditAndContinue</DebugInformationFormat>
- <MinimalRebuild>true</MinimalRebuild>
</ClCompile>
</ItemDefinitionGroup>
<ItemGroup />
@@ -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
*/
@@ -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++)
{
@@ -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 */
@@ -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);
@@ -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
@@ -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))
{
@@ -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)
{
@@ -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
@@ -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;