[Openvpn-devel,v2] options.c: fix broken unary minus usage

From: Lev Stipakov <lev@openvpn.net>

In Visual Studio when unary minus is applied to unsigned,
result is still unsigned. This means that when we use result
as function formal parameter, we pass incorrect value.

Fix by introducing frame_remove_from_extra_frame function,
which makes code semantically more clear and eliminates
the need in negative value and cast.

Since GCC didn't complain (and users too :), it probably performed
cast to signed automatically.

Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
v2: use frame_remove_from_extra_frame instead of passing negative value

 src/openvpn/crypto.c  | 2 +-
 src/openvpn/init.c    | 2 +-
 src/openvpn/mtu.h     | 8 +++++++-
 src/openvpn/options.c | 2 +-
 src/openvpn/ssl.c     | 2 +-
 5 files changed, 11 insertions(+), 5 deletions(-)

Am 10.10.18 um 14:26 schrieb Lev Stipakov:
> From: Lev Stipakov <lev@openvpn.net>
> 
> In Visual Studio when unary minus is applied to unsigned,
> result is still unsigned. This means that when we use result
> as function formal parameter, we pass incorrect value.
> 
> Fix by introducing frame_remove_from_extra_frame function,
> which makes code semantically more clear and eliminates
> the need in negative value and cast.


ACK. Looks much cleaner.

Acked-By: Arne Schwabe <arne@rfc2549.org>

Hi,

On Wed, Oct 10, 2018 at 03:26:17PM +0300, Lev Stipakov wrote:
>      crypto_overhead += kt->hmac_length;
>  
> -    frame_add_to_extra_frame(frame, crypto_overhead);
> +    frame_add_to_extra_frame(frame, (unsigned int) crypto_overhead);

Even if Arne already ACKed it, I have reservations about this.

Since crypto_overhead and crypto_max_overhead() are both size_t, and 
frame_add_to_extra_frame() is declared to take an "unsigned int" now, 
this cast should not be necessary.

Unnecessary casts hide bugs and hurt my eyes...

>  static inline void
> +frame_remove_from_extra_frame(struct frame *frame, const unsigned int increment)
> +{
> +    frame->extra_frame -= increment;
> +}

Maybe call the negative increment a "decrement" instead?

> @@ -3509,7 +3509,7 @@ calc_options_string_link_mtu(const struct options *o, const struct frame *frame)
>          struct key_type fake_kt;
>          init_key_type(&fake_kt, o->ciphername, o->authname, o->keysize, true,
>                        false);
> -        frame_add_to_extra_frame(&fake_frame, -(crypto_max_overhead()));
> +        frame_remove_from_extra_frame(&fake_frame, (unsigned int) crypto_max_overhead());

Same here wrt (unsigned int) cast.

Otherwise indeed much clearer.

gert

Hi,


> Since crypto_overhead and crypto_max_overhead() are both size_t, and
> frame_add_to_extra_frame() is declared to take an "unsigned int" now,
> this cast should not be necessary.


Visual Studio disagrees. Without explicit cast I got

> warning C4267: 'function': conversion from 'size_t' to 'const unsigned
int', possible loss of data

https://docs.microsoft.com/fi-fi/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4267?view=vs-2017


'var' : conversion from 'size_t' to 'type', possible loss of data
The compiler detected a conversion from size_t to a smaller type.


> Unnecessary casts hide bugs and hurt my eyes...
>

OTOH this is level 3 warning, which we have over 500. Since we'll probably
change level warning to 2 and
GCC doesn't complain, I'll remove it.

Maybe call the negative increment a "decrement" instead?
>

Yes.

Otherwise indeed much clearer.
>

V3 is on its way.

Hi,

On Thu, Oct 11, 2018 at 01:40:16PM +0300, Lev Stipakov wrote:
> > Since crypto_overhead and crypto_max_overhead() are both size_t, and
> > frame_add_to_extra_frame() is declared to take an "unsigned int" now,
> > this cast should not be necessary.
> 
> 
> Visual Studio disagrees. Without explicit cast I got
> 
> > warning C4267: 'function': conversion from 'size_t' to 'const unsigned
> int', possible loss of data
> 
> https://docs.microsoft.com/fi-fi/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4267?view=vs-2017
> 
> 
> 'var' : conversion from 'size_t' to 'type', possible loss of data
> The compiler detected a conversion from size_t to a smaller type.

