Hi On Tue, Oct 9, 2018 at 4:39 PM Steffan Karger <stef...@karger.me> wrote:
> Hi, > > On 08-10-18 18:09, Lev Stipakov wrote: > > From: Lev Stipakov <l...@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 <l...@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
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
