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

Message ID 1539174377-2678-1-git-send-email-lstipakov@gmail.com
State Superseded
Headers show
Series
  • [Openvpn-devel,v2] options.c: fix broken unary minus usage
Related show

Commit Message

Lev Stipakov Oct. 10, 2018, 12:26 p.m.
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(-)

Comments

Arne Schwabe Oct. 10, 2018, 12:41 p.m. | #1
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>
Gert Doering Oct. 10, 2018, 6:50 p.m. | #2
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
Lev Stipakov Oct. 11, 2018, 10:40 a.m. | #3
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.
Gert Doering Oct. 11, 2018, 11:23 a.m. | #4
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
Steffan Karger Oct. 11, 2018, 11:52 a.m. | #5
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
Gert Doering Oct. 11, 2018, 11:56 a.m. | #6
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
Steffan Karger Oct. 11, 2018, 12:10 p.m. | #7
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

Patch

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 6d34acd..b1d6780 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -722,7 +722,7 @@  crypto_adjust_frame_parameters(struct frame *frame,
 
     crypto_overhead += kt->hmac_length;
 
-    frame_add_to_extra_frame(frame, crypto_overhead);
+    frame_add_to_extra_frame(frame, (unsigned int) crypto_overhead);
 
     msg(D_MTU_DEBUG, "%s: Adjusting frame parameters for crypto by %u bytes",
         __func__, (unsigned int) crypto_overhead);
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 52c64da..fe70175 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2675,7 +2675,7 @@  do_init_crypto_tls(struct context *c, const unsigned int flags)
     if (c->options.pull || c->options.mode == MODE_SERVER)
     {
         /* Account for worst-case crypto overhead before allocating buffers */
-        frame_add_to_extra_frame(&c->c2.frame, crypto_max_overhead());
+        frame_add_to_extra_frame(&c->c2.frame, (unsigned int) crypto_max_overhead());
     }
     else
     {
diff --git a/src/openvpn/mtu.h b/src/openvpn/mtu.h
index a82154a..690eb81 100644
--- a/src/openvpn/mtu.h
+++ b/src/openvpn/mtu.h
@@ -271,12 +271,18 @@  frame_add_to_link_mtu(struct frame *frame, const int increment)
 }
 
 static inline void
-frame_add_to_extra_frame(struct frame *frame, const int increment)
+frame_add_to_extra_frame(struct frame *frame, const unsigned int increment)
 {
     frame->extra_frame += increment;
 }
 
 static inline void
+frame_remove_from_extra_frame(struct frame *frame, const unsigned int increment)
+{
+    frame->extra_frame -= increment;
+}
+
+static inline void
 frame_add_to_extra_tun(struct frame *frame, const int increment)
 {
     frame->extra_tun += increment;
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index e42029c..be47090 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -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());
         crypto_adjust_frame_parameters(&fake_frame, &fake_kt, o->replay,
                                        cipher_kt_mode_ofb_cfb(fake_kt.cipher));
         frame_finalize(&fake_frame, o->ce.link_mtu_defined, o->ce.link_mtu,
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 315303b..747e176 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1987,7 +1987,7 @@  tls_session_update_crypto_params(struct tls_session *session,
     }
 
     /* Update frame parameters: undo worst-case overhead, add actual overhead */
-    frame_add_to_extra_frame(frame, -(crypto_max_overhead()));
+    frame_remove_from_extra_frame(frame, (unsigned int) crypto_max_overhead());
     crypto_adjust_frame_parameters(frame, &session->opt->key_type,
                                    options->replay, packet_id_long_form);
     frame_finalize(frame, options->ce.link_mtu_defined, options->ce.link_mtu,