| Message ID | 20200625083745.211-1-lstipakov@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel,v2] msvc: fix various level2 warnings | expand |
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.
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;