| Message ID | 20171010231130.6832-9-simon@rozman.si |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | [Openvpn-devel,01/13] snwprintf() => _snwprintf() | expand |
Hi, On Wed, Oct 11, 2017 at 01:11:26AM +0200, simon@rozman.si wrote: > From: Simon Rozman <simon@rozman.si> > > --- > src/openvpn/block_dns.c | 2 +- > src/openvpnserv/automatic.c | 2 +- > src/openvpnserv/common.c | 2 +- > src/openvpnserv/interactive.c | 4 ++-- > src/openvpnserv/validate.c | 2 +- > 5 files changed, 6 insertions(+), 6 deletions(-) So... trying to make sense of this. > diff --git a/src/openvpn/block_dns.c b/src/openvpn/block_dns.c > index d43cbcf..f88ba2c 100644 > --- a/src/openvpn/block_dns.c > +++ b/src/openvpn/block_dns.c > @@ -370,7 +370,7 @@ get_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY family) > } > return ipiface.Metric; > } > - return -err; > + return -(int)err; > } This, I can somewhat agree to, as "err" is an unsigned type so there might be a hidden integer overflow if it happens to be "large". Which it won't be, but still. > > /* > diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c > index 4123d0f..6c39aaa 100644 > --- a/src/openvpnserv/automatic.c > +++ b/src/openvpnserv/automatic.c > @@ -155,7 +155,7 @@ match(const WIN32_FIND_DATA *find, LPCTSTR ext) > * Modify the extension on a filename. > */ > static bool > -modext(LPTSTR dest, int size, LPCTSTR src, LPCTSTR newext) > +modext(LPTSTR dest, size_t size, LPCTSTR src, LPCTSTR newext) > { > int i; Not sure why this is needed? The code verifies that size is ">0", so a signed variable is ok here. > > diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c > index b8b817b..7901fd6 100644 > --- a/src/openvpnserv/common.c > +++ b/src/openvpnserv/common.c > @@ -36,7 +36,7 @@ openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list arglist) > len = _vsntprintf(str, size, format, arglist); > str[size - 1] = 0; > } > - return (len >= 0 && len < size); > + return (len >= 0 && (size_t)len < size); > } This is, if I understand right, because "len < size" will implicitly cast size to (int), causing integer overflow if size is too big for a signed int? > int > openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...) > diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c > index 8d94197..96e0de0 100644 > --- a/src/openvpnserv/interactive.c > +++ b/src/openvpnserv/interactive.c > @@ -188,7 +188,7 @@ typedef enum { > static DWORD > AsyncPipeOp(async_op_t op, HANDLE pipe, LPVOID buffer, DWORD size, DWORD count, LPHANDLE events) > { > - int i; > + DWORD i; ... and this totally escapes me. Why would an "int" suddenly be no longer a legal loop variable...? > BOOL success; > HANDLE io_event; > DWORD res, bytes = 0; > @@ -1061,7 +1061,7 @@ RegisterDNS(LPVOID unused) > { ipcfg, L"ipconfig /flushdns", timeout }, > { ipcfg, L"ipconfig /registerdns", timeout }, > }; > - int ncmds = sizeof(cmds) / sizeof(cmds[0]); > + DWORD ncmds = sizeof(cmds) / sizeof(cmds[0]); Same thing here... > index 7a2d0e3..5422d33 100644 > --- a/src/openvpnserv/validate.c > +++ b/src/openvpnserv/validate.c > @@ -298,7 +298,7 @@ IsUserInGroup(PSID sid, const PTOKEN_GROUPS token_groups, const WCHAR *group_nam > break; > } > /* If a match is already found, ret == TRUE and the loop is skipped */ > - for (int i = 0; i < nread && !ret; ++i) > + for (DWORD i = 0; i < nread && !ret; ++i) ... and here. Consider me not convinced... what exactly are the warnings MSVC spits out here? gert
Hi, > > index 7a2d0e3..5422d33 100644 > > --- a/src/openvpnserv/validate.c > > +++ b/src/openvpnserv/validate.c > > @@ -298,7 +298,7 @@ IsUserInGroup(PSID sid, const PTOKEN_GROUPS > token_groups, const WCHAR *group_nam > > break; > > } > > /* If a match is already found, ret == TRUE and the loop is > skipped */ > > - for (int i = 0; i < nread && !ret; ++i) > > + for (DWORD i = 0; i < nread && !ret; ++i) > > ... and here. > > > Consider me not convinced... > > what exactly are the warnings MSVC spits out here? > I think these are needed to silence signed/unsigned comparison warnings (== is ok, but < and > would warn). gcc also would warn if -Wextra or -Wsign-compare is used. Simon may have more to say. Selva <div dir="ltr">Hi,<br><div class="gmail_extra"><br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span><br> > index 7a2d0e3..5422d33 100644<br> > --- a/src/openvpnserv/validate.c<br> > +++ b/src/openvpnserv/validate.c<br> > @@ -298,7 +298,7 @@ IsUserInGroup(PSID sid, const PTOKEN_GROUPS token_groups, const WCHAR *group_nam<br> > break;<br> > }<br> > /* If a match is already found, ret == TRUE and the loop is skipped */<br> > - for (int i = 0; i < nread && !ret; ++i)<br> > + for (DWORD i = 0; i < nread && !ret; ++i)<br> <br> </span>... and here.<br> <br> <br> Consider me not convinced...<br> <br> what exactly are the warnings MSVC spits out here?<br></blockquote><div><br></div><div>I think these are needed to silence signed/unsigned comparison warnings</div><div>(== is ok, but < and > would warn). gcc also would warn if -Wextra or </div><div>-Wsign-compare is used.</div><div><br></div><div>Simon may have more to say.</div><div><br></div><div>Selva</div></div></div></div> ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On 05/11/17 20:20, Gert Doering wrote: >> -modext(LPTSTR dest, int size, LPCTSTR src, LPCTSTR newext) >> +modext(LPTSTR dest, size_t size, LPCTSTR src, LPCTSTR newext) >> { >> int i; > Not sure why this is needed? The code verifies that size is ">0", so > a signed variable is ok here. I would label this more as "use size_t for size/length related variables". So I read this more as a coding style change than anything else. From a general point of view, I can agree to such changes. But not convinced that it is strictly needed everywhere. If it silences compiler warnings and don't add "anything else" (behavioural changes or warnings) elsewhere (both code and platform wise), then I'm fairly positive to it.
Hi, Let me explain all proposed modifications case-by-case below... > > diff --git a/src/openvpn/block_dns.c b/src/openvpn/block_dns.c index > > d43cbcf..f88ba2c 100644 > > --- a/src/openvpn/block_dns.c > > +++ b/src/openvpn/block_dns.c > > @@ -370,7 +370,7 @@ get_interface_metric(const NET_IFINDEX index, > const ADDRESS_FAMILY family) > > } > > return ipiface.Metric; > > } > > - return -err; > > + return -(int)err; > > } > > This, I can somewhat agree to, as "err" is an unsigned type so there might be > a hidden integer overflow if it happens to be "large". Which it won't be, but > still. Adding the `(int)` resolves the warning C4146: unary minus operator applied to unsigned type, result still unsigned. https://msdn.microsoft.com/en-us/library/4kh09110.aspx It doesn't change much, but it shut-ups one compiler warning. > > /* > > diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c > > index 4123d0f..6c39aaa 100644 > > --- a/src/openvpnserv/automatic.c > > +++ b/src/openvpnserv/automatic.c > > @@ -155,7 +155,7 @@ match(const WIN32_FIND_DATA *find, LPCTSTR > ext) > > * Modify the extension on a filename. > > */ > > static bool > > -modext(LPTSTR dest, int size, LPCTSTR src, LPCTSTR newext) > > +modext(LPTSTR dest, size_t size, LPCTSTR src, LPCTSTR newext) > > { > > int i; > > Not sure why this is needed? The code verifies that size is ">0", so a signed > variable is ok here. warning C4018: '<=': signed/unsigned mismatch (in Win32 builds twice) https://msdn.microsoft.com/en-us/library/y92ktdf2.aspx Parameter size is compared to the result of _tcslen() which is size_t. Hence, you had a signed int and unsigned size_t comparison. However, modext() is called exactly at one location in the code: if (!modext(log_file, _countof(log_file), find_obj.cFileName, TEXT("log"))) ... Observe, the size parameter is _countof(log_file) which is size_t, converted to int to be passed as parameter to modext(), and finally compared against _tcslen() which is size_t in modext(). In other words: it had an orange, converted it to an apple, and finally compared it to another orange. > > diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c > index > > b8b817b..7901fd6 100644 > > --- a/src/openvpnserv/common.c > > +++ b/src/openvpnserv/common.c > > @@ -36,7 +36,7 @@ openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR > format, va_list arglist) > > len = _vsntprintf(str, size, format, arglist); > > str[size - 1] = 0; > > } > > - return (len >= 0 && len < size); > > + return (len >= 0 && (size_t)len < size); > > } > > This is, if I understand right, because "len < size" will implicitly cast size to > (int), causing integer overflow if size is too big for a signed int? warning C4018: '<': signed/unsigned mismatch (in Win32 builds). The MSVC is not happy if it needs to decide between signed or unsigned comparison itself. So the programmer needs to cast either side to match the signed/unsigned manually. The `len >= 0` assures that len is always positive or zero before `len < size` is evaluated. Therefore, it is no harm to cast len to size_t to force unsigned comparison. > > int > > openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...) diff > > --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c > > index 8d94197..96e0de0 100644 > > --- a/src/openvpnserv/interactive.c > > +++ b/src/openvpnserv/interactive.c > > @@ -188,7 +188,7 @@ typedef enum { > > static DWORD > > AsyncPipeOp(async_op_t op, HANDLE pipe, LPVOID buffer, DWORD size, > > DWORD count, LPHANDLE events) { > > - int i; > > + DWORD i; > > ... and this totally escapes me. Why would an "int" suddenly be no longer a > legal loop variable...? This one is my favourite. :) I hope I'll manage to explain it (I am not a native English speaker. Please, be patient.) This one is not just about C4018 again. Although, the `i < count` in `for (i = 0; i < count; i++)` does raise the C4018. That's how it got my attention. This particular loop variable i was used in one loop only. In that particular loop, it iterates on [0...count) interval. The lower bound is zero (any scalar zero). The upper bound is a DWORD. Thus, the interval is essentially a [DWORD...DWORD) type. So the most natural selection for the loop variable is DWORD then. This avoids all 32/64-bit issues, signed/unsigned comparison warnings etc. When you need to iterate from 'A' to 'Z', the most natural selection for loop variable is char; when you count oranges, the appropriate loop variable is orange... > > BOOL success; > > HANDLE io_event; > > DWORD res, bytes = 0; > > @@ -1061,7 +1061,7 @@ RegisterDNS(LPVOID unused) > > { ipcfg, L"ipconfig /flushdns", timeout }, > > { ipcfg, L"ipconfig /registerdns", timeout }, > > }; > > - int ncmds = sizeof(cmds) / sizeof(cmds[0]); > > + DWORD ncmds = sizeof(cmds) / sizeof(cmds[0]); > > Same thing here... Exactly. This one's the other way around: Here, the original code used `DWORD i` as the loop variable. But the upper bound was `int ncmds`. Since ncmds is a school example for size_t, the unsigned DWORD is still a better choice than int. Although, if I was coding this, I'd change everything to size_t. :) > > index 7a2d0e3..5422d33 100644 > > --- a/src/openvpnserv/validate.c > > +++ b/src/openvpnserv/validate.c > > @@ -298,7 +298,7 @@ IsUserInGroup(PSID sid, const PTOKEN_GROUPS > token_groups, const WCHAR *group_nam > > break; > > } > > /* If a match is already found, ret == TRUE and the loop is skipped */ > > - for (int i = 0; i < nread && !ret; ++i) > > + for (DWORD i = 0; i < nread && !ret; ++i) > > ... and here. Again, the nread is number of members of a security group - a DWORD (Windows API really likes this type). It should be iterated using a DWORD or face the dreaded signed/unsigned comparison warning again. > Consider me not convinced... I hope you might reconsider again. :) > what exactly are the warnings MSVC spits out here? As described above. MSVC for OpenVPN projects is set to Warning level 3. On the scale of 0 to 4 Microsoft calls this level the "production quality" level. Apart from those signed/unsigned warnings, there were some 32/64-bit warnings reported at 64-bit MSVC builds and are addressed by "[PATCH 12/13] Memory size arithmetic reviewed according to 64-bit MSVC complaints". All that said, the OpenVPN Interactive Service code is a very fine example of code that - given the number of lines - produces very low volume of warnings. Why not address and fix them, when we're so close? While most of the MSVC compiler warnings are false-alarms, one out of many does represent a potential threat. But you don't know which one that is. Therefore it is best to address them all. :) Just my personal philosophy, using which, I keep the helpdesk guys out of my office quite efficiently. Best regards, Simon ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, I understand the intent here and its nice to get rid of compiler warnings, when it makes sense. But be careful while silencing by just a cast. On Sun, Nov 5, 2017 at 5:06 PM, Simon Rozman <simon@rozman.si> wrote: > > Hi, > > Let me explain all proposed modifications case-by-case below... > > > > diff --git a/src/openvpn/block_dns.c b/src/openvpn/block_dns.c index > > > d43cbcf..f88ba2c 100644 > > > --- a/src/openvpn/block_dns.c > > > +++ b/src/openvpn/block_dns.c > > > @@ -370,7 +370,7 @@ get_interface_metric(const NET_IFINDEX index, > > const ADDRESS_FAMILY family) > > > } > > > return ipiface.Metric; > > > } > > > - return -err; > > > + return -(int)err; > > > } > > > > This, I can somewhat agree to, as "err" is an unsigned type so there might be > > a hidden integer overflow if it happens to be "large". Which it won't be, but > > still. > > Adding the `(int)` resolves the warning C4146: unary minus operator > applied to unsigned type, result still unsigned. > https://msdn.microsoft.com/en-us/library/4kh09110.aspx > > It doesn't change much, but it shut-ups one compiler warning. The original code could be argued to be logically wrong (though perfectly legal C) if err becomes larger than a signed int. This change does not fix that. So what's the point? As an example, consider this err = 2147483649 <(214)%20748-3649> (2 more than INT_MAX) Original will return 2147483647 <(214)%20748-3647> (not a -ve int) The new code will return the same 2147483647 <(214)%20748-3647> That is, the return value is +ve int which the caller will wrongly interpret as valid result. This is hypothetical as Windows system err codes do not get that large. But then the original is as good as the replacement except for a C++-trained compiler being silenced. While most of the MSVC compiler warnings are false-alarms, one out of many > does represent a potential threat. But you don't know which one that is. > Therefore it is best to address them all. :) > To repeat, cast does not eliminate a potential for error, it just just hides it. Selva <div dir="ltr">Hi,<br><br>I understand the intent here and its nice to get rid of compiler warnings, when<br>it makes sense. But be careful while silencing by just a cast.<br><br>On Sun, Nov 5, 2017 at 5:06 PM, Simon Rozman <<a href="mailto:simon@rozman.si" target="_blank">simon@rozman.si</a>> wrote:<br>><br>> Hi,<br>><br>> Let me explain all proposed modifications case-by-case below...<br>><br>> > > diff --git a/src/openvpn/block_dns.c b/src/openvpn/block_dns.c index<br>> > > d43cbcf..f88ba2c 100644<br>> > > --- a/src/openvpn/block_dns.c<br>> > > +++ b/src/openvpn/block_dns.c<br>> > > @@ -370,7 +370,7 @@ get_interface_metric(const NET_IFINDEX index,<br>> > const ADDRESS_FAMILY family)<br>> > > }<br>> > > return ipiface.Metric;<br>> > > }<br>> > > - return -err;<br>> > > + return -(int)err;<br>> > > }<br>> ><br>> > This, I can somewhat agree to, as "err" is an unsigned type so there might be<br>> > a hidden integer overflow if it happens to be "large". Which it won't be, but<br>> > still.<br>><br>> Adding the `(int)` resolves the warning C4146: unary minus operator<div>> applied to unsigned type, result still unsigned.<br>> <a href="https://msdn.microsoft.com/en-us/library/4kh09110.aspx" target="_blank">https://msdn.microsoft.com/en-<wbr>us/library/4kh09110.aspx</a><br>><br>> It doesn't change much, but it shut-ups one compiler warning.<br><br>The original code could be argued to be logically wrong (though<br>perfectly legal C) if err becomes larger than a signed int. This change</div><div>does not fix that. So what's the point?<br><br>As an example, consider this<br><br>err = <a href="tel:(214)%20748-3649" value="+12147483649" target="_blank">2147483649</a> (2 more than INT_MAX)<br>Original will return <a href="tel:(214)%20748-3647" value="+12147483647" target="_blank">2147483647</a> (not a -ve int)<br>The new code will return the same <a href="tel:(214)%20748-3647" value="+12147483647" target="_blank">2147483647</a><br>That is, the return value is +ve int which the caller will wrongly<br>interpret as valid result.<br><br>This is hypothetical as Windows system err codes do not get that large.<br>But then the original is as good as the replacement except for a <br>C++-trained compiler being silenced.<div><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">While most of the MSVC compiler warnings are false-alarms, one out of many does represent a potential threat. But you don't know which one that is. Therefore it is best to address them all. :) <br></blockquote><div><br></div><div>To repeat, cast does not eliminate a potential for error, it just just hides it.</div><div><br></div>Selva<br></div></div></div> ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, > > > > diff --git a/src/openvpn/block_dns.c b/src/openvpn/block_dns.c > > > > index d43cbcf..f88ba2c 100644 > > > > --- a/src/openvpn/block_dns.c > > > > +++ b/src/openvpn/block_dns.c > > > > @@ -370,7 +370,7 @@ get_interface_metric(const NET_IFINDEX index, > > > const ADDRESS_FAMILY family) > > > > } > > > > return ipiface.Metric; > > > > } > > > > - return -err; > > > > + return -(int)err; > > > > } > > > > > > This, I can somewhat agree to, as "err" is an unsigned type so there > > > might be a hidden integer overflow if it happens to be "large". > > > Which it won't be, but still. > > > > Adding the `(int)` resolves the warning C4146: unary minus operator > > applied to unsigned type, result still unsigned. > > https://msdn.microsoft.com/en-us/library/4kh09110.aspx > > > > It doesn't change much, but it shut-ups one compiler warning. > > The original code could be argued to be logically wrong (though perfectly > legal C) if err becomes larger than a signed int. This change does not fix that. > So what's the point? > > As an example, consider this > > err = 2147483649 (2 more than INT_MAX) > Original will return 2147483647 (not a -ve int) The new code will return the > same 2147483647 That is, the return value is +ve int which the caller will > wrongly interpret as valid result. > > This is hypothetical as Windows system err codes do not get that large. > But then the original is as good as the replacement except for a > C++-trained compiler being silenced. I totally agree with your point, the original code might be logically wrong. If the interface metric is > INT_MAX (which ULONG theoretically could be), it'll get returned as a negative number => false-positive error condition. The get_interface_metric()also return error codes > INT_MAX as a positive numbers => invalid interface metric instead of error condition. As you described this case yourself. > To repeat, cast does not eliminate a potential for error, it just just hides it. True and I totally agree with you. I just didn't dare to rewrite the function more thoroughly. Can I, please? Option 1: Add an `if` to limit metric to INT_MAX, and an `if` to return all errors greater than INT_MAX simply as -1. Option 2: redefine function prototype to return error code unmodified, and metrics using an extra parameter passed by reference. I'd prefer option 1 after reviewing all get_interface_metrics() calls. None of them cares about the exact error code. They just test for negative value and set the metrics to -1. As a matter of fact, the get_interface_metrics() itself could be rewritten to return -1 as the metric for any error in the first place, thus making the error checks after each function call redundant, thus simplifying code. Best regards, Simon ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On Mon, Nov 06, 2017 at 08:35:48AM +0000, Simon Rozman wrote: > > To repeat, cast does not eliminate a potential for error, it just just hides it. > > True and I totally agree with you. I just didn't dare to rewrite the function more thoroughly. Can I, please? Refactoring is definitely welcome for master, and can be argued into 2.4 if it fixes things that need fixing... (And thanks for taking your time to explain the background for all these changes - I haven't had time today to do more than just read all the mails, but they were helpful - thanks to Selva, too) gert
Hi, On Mon, Nov 6, 2017 at 3:35 AM, Simon Rozman <simon@rozman.si> wrote: > > Hi, > > > > > > diff --git a/src/openvpn/block_dns.c b/src/openvpn/block_dns.c > > > > > index d43cbcf..f88ba2c 100644 > > > > > --- a/src/openvpn/block_dns.c > > > > > +++ b/src/openvpn/block_dns.c > > > > > @@ -370,7 +370,7 @@ get_interface_metric(const NET_IFINDEX index, > > > > const ADDRESS_FAMILY family) > > > > > } > > > > > return ipiface.Metric; > > > > > } > > > > > - return -err; > > > > > + return -(int)err; > > > > > } > > > > > > > > This, I can somewhat agree to, as "err" is an unsigned type so there > > > > might be a hidden integer overflow if it happens to be "large". > > > > Which it won't be, but still. > > > > > > Adding the `(int)` resolves the warning C4146: unary minus operator > > > applied to unsigned type, result still unsigned. > > > https://msdn.microsoft.com/en-us/library/4kh09110.aspx > > > > > > It doesn't change much, but it shut-ups one compiler warning. > > > > The original code could be argued to be logically wrong (though perfectly > > legal C) if err becomes larger than a signed int. This change does not fix that. > > So what's the point? > > > > As an example, consider this > > > > err = 2147483649 <(214)%20748-3649> (2 more than INT_MAX) > > Original will return 2147483647 <(214)%20748-3647> (not a -ve int) The new code will return the > > same 2147483647 <(214)%20748-3647> That is, the return value is +ve int which the caller will > > wrongly interpret as valid result. > > > > This is hypothetical as Windows system err codes do not get that large. > > But then the original is as good as the replacement except for a > > C++-trained compiler being silenced. > > I totally agree with your point, the original code might be logically wrong. If the interface metric is > INT_MAX (which ULONG theoretically could be), it'll get returned as a negative number => false-positive error condition. > > The get_interface_metric()also return error codes > INT_MAX as a positive numbers => invalid interface metric instead of error condition. As you described this case yourself. > > > To repeat, cast does not eliminate a potential for error, it just just hides it. > > True and I totally agree with you. I just didn't dare to rewrite the function more thoroughly. Can I, please? > > > Option 1: Add an `if` to limit metric to INT_MAX, and an `if` to return all errors greater than INT_MAX simply as -1. > > Option 2: redefine function prototype to return error code unmodified, and metrics using an extra parameter passed by reference. > > > I'd prefer option 1 after reviewing all get_interface_metrics() calls. None of them cares about the exact error code. They just test for negative value and set the metrics to -1. As a matter of fact, the get_interface_metrics() itself could be rewritten to return -1 as the metric for any error in the first place, thus making the error checks after each function call redundant, thus simplifying code. The best time to re-factor a function would be when a a new use case needs to change its semantics. Apart from the ill-chosen -err as a return value, currently it returns 0 if automatic metric is in use, making it impossible to use it as a generic function to find the current metric of an interface. In fact I've a pending patch where such a change would help. So, if you re-write this, consider some improvements: I think it belongs to win32.c (declaration in win32.h). Second, return the current metric even if Automatic metric is on (could indicate automatic metric needed in current use case by a boolean parameter, probably). In case of error, logging it and returning -1 should be good enough. Based on my tests, Windows does not permit setting a metric larger than INT_MAX, so a return value of signed int is fine and convenient for indicating error. Selva <div dir="ltr">Hi,<br><br>On Mon, Nov 6, 2017 at 3:35 AM, Simon Rozman <<a href="mailto:simon@rozman.si" target="_blank">simon@rozman.si</a>> wrote:<br>><br>> Hi,<br>><br>> > > > > diff --git a/src/openvpn/block_dns.c b/src/openvpn/block_dns.c<br>> > > > > index d43cbcf..f88ba2c 100644<br>> > > > > --- a/src/openvpn/block_dns.c<br>> > > > > +++ b/src/openvpn/block_dns.c<br>> > > > > @@ -370,7 +370,7 @@ get_interface_metric(const NET_IFINDEX index,<br>> > > > const ADDRESS_FAMILY family)<br>> > > > > }<br>> > > > > return ipiface.Metric;<br>> > > > > }<br>> > > > > - return -err;<br>> > > > > + return -(int)err;<br>> > > > > }<br>> > > ><br>> > > > This, I can somewhat agree to, as "err" is an unsigned type so there<br>> > > > might be a hidden integer overflow if it happens to be "large".<br>> > > > Which it won't be, but still.<br>> > ><br>> > > Adding the `(int)` resolves the warning C4146: unary minus operator<br>> > > applied to unsigned type, result still unsigned.<br>> > > <a href="https://msdn.microsoft.com/en-us/library/4kh09110.aspx" target="_blank">https://msdn.microsoft.com/en-<wbr>us/library/4kh09110.aspx</a><br>> > ><br>> > > It doesn't change much, but it shut-ups one compiler warning.<br>> ><br>> > The original code could be argued to be logically wrong (though perfectly<br>> > legal C) if err becomes larger than a signed int. This change does not fix that.<br>> > So what's the point?<br>> ><br>> > As an example, consider this<br>> ><br>> > err = <a href="tel:(214)%20748-3649" value="+12147483649" target="_blank">2147483649</a> (2 more than INT_MAX)<br>> > Original will return <a href="tel:(214)%20748-3647" value="+12147483647" target="_blank">2147483647</a> (not a -ve int) The new code will return the<br>> > same <a href="tel:(214)%20748-3647" value="+12147483647" target="_blank">2147483647</a> That is, the return value is +ve int which the caller will<br>> > wrongly interpret as valid result.<br>> ><br>> > This is hypothetical as Windows system err codes do not get that large.<br>> > But then the original is as good as the replacement except for a<br>> > C++-trained compiler being silenced.<br>><br>> I totally agree with your point, the original code might be logically wrong. If the interface metric is > INT_MAX (which ULONG theoretically could be), it'll get returned as a negative number => false-positive error condition.<br>><br>> The get_interface_metric()also return error codes > INT_MAX as a positive numbers => invalid interface metric instead of error condition. As you described this case yourself.<br>><br>> > To repeat, cast does not eliminate a potential for error, it just just hides it.<br>><br>> True and I totally agree with you. I just didn't dare to rewrite the function more thoroughly. Can I, please?<br>><br>><br>> Option 1: Add an `if` to limit metric to INT_MAX, and an `if` to return all errors greater than INT_MAX simply as -1.<br>><br>> Option 2: redefine function prototype to return error code unmodified, and metrics using an extra parameter passed by reference.<br>><br>><br>> I'd prefer option 1 after reviewing all get_interface_metrics() calls. None of them cares about the exact error code. They just test for negative value and set the metrics to -1. As a matter of fact, the get_interface_metrics() itself could be rewritten to return -1 as the metric for any error in the first place, thus making the error checks after each function call redundant, thus simplifying code.<br><br>The best time to re-factor a function would be when a a new use case<div>needs to change its semantics. Apart from the ill-chosen -err as a return</div><div>value, currently it returns 0 if automatic metric is in use, making it impossible</div><div>to use it as a generic function to find the current metric of an interface.<div><div><br></div><div>In fact I've a pending patch where such a change would help.</div><div><br></div><div>So, if you re-write this, consider some improvements: I think it belongs to</div><div>win32.c (declaration in win32.h). Second, return the current metric </div><div>even if Automatic metric is on (could indicate automatic metric needed in</div><div>current use case by a boolean parameter, probably). In case of error,</div><div>logging it and returning -1 should be good enough.</div><div><br></div><div>Based on my tests, Windows does not permit setting a metric</div><div>larger than INT_MAX, so a return value of signed int is fine and</div><div>convenient for indicating error.</div><div><br>Selva<br></div></div></div></div> ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, > The best time to re-factor a function would be when a a new use case needs > to change its semantics. Apart from the ill-chosen -err as a return value, > currently it returns 0 if automatic metric is in use, making it impossible to use > it as a generic function to find the current metric of an interface. > > In fact I've a pending patch where such a change would help. > > So, if you re-write this, consider some improvements: I think it belongs to > win32.c (declaration in win32.h). Second, return the current metric even if > Automatic metric is on (could indicate automatic metric needed in current > use case by a boolean parameter, probably). In case of error, logging it and > returning -1 should be good enough. > > Based on my tests, Windows does not permit setting a metric larger than > INT_MAX, so a return value of signed int is fine and convenient for indicating > error. I shall give it a look after the Hackathon. Best regards, Simon ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On Wed, Nov 08, 2017 at 06:46:53PM +0000, Simon Rozman wrote: > > The best time to re-factor a function would be when a a new use case needs > > to change its semantics. Apart from the ill-chosen -err as a return value, > > currently it returns 0 if automatic metric is in use, making it impossible to use > > it as a generic function to find the current metric of an interface. > > > > In fact I've a pending patch where such a change would help. [..] > I shall give it a look after the Hackathon. "ping"? This seems to be the only hunk left from the MSVC correction series (as far as .c/.h files are concerned, not .proj) gert
Hi, > On Wed, Nov 08, 2017 at 06:46:53PM +0000, Simon Rozman wrote: > > > The best time to re-factor a function would be when a a new use > > > case needs to change its semantics. Apart from the ill-chosen -err > > > as a return value, currently it returns 0 if automatic metric is in > > > use, making it impossible to use it as a generic function to find the current > metric of an interface. > > > > > > In fact I've a pending patch where such a change would help. > [..] > > I shall give it a look after the Hackathon. > > "ping"? > > This seems to be the only hunk left from the MSVC correction series (as far > as .c/.h files are concerned, not .proj) > I really appreciate your "ping" Gert. I totally forgot about this one and have now flagged this thread so I shall finish it in the following weeks. The get_interface_metric() function should get a more thorough rewrite than just a compiler warning shut-up. So the patch will probably get divided in two - the simple signed/unsigned fixes and get_interface_metric() redesign. Best regards, Simon ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi Simon, On Tue, Dec 5, 2017 at 4:44 AM, Simon Rozman <simon@rozman.si> wrote: > Hi, > >> On Wed, Nov 08, 2017 at 06:46:53PM +0000, Simon Rozman wrote: >> > > The best time to re-factor a function would be when a a new use >> > > case needs to change its semantics. Apart from the ill-chosen -err >> > > as a return value, currently it returns 0 if automatic metric is in >> > > use, making it impossible to use it as a generic function to find the current >> metric of an interface. >> > > >> > > In fact I've a pending patch where such a change would help. >> [..] >> > I shall give it a look after the Hackathon. >> >> "ping"? >> >> This seems to be the only hunk left from the MSVC correction series (as far >> as .c/.h files are concerned, not .proj) >> > > I really appreciate your "ping" Gert. I totally forgot about this one and have now flagged this thread so I shall finish it in the following weeks. > > The get_interface_metric() function should get a more thorough rewrite than just a compiler warning shut-up. So the patch will probably get divided in two - the simple signed/unsigned fixes and get_interface_metric() redesign. For the latter, I had "re-invented" the get_interface_metric function for the pending "use lowest metric interface when multiple interfaces match a route" patch. Obviously, its better to refactor this one and use it there, so I just copied the implementation and submitted a patch to do so. Unlike I had thought earlier, this has to stay in block_dns.h for ease of sharing with the service. Could you please take a look and see whether it addresses MSVC's concerns among other things? Thanks, Selva ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, > > The get_interface_metric() function should get a more thorough rewrite > than just a compiler warning shut-up. So the patch will probably get divided > in two - the simple signed/unsigned fixes and get_interface_metric() > redesign. > > For the latter, I had "re-invented" the get_interface_metric function for the > pending "use lowest metric interface when multiple interfaces match a > route" patch. > > Obviously, its better to refactor this one and use it there, so I just copied the > implementation and submitted a patch to do so. Unlike I had thought earlier, > this has to stay in block_dns.h for ease of sharing with the service. > > Could you please take a look and see whether it addresses MSVC's concerns > among other things? I have tested and am confirming MSVC is happy with the block_dns.c now. Should your patch be merged, I shall rebase the "[PATCH 09/13] Signed/unsigned warnings of MSVC resolved" to the new master and deliver the next version. Best regards, Simon ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On Wed, Dec 6, 2017 at 12:10 PM, Simon Rozman <simon@rozman.si> wrote: > Hi, > >> > The get_interface_metric() function should get a more thorough rewrite >> than just a compiler warning shut-up. So the patch will probably get divided >> in two - the simple signed/unsigned fixes and get_interface_metric() >> redesign. >> >> For the latter, I had "re-invented" the get_interface_metric function for the >> pending "use lowest metric interface when multiple interfaces match a >> route" patch. >> >> Obviously, its better to refactor this one and use it there, so I just copied the >> implementation and submitted a patch to do so. Unlike I had thought earlier, >> this has to stay in block_dns.h for ease of sharing with the service. >> >> Could you please take a look and see whether it addresses MSVC's concerns >> among other things? > > I have tested and am confirming MSVC is happy with the block_dns.c now. > > Should your patch be merged, I shall rebase the "[PATCH 09/13] Signed/unsigned warnings of MSVC resolved" to the new master and deliver the next version. Yes, if you can review and ack/nak it :) Selva ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, > > Should your patch be merged, I shall rebase the "[PATCH 09/13] > Signed/unsigned warnings of MSVC resolved" to the new master and deliver > the next version. > > Yes, if you can review and ack/nak it :) You mean Gert to ack/nak it? I don't believe I have earned enough reputation to do it yet. However, I did stare-review your code: - It does not introduce any new Windows API calls it has not used before. - It compiles fine. Regards, Simon ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On Thu, Dec 07, 2017 at 06:41:03AM +0000, Simon Rozman wrote: > > > Should your patch be merged, I shall rebase the "[PATCH 09/13] > > Signed/unsigned warnings of MSVC resolved" to the new master and deliver > > the next version. > > > > Yes, if you can review and ack/nak it :) > > You mean Gert to ack/nak it? I don't believe I have earned enough reputation to do it yet. Formally we have no really clear line on "who can do ACK/NAK" (and this has caused a bit of friction in the past, my fault, apologies). I tend to see ACKs as "I have stared at the code, ran some tests, can't see anything bad in the code and the change is useful" - but usually, David or I do our own review and tests before actually merging. Depending on the "track record" of the one sending that ACK, it might be a very loose review, or very thorough :-) (like, when Steffan ACKs a crypto patch, all I do is "does it compile and pass t_client test?", but I do not try to understand crypto library API nuances...). Sometimes I find things in post-ACK-review that reopen the discussion, though :-) NAKs are maybe even more important - "watch out, this code might break under circumstances x, y, z!" - while the reviewer only considered cases a, b and c... So - feel free to comment on patches you feel qualified to review! > However, I did stare-review your code: > - It does not introduce any new Windows API calls it has not used before. > - It compiles fine. Good enough for a start :-) gert
Simon Rozman wrote: > However, I did stare-review your code: > - It does not introduce any new Windows API calls it has not used before. > - It compiles fine. It also builds fine here with cl v19.11. But using clang-cl v5, I'm getting: In file included from src/openvpn/argv.c:36: ./src/openvpn/syshead.h(368,13) : error: typedef redefinition with different types ('int' vs 'enum MIB_TCP_STATE') typedef int MIB_TCP_STATE; ^ f:\ProgramFiler-x86\WindowsKits\Include\10.0.15063.0\shared\tcpmib.h(56,3) : note: previous definition is here } MIB_TCP_STATE; ^ 1 error generated. I fail to why I don't get this error with MSVC. But the lines in syshead.h should simply not assume MinGW is the only compiler on Windows. Line 365: #ifdef _WIN32 /* Missing declarations for old-school MinGW32 or MinGW-w64 v1.x. */ #if defined(__MINGW32__) && !(defined(__MINGW64_VERSION_MAJOR) && __MINGW64_VERSION_MAJOR < 2) typedef int MIB_TCP_STATE; #endif
Hi, On Thu, Dec 7, 2017 at 12:32 PM, Gisle Vanem <gisle.vanem@gmail.com> wrote: > Simon Rozman wrote: > >> However, I did stare-review your code: >> - It does not introduce any new Windows API calls it has not used before. >> - It compiles fine. > > It also builds fine here with cl v19.11. > But using clang-cl v5, I'm getting: > In file included from src/openvpn/argv.c:36: > ./src/openvpn/syshead.h(368,13) : error: typedef redefinition with > different types ('int' vs 'enum MIB_TCP_STATE') > typedef int MIB_TCP_STATE; > ^ > f:\ProgramFiler-x86\WindowsKits\Include\10.0.15063.0\shared\tcpmib.h(56,3) > : note: previous definition is here > } MIB_TCP_STATE; > ^ > 1 error generated. > > I fail to why I don't get this error with MSVC. > But the lines in syshead.h should simply not assume > MinGW is the only compiler on Windows. Line 365: > > #ifdef _WIN32 > /* Missing declarations for old-school MinGW32 or MinGW-w64 v1.x. > */ > #if defined(__MINGW32__) && !(defined(__MINGW64_VERSION_MAJOR) && > __MINGW64_VERSION_MAJOR < 2) > typedef int MIB_TCP_STATE; > #endif Are you referring to the patch under discussion or something else? The code you quote above is not in current master nor in 2.4. In fact that !(defined(__MINGW64_VERSION_MAJOR) &&__MINGW64_VERSION_MAJOR < 2 even contradicts the comment above it. Its likely not from the official codebase. Anyway, MIB_TCP_STATE is typedef-ed to int in syshead.h whenever _WIN32 is defined. We may not need it anymore, but it hasn't so far interfered with any builds. Indeed its an enum as per MSDN (vista+ addition?). If clang doesn't like passing an int as enum and is using Microsoft's headers, it could trip there even if MSVC allows it. Such a combination has never been tested before, I suppose. Selva ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On Wed, Dec 06, 2017 at 05:10:45PM +0000, Simon Rozman wrote: > > > The get_interface_metric() function should get a more thorough rewrite > > than just a compiler warning shut-up. So the patch will probably get divided > > in two - the simple signed/unsigned fixes and get_interface_metric() > > redesign. [..] > I have tested and am confirming MSVC is happy with the block_dns.c now. > > Should your patch be merged, I shall rebase the "[PATCH 09/13] Signed/unsigned warnings of MSVC resolved" to the new master and deliver the next version. I finally came around to merging the get_interface_metric() patch from Selva today, so I await your fixup patch of the remaining MSVC warnings :-) thanks, gert
Hi, > > I have tested and am confirming MSVC is happy with the block_dns.c now. > > > > Should your patch be merged, I shall rebase the "[PATCH 09/13] > Signed/unsigned warnings of MSVC resolved" to the new master and deliver > the next version. > > I finally came around to merging the get_interface_metric() patch from Selva > today, so I await your fixup patch of the remaining MSVC warnings :-) > > thanks, > > gert I have been insanely busy for the last few months and the OpenVPN contribution was one of the sacrifices I had to make. Please apologize for the inconvenience. After I get back to my-old-self, count my children and see if my wife still recognizes me, I shall address all the OpenVPN things. I have quite some messages flagged for a follow-up here. Including this thread. Best regards, Simon ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Il 02/02/2018 03:00, Simon Rozman ha scritto: > Hi, > >>> I have tested and am confirming MSVC is happy with the block_dns.c now. >>> >>> Should your patch be merged, I shall rebase the "[PATCH 09/13] >> Signed/unsigned warnings of MSVC resolved" to the new master and deliver >> the next version. >> >> I finally came around to merging the get_interface_metric() patch from > Selva >> today, so I await your fixup patch of the remaining MSVC warnings :-) >> >> thanks, >> >> gert > > I have been insanely busy for the last few months and the OpenVPN > contribution was one of the sacrifices I had to make. Please apologize for > the inconvenience. > > After I get back to my-old-self, count my children and see if my wife still > recognizes me, I shall address all the OpenVPN things. I have quite some > messages flagged for a follow-up here. Including this thread. > Hi Simon, Please take your time and recover. We can wait a bit more to get the old Simon back :).
diff --git a/src/openvpn/block_dns.c b/src/openvpn/block_dns.c index d43cbcf..f88ba2c 100644 --- a/src/openvpn/block_dns.c +++ b/src/openvpn/block_dns.c @@ -370,7 +370,7 @@ get_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY family) } return ipiface.Metric; } - return -err; + return -(int)err; } /* diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c index 4123d0f..6c39aaa 100644 --- a/src/openvpnserv/automatic.c +++ b/src/openvpnserv/automatic.c @@ -155,7 +155,7 @@ match(const WIN32_FIND_DATA *find, LPCTSTR ext) * Modify the extension on a filename. */ static bool -modext(LPTSTR dest, int size, LPCTSTR src, LPCTSTR newext) +modext(LPTSTR dest, size_t size, LPCTSTR src, LPCTSTR newext) { int i; diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c index b8b817b..7901fd6 100644 --- a/src/openvpnserv/common.c +++ b/src/openvpnserv/common.c @@ -36,7 +36,7 @@ openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list arglist) len = _vsntprintf(str, size, format, arglist); str[size - 1] = 0; } - return (len >= 0 && len < size); + return (len >= 0 && (size_t)len < size); } int openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...) diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c index 8d94197..96e0de0 100644 --- a/src/openvpnserv/interactive.c +++ b/src/openvpnserv/interactive.c @@ -188,7 +188,7 @@ typedef enum { static DWORD AsyncPipeOp(async_op_t op, HANDLE pipe, LPVOID buffer, DWORD size, DWORD count, LPHANDLE events) { - int i; + DWORD i; BOOL success; HANDLE io_event; DWORD res, bytes = 0; @@ -1061,7 +1061,7 @@ RegisterDNS(LPVOID unused) { ipcfg, L"ipconfig /flushdns", timeout }, { ipcfg, L"ipconfig /registerdns", timeout }, }; - int ncmds = sizeof(cmds) / sizeof(cmds[0]); + DWORD ncmds = sizeof(cmds) / sizeof(cmds[0]); HANDLE wait_handles[2] = {rdns_semaphore, exit_event}; diff --git a/src/openvpnserv/validate.c b/src/openvpnserv/validate.c index 7a2d0e3..5422d33 100644 --- a/src/openvpnserv/validate.c +++ b/src/openvpnserv/validate.c @@ -298,7 +298,7 @@ IsUserInGroup(PSID sid, const PTOKEN_GROUPS token_groups, const WCHAR *group_nam break; } /* If a match is already found, ret == TRUE and the loop is skipped */ - for (int i = 0; i < nread && !ret; ++i) + for (DWORD i = 0; i < nread && !ret; ++i) { ret = EqualSid(members[i].lgrmi0_sid, sid); }
From: Simon Rozman <simon@rozman.si> --- src/openvpn/block_dns.c | 2 +- src/openvpnserv/automatic.c | 2 +- src/openvpnserv/common.c | 2 +- src/openvpnserv/interactive.c | 4 ++-- src/openvpnserv/validate.c | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-)