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

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
Related show

Commit Message

Lev Stipakov Oct. 8, 2018, 4:09 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 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(-)

Comments

Steffan Karger Oct. 9, 2018, 8:38 p.m. | #1
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
Selva Nair Oct. 9, 2018, 9:14 p.m. | #2
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 &lt;<a href="mailto:steffan@karger.me">steffan@karger.me</a>&gt; 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>
&gt; From: Lev Stipakov &lt;<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>&gt;<br>
&gt; <br>
&gt; In Visual Studio when unary minus is applied to unsigned,<br>
&gt; result is still unsigned. This means that when we use result<br>
&gt; as function formal parameter, we pass incorrect value.<br>
&gt; <br>
&gt; Fix by adding explicit cast to signed type.<br>
&gt; <br>
&gt; Since GCC doesn&#39;t complain (and users too :), it probably<br>
&gt; casts to signed automatically.<br>
&gt; <br>
&gt; Signed-off-by: Lev Stipakov &lt;<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>&gt;<br>
&gt; ---<br>
&gt;  src/openvpn/options.c | 2 +-<br>
&gt;  src/openvpn/ssl.c     | 2 +-<br>
&gt;  2 files changed, 2 insertions(+), 2 deletions(-)<br>
&gt; <br>
&gt; diff --git a/src/openvpn/options.c b/src/openvpn/options.c<br>
&gt; index e42029c..1927a32 100644<br>
&gt; --- a/src/openvpn/options.c<br>
&gt; +++ b/src/openvpn/options.c<br>
&gt; @@ -3509,7 +3509,7 @@ calc_options_string_link_mtu(const struct options *o, const struct frame *frame)<br>
&gt;          struct key_type fake_kt;<br>
&gt;          init_key_type(&amp;fake_kt, o-&gt;ciphername, o-&gt;authname, o-&gt;keysize, true,<br>
&gt;                        false);<br>
&gt; -        frame_add_to_extra_frame(&amp;fake_frame, -(crypto_max_overhead()));<br>
&gt; +        frame_add_to_extra_frame(&amp;fake_frame, -(int)(crypto_max_overhead()));<br>
&gt;          crypto_adjust_frame_parameters(&amp;fake_frame, &amp;fake_kt, o-&gt;replay,<br>
&gt;                                         cipher_kt_mode_ofb_cfb(fake_kt.cipher));<br>
&gt;          frame_finalize(&amp;fake_frame, o-&gt;ce.link_mtu_defined, o-&gt;ce.link_mtu,<br>
&gt; diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c<br>
&gt; index 315303b..18b1fbd 100644<br>
&gt; --- a/src/openvpn/ssl.c<br>
&gt; +++ b/src/openvpn/ssl.c<br>
&gt; @@ -1987,7 +1987,7 @@ tls_session_update_crypto_params(struct tls_session *session,<br>
&gt;      }<br>
&gt;  <br>
&gt;      /* Update frame parameters: undo worst-case overhead, add actual overhead */<br>
&gt; -    frame_add_to_extra_frame(frame, -(crypto_max_overhead()));<br>
&gt; +    frame_add_to_extra_frame(frame, -(int)(crypto_max_overhead()));<br>
&gt;      crypto_adjust_frame_parameters(frame, &amp;session-&gt;opt-&gt;key_type,<br>
&gt;                                     options-&gt;replay, packet_id_long_form);<br>
&gt;      frame_finalize(frame, options-&gt;ce.link_mtu_defined, options-&gt;ce.link_mtu,<br>
&gt; <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>
&quot;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.&quot;<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-&gt;signed conversion is ill-defined -- cast doesn&#39;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&amp;R A7.4.5 (p. 204 in my copy):<br><br></div><div>&quot;If the operand is unsigned, the result is computed by subtracting the value from the largest value of the promoted type&quot;<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>&quot;If the operand is signed, the result is computed by promoted operand to its unsigned type, applying -, and converting back to the signed type&quot;.!!<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&#39;m just not sure whether we should add casts, or stop using the &#39;hack&#39;<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>
Selva Nair Oct. 9, 2018, 9:19 p.m. | #3
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 &lt;<a href="mailto:selva.nair@gmail.com">selva.nair@gmail.com</a>&gt; 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 &lt;<a href="mailto:steffan@karger.me" target="_blank">steffan@karger.me</a>&gt; 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>
&gt; From: Lev Stipakov &lt;<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>&gt;<br>
&gt; <br>
&gt; In Visual Studio when unary minus is applied to unsigned,<br>
&gt; result is still unsigned. This means that when we use result<br>
&gt; 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>
&quot;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.&quot;<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-&gt;signed conversion is ill-defined -- cast doesn&#39;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&amp;R A7.4.5 (p. 204 in my copy):<br><br></div><div>&quot;If the operand is unsigned, the result is computed by subtracting the value from the largest value of the promoted type&quot;<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>&quot;If the operand is signed, the result is computed by promoted operand to its unsigned type, applying -, and converting back to the signed type&quot;.!!<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&#39;m just not sure whether we should add casts, or stop using the &#39;hack&#39;<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>
Selva Nair Oct. 9, 2018, 9:29 p.m. | #4
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 &lt;<a href="mailto:selva.nair@gmail.com">selva.nair@gmail.com</a>&gt; 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-&gt;signed conversion is ill-defined -- cast doesn&#39;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&amp;R A7.4.5 (p. 204 in my copy):<br><br></div><div>&quot;If the operand is unsigned, the result is computed by subtracting the value from the largest value of the promoted type&quot;<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>&quot;If the operand is signed, the result is computed by promoted operand to its unsigned type, applying -, and converting back to the signed type&quot;.!!<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&#39;m having really bad day and writing mostly nonsense.<br><br></div><div>Selva <br></div></div></div></div>
Lev Stipakov Oct. 10, 2018, 7:13 a.m. | #5
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.

Patch

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,