| Message ID | 1539014950-21921-1-git-send-email-lstipakov@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] options.c: fix broken unary minus usage | expand |
Hi, On 08-10-18 18:09, Lev Stipakov wrote: > 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 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 <lev@openvpn.net> > --- > src/openvpn/options.c | 2 +- > src/openvpn/ssl.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/openvpn/options.c b/src/openvpn/options.c > index e42029c..1927a32 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_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, > diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c > index 315303b..18b1fbd 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_add_to_extra_frame(frame, -(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, > 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? -Steffan
Hi On Tue, Oct 9, 2018 at 4:39 PM Steffan Karger <steffan@karger.me> wrote: > Hi, > > On 08-10-18 18:09, Lev Stipakov wrote: > > 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 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 <lev@openvpn.net> > > --- > > src/openvpn/options.c | 2 +- > > src/openvpn/ssl.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/src/openvpn/options.c b/src/openvpn/options.c > > index e42029c..1927a32 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_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, > > diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c > > index 315303b..18b1fbd 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_add_to_extra_frame(frame, -(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, > > > > 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. > 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. > 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 <div dir="ltr">Hi<br><div><br><div class="gmail_quote"><div dir="ltr">On Tue, Oct 9, 2018 at 4:39 PM Steffan Karger <<a href="mailto:steffan@karger.me">steffan@karger.me</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br> <br> On 08-10-18 18:09, Lev Stipakov wrote:<br> > From: Lev Stipakov <<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>><br> > <br> > In Visual Studio when unary minus is applied to unsigned,<br> > result is still unsigned. This means that when we use result<br> > as function formal parameter, we pass incorrect value.<br> > <br> > Fix by adding explicit cast to signed type.<br> > <br> > Since GCC doesn't complain (and users too :), it probably<br> > casts to signed automatically.<br> > <br> > Signed-off-by: Lev Stipakov <<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>><br> > ---<br> > src/openvpn/options.c | 2 +-<br> > src/openvpn/ssl.c | 2 +-<br> > 2 files changed, 2 insertions(+), 2 deletions(-)<br> > <br> > diff --git a/src/openvpn/options.c b/src/openvpn/options.c<br> > index e42029c..1927a32 100644<br> > --- a/src/openvpn/options.c<br> > +++ b/src/openvpn/options.c<br> > @@ -3509,7 +3509,7 @@ calc_options_string_link_mtu(const struct options *o, const struct frame *frame)<br> > struct key_type fake_kt;<br> > init_key_type(&fake_kt, o->ciphername, o->authname, o->keysize, true,<br> > false);<br> > - frame_add_to_extra_frame(&fake_frame, -(crypto_max_overhead()));<br> > + frame_add_to_extra_frame(&fake_frame, -(int)(crypto_max_overhead()));<br> > crypto_adjust_frame_parameters(&fake_frame, &fake_kt, o->replay,<br> > cipher_kt_mode_ofb_cfb(fake_kt.cipher));<br> > frame_finalize(&fake_frame, o->ce.link_mtu_defined, o->ce.link_mtu,<br> > diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c<br> > index 315303b..18b1fbd 100644<br> > --- a/src/openvpn/ssl.c<br> > +++ b/src/openvpn/ssl.c<br> > @@ -1987,7 +1987,7 @@ tls_session_update_crypto_params(struct tls_session *session,<br> > }<br> > <br> > /* Update frame parameters: undo worst-case overhead, add actual overhead */<br> > - frame_add_to_extra_frame(frame, -(crypto_max_overhead()));<br> > + frame_add_to_extra_frame(frame, -(int)(crypto_max_overhead()));<br> > crypto_adjust_frame_parameters(frame, &session->opt->key_type,<br> > options->replay, packet_id_long_form);<br> > frame_finalize(frame, options->ce.link_mtu_defined, options->ce.link_mtu,<br> > <br> <br> Good catch. I had to look it up, but I think this is actually undefined<br> behaviour. 6.5 point 5 of C99 says:<br> <br> "If an exceptional condition occurs during the evaluation of an<br> expression (that is, if the result is not mathematically defined or not<br> in the range of representable values for its type), the behavior is<br> undefined."<br> <br> Remarkably, even with -Wall -Wextra -pedantic, GCC 7.3 does not throw<br> any warnings. But some tests show that GCC does what one might expect<br> when reading this code: cast to a signed value.<br></blockquote><div><br></div><div>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):<br><br>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):<br><br></div><div>"If the operand is unsigned, the result is computed by subtracting the value from the largest value of the promoted type"<br><br></div><div>So unary minus of unsigned itself is well-defined. Actually even the unary minus of unsigned is computed using the above rule for signed:<br><br>"If the operand is signed, the result is computed by promoted operand to its unsigned type, applying -, and converting back to the signed type".!!<br><br></div><div>So what the cast achieves is a an additional iteration of conversions which gains nothing.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br> I'm just not sure whether we should add casts, or stop using the 'hack'<br> of supplying a negative value to frame_add_to_extra_frame. Maybe we<br> should add a frame_remove_from_extra_frame function instead. What do you<br> think?<br></blockquote><div><br>If the logic could be changed that should be preferred.<br><br></div><div>Selva<br></div><br></div></div></div>
Hi, More noise: a typo alert below: On Tue, Oct 9, 2018 at 5:14 PM Selva Nair <selva.nair@gmail.com> wrote: > Hi > > On Tue, Oct 9, 2018 at 4:39 PM Steffan Karger <steffan@karger.me> wrote: > >> Hi, >> >> On 08-10-18 18:09, Lev Stipakov wrote: >> > 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. >> >> 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. >> > > 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: > Typo alert: signed and unsigned inverted in the last sentence, in case anyone really read all that.. > "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. > > >> 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 > <div dir="ltr"><div><div>Hi,<br></div><br></div>More noise: a typo alert below:<br><br><div><div><div><div class="gmail_quote"><div dir="ltr">On Tue, Oct 9, 2018 at 5:14 PM Selva Nair <<a href="mailto:selva.nair@gmail.com">selva.nair@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi<br><div><br><div class="gmail_quote"><div dir="ltr">On Tue, Oct 9, 2018 at 4:39 PM Steffan Karger <<a href="mailto:steffan@karger.me" target="_blank">steffan@karger.me</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br> <br> On 08-10-18 18:09, Lev Stipakov wrote:<br> > From: Lev Stipakov <<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>><br> > <br> > In Visual Studio when unary minus is applied to unsigned,<br> > result is still unsigned. This means that when we use result<br> > as function formal parameter, we pass incorrect value.<br><br> Good catch. I had to look it up, but I think this is actually undefined<br> behaviour. 6.5 point 5 of C99 says:<br> <br> "If an exceptional condition occurs during the evaluation of an<br> expression (that is, if the result is not mathematically defined or not<br> in the range of representable values for its type), the behavior is<br> undefined."<br> <br> Remarkably, even with -Wall -Wextra -pedantic, GCC 7.3 does not throw<br> any warnings. But some tests show that GCC does what one might expect<br> when reading this code: cast to a signed value.<br></blockquote><div><br></div><div>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):<br><br>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):<br><br></div><div>"If the operand is unsigned, the result is computed by subtracting the value from the largest value of the promoted type"<br><br></div><div>So unary minus of unsigned itself is well-defined. Actually even the unary minus of unsigned is computed using the above rule for signed:<br></div></div></div></div></blockquote><div><br>Typo alert: signed and unsigned inverted in the last sentence, in case anyone really read all that..<br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div class="gmail_quote"><div><br>"If the operand is signed, the result is computed by promoted operand to its unsigned type, applying -, and converting back to the signed type".!!<br><br></div><div>So what the cast achieves is a an additional iteration of conversions which gains nothing.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br> I'm just not sure whether we should add casts, or stop using the 'hack'<br> of supplying a negative value to frame_add_to_extra_frame. Maybe we<br> should add a frame_remove_from_extra_frame function instead. What do you<br> think?<br></blockquote><div><br>If the logic could be changed that should be preferred.<br><br></div><div>Selva<br></div></div></div></div> </blockquote></div></div></div></div></div>
Hi On Tue, Oct 9, 2018 at 5:14 PM Selva Nair <selva.nair@gmail.com> wrote: > > > 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. > Ignore most of it -- I'm having really bad day and writing mostly nonsense. Selva <div dir="ltr">Hi<br><div><br><div class="gmail_quote"><div dir="ltr">On Tue, Oct 9, 2018 at 5:14 PM Selva Nair <<a href="mailto:selva.nair@gmail.com">selva.nair@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div><div class="gmail_quote"><div><br></div><div>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):<br><br>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):<br><br></div><div>"If the operand is unsigned, the result is computed by subtracting the value from the largest value of the promoted type"<br><br></div><div>So unary minus of unsigned itself is well-defined. Actually even the unary minus of unsigned is computed using the above rule for signed:<br><br>"If the operand is signed, the result is computed by promoted operand to its unsigned type, applying -, and converting back to the signed type".!!<br> </div></div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div class="gmail_quote"><div><br></div><div>So what the cast achieves is a an additional iteration of conversions which gains nothing.<br></div></div></div></div></blockquote><div><br></div><div>Ignore most of it -- I'm having really bad day and writing mostly nonsense.<br><br></div><div>Selva <br></div></div></div></div>
Hi, This was catched by Visual Studio. Moreover, with SDL enabled ( https://docs.microsoft.com/fi-fi/cpp/build/reference/sdl-enable-additional-security-checks?view=vs-2017 ) this warning is treated as an error. It is probably a good idea to trigger Visual Studio build on every commit to master (and enable SDL), have to think what would be the best way to do it. Currently we build with VS when making PRs to openvpn-build, which we don't do that often. 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? > Agreed, semantically it is more clear when _add_ method adds and _remove_ method removes. I'll add _remove_ method and change type of "increment" parameter to unsigned in both methods so that next time when we'll try to use _add_ with negative value, we'll get a warning.
diff --git a/src/openvpn/options.c b/src/openvpn/options.c index e42029c..1927a32 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_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, diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 315303b..18b1fbd 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_add_to_extra_frame(frame, -(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,