Hi,
On Mon, Nov 6, 2017 at 3:35 AM, Simon Rozman <si...@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
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
