options.c: fix broken unary minus usage

From: Lev Stipakov <[email protected]...>

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 adding explicit cast to signed type.

Since GCC doesn't complain (and users too :), it probably casts to signed automatically.

Signed-off-by Lev Stipakov <[email protected]...>

---
 src/openvpn/options.c | 2 +-
 src/openvpn/ssl.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

src/openvpn/options.c +1 -1

index e42029c..1927a32 100644
@@ -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_add_to_extra_frame(&fake_frame, -(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,

src/openvpn/ssl.c +0 -0

index 315303b..18b1fbd 100644

Hi,

src/openvpn/options.c calc_options_string_link_mtu(const struct options

Quoted 22 lines · Lev Stipakov

From: Lev Stipakov <[email protected]...>

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 adding explicit cast to signed type.

Since GCC doesn't complain (and users too :), it probably casts to signed automatically.

Signed-off-by Lev Stipakov <[email protected]...>

---
 src/openvpn/options.c | 2 +-
 src/openvpn/ssl.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

src/openvpn/options.c +0 -0

index e42029c..1927a32 100644
@@ -3509,7 +3509,7 @@ calc_options_string_link_mtu(const struct options

*o, const struct frame *frame)

src/openvpn/options.c calc_options_string_link_mtu(const struct options · line 3510

Quoted 2 lines

src/openvpn/options.c +0 -0

         struct key_type fake_kt;
         init_key_type(&fake_kt, o->ciphername, o->authname, o->keysize,

true,

src/openvpn/options.c calc_options_string_link_mtu(const struct options *o, const struct frame *frame)

Quoted 3 lines

src/openvpn/options.c +1 -1

                       false);
-        frame_add_to_extra_frame(&fake_frame, -(crypto_max_overhead()));
+        frame_add_to_extra_frame(&fake_frame,

-(int)(crypto_max_overhead()));

src/openvpn/options.c calc_options_string_link_mtu(const struct options · line 3514

Quoted 2 lines

src/openvpn/options.c +0 -0

         crypto_adjust_frame_parameters(&fake_frame, &fake_kt, o->replay,
 

cipher_kt_mode_ofb_cfb(fake_kt.cipher));

src/openvpn/options.c calc_options_string_link_mtu(const struct options · line 3515

Quoted 1 line

src/openvpn/options.c +0 -0

         frame_finalize(&fake_frame, o->ce.link_mtu_defined,

o->ce.link_mtu,

src/openvpn/ssl.c tls_session_update_crypto_params(struct

Quoted 5 lines

src/openvpn/ssl.c +0 -0

index 315303b..18b1fbd 100644
@@ -1987,7 +1987,7 @@ tls_session_update_crypto_params(struct

tls_session *session,

src/openvpn/ssl.c tls_session_update_crypto_params(struct · line 1989

Quoted 3 lines

src/openvpn/ssl.c +0 -0

     }
 
     /* Update frame parameters: undo worst-case overhead, add actual

overhead */

src/openvpn/ssl.c tls_session_update_crypto_params(struct tls_session *session,

Quoted 4 lines

src/openvpn/ssl.c +1 -1

-    frame_add_to_extra_frame(frame, -(crypto_max_overhead()));
+    frame_add_to_extra_frame(frame, -(int)(crypto_max_overhead()));
     crypto_adjust_frame_parameters(frame, &session->opt->key_type,
                                    options->replay,

packet_id_long_form);

src/openvpn/ssl.c tls_session_update_crypto_params(struct · line 1993

Quoted 1 line

src/openvpn/ssl.c +0 -0

     frame_finalize(frame, options->ce.link_mtu_defined,

options->ce.link_mtu,

Quoted 1 line

Good catch. I had to look it up, but I think this is actually undefined behaviour. 6.5 point 5 of C99 says:

"If an exceptional condition occurs during the evaluation of an expression (that is, if the result is not mathematically defined or not in the range of representable values for its type), the behavior is undefined."

Remarkably, even with -Wall -Wextra -pedantic, GCC 7.3 does not throw any warnings. But some tests show that GCC does what one might expect when reading this code: cast to a signed value.

I'm just not sure whether we should add casts, or stop using the 'hack' of supplying a negative value to frame_add_to_extra_frame. Maybe we should add a frame_remove_from_extra_frame function instead. What do you think?

Hi

Quoted 12 lines · Steffan Karger

Hi,

Quoted 5 lines · Lev Stipakov

From: Lev Stipakov <[email protected]...>

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.

Good catch. I had to look it up, but I think this is actually undefined behaviour. 6.5 point 5 of C99 says:

Quoted 9 lines

"If an exceptional condition occurs during the evaluation of an expression (that is, if the result is not mathematically defined or not in the range of representable values for its type), the behavior is undefined."

Remarkably, even with -Wall -Wextra -pedantic, GCC 7.3 does not throw any warnings. But some tests show that GCC does what one might expect when reading this code: cast to a signed value.

In fact the issue here is not the unary minus, but the unsigned to signed conversion. So when there is no scope for overflow all is good. If there is overflow, unsigned->signed conversion is ill-defined -- cast doesn't fix it. In fact the cast is meaningless here (see more on that below):

Although C99 std does not seem to be explicit about unary minus of unsigned, the practice in C even before that was to subtract the number from the largest possible value and add 1: as in K&R A7.4.5 (p. 204 in my copy):

"If the operand is unsigned, the result is computed by subtracting the value from the largest value of the promoted type"

So unary minus of unsigned itself is well-defined. Actually even the unary minus of unsigned is computed using the above rule for signed:

"If the operand is signed, the result is computed by promoted operand to its unsigned type, applying -, and converting back to the signed type".!!

So what the cast achieves is a an additional iteration of conversions which gains nothing.

Quoted 5 lines

I'm just not sure whether we should add casts, or stop using the 'hack' of supplying a negative value to frame_add_to_extra_frame. Maybe we should add a frame_remove_from_extra_frame function instead. What do you think?

If the logic could be changed that should be preferred.

Selva

In fact the issue here is not the unary minus, but the unsigned to signed conversion. So when there is no scope for overflow all is good. If there is overflow, unsigned->signed conversion is ill-defined -- cast doesn't fix it. In fact the cast is meaningless here (see more on that below):

Although C99 std does not seem to be explicit about unary minus of unsigned, the practice in C even before that was to subtract the number from the largest possible value and add 1: as in K&R A7.4.5 (p. 204 in my copy):

"If the operand is unsigned, the result is computed by subtracting the value from the largest value of the promoted type"

So unary minus of unsigned itself is well-defined. Actually even the unary minus of unsigned is computed using the above rule for signed:

"If the operand is signed, the result is computed by promoted operand to its unsigned type, applying -, and converting back to the signed type".!!

So what the cast achieves is a an additional iteration of conversions which gains nothing.

of supplying a negative value to frame_add_to_extra_frame. Maybe we should add a frame_remove_from_extra_frame function instead. What do you think?

From: Lev Stipakov <[email protected]...>

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 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.

warning C4267: 'function': conversion from 'size_t' to 'const unsigned

Unnecessary casts hide bugs and hurt my eyes...

Quoted 3 lines

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

Quoted 1 line

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.

Quoted 1 line

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.

Quoted 7 lines · Lev Stipakov

Quoted 3 lines

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

Quoted 8 lines

Quoted 1 line

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 :-)

Quoted 2 lines

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 ;-)

Quoted 3 lines · Steffan Karger

Quoted 2 lines

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

Quoted 5 lines

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...