So, maybe "crypto_overhead" and "crypto_max_overhead()" should be
changed to be just "int" here...?

For something which has a numeric value between "10-ish" and "100" (maybe),
using a 64bit integer initially and then adding casts because we want
to work with 32bit integers later on and not be warned about it sounds
like the wrong way to tackle this...

I know that Steffan likes using size_t for "things that have a size"
but I find it a bit questionable here :-)


> > Unnecessary casts hide bugs and hurt my eyes...
> 
> OTOH this is level 3 warning, which we have over 500. Since we'll probably
> change level warning to 2 and
> GCC doesn't complain, I'll remove it.

I'm all for fixing compiler warnings, but I hate silencing warnings with
casts that serve no purpose but "silence warning".  If we can fix it
for real, so the compiler has nothing to warn about, this is way better.

(It might not always be possible, like const/no-const mismatch on some
API functions, or struct sockaddr* madness on the socket API)

gert

Hi,

On 11-10-18 13:23, Gert Doering wrote:
> On Thu, Oct 11, 2018 at 01:40:16PM +0300, Lev Stipakov wrote:
>>> Since crypto_overhead and crypto_max_overhead() are both size_t, and
>>> frame_add_to_extra_frame() is declared to take an "unsigned int" now,
>>> this cast should not be necessary.
>>
>>
>> Visual Studio disagrees. Without explicit cast I got
>>
>>> warning C4267: 'function': conversion from 'size_t' to 'const unsigned
>> int', possible loss of data
>>
>> https://docs.microsoft.com/fi-fi/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4267?view=vs-2017
>>
>>
>> 'var' : conversion from 'size_t' to 'type', possible loss of data
>> The compiler detected a conversion from size_t to a smaller type.
> 
> So, maybe "crypto_overhead" and "crypto_max_overhead()" should be
> changed to be just "int" here...?
> 
> For something which has a numeric value between "10-ish" and "100" (maybe),
> using a 64bit integer initially and then adding casts because we want
> to work with 32bit integers later on and not be warned about it sounds
> like the wrong way to tackle this...
> 
> I know that Steffan likes using size_t for "things that have a size"
> but I find it a bit questionable here :-)

So the underlying problem is that "further down" used int to store
sizes, but since that just is the way it is and we shouldn't pull in
refactoring the code base for each small change, I would say either make
both 'int', or make both 'size_t', but without changing the types of
struct frame itself.  I of course prefer the latter ;-)

-Steffan

Hi,

On Thu, Oct 11, 2018 at 01:52:37PM +0200, Steffan Karger wrote:
> > I know that Steffan likes using size_t for "things that have a size"
> > but I find it a bit questionable here :-)
> 
> So the underlying problem is that "further down" used int to store
> sizes, but since that just is the way it is and we shouldn't pull in
> refactoring the code base for each small change, I would say either make
> both 'int', or make both 'size_t', but without changing the types of
> struct frame itself.  I of course prefer the latter ;-)

Adding or substracting a size_t from frame->extra_frame (which is an int)
will quite likely cause the same warning again...  so I'd argue for
"make it all int, and be happy with it foreverafter" :-)

(And possibly revisit our use of size_t... *cough*)

gert

On 11-10-18 13:56, Gert Doering wrote:
> On Thu, Oct 11, 2018 at 01:52:37PM +0200, Steffan Karger wrote:
>>> I know that Steffan likes using size_t for "things that have a size"
>>> but I find it a bit questionable here :-)
>>
>> So the underlying problem is that "further down" used int to store
>> sizes, but since that just is the way it is and we shouldn't pull in
>> refactoring the code base for each small change, I would say either make
>> both 'int', or make both 'size_t', but without changing the types of
>> struct frame itself.  I of course prefer the latter ;-)
> 
> Adding or substracting a size_t from frame->extra_frame (which is an int)
> will quite likely cause the same warning again...

I don't think so, because at that level, you'll be adding/subtracting
either "int + int" or "int + size_t", which are equally likely to cause
overflows. Only that signed overflows are not only not what you want,
but also undefined behaviour.

But well, as I said, I'm fine with both in this case. It will
effectively not change any logic anyway.

-Steffan