[Openvpn-devel] route: fix format string passed to argv_printf

Message ID 20180622102110.21888-1-a@unstable.cc
State Superseded
Headers show
Series
  • [Openvpn-devel] route: fix format string passed to argv_printf
Related show

Commit Message

Antonio Quartulli June 22, 2018, 10:21 a.m.
%lu is not supported by our tiny argv_printf implementation and will
trigger an ASSERT() when parsing it. Even though this particular
ASSERT() is not critical as it happens during shutdown, we still have to
fix it.

Since in this case the code is trying to use a DWORD variable as
argument, and we know that DWORD is an unsigned long, which in turn is
32bit long on MS Windows (32 and 64bit version)[1][2], simply use
the already supported %u instead of %lu.

[1]https://msdn.microsoft.com/en-us/library/aa383751(VS.85).aspx
[2]https://en.wikipedia.org/wiki/Integer_(computer_science)#Long_integer

Signed-off-by: Antonio Quartulli <a@unstable.cc>
---
 src/openvpn/route.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Selva Nair June 22, 2018, 2:45 p.m. | #1
Hi,

On Fri, Jun 22, 2018 at 6:21 AM, Antonio Quartulli <a@unstable.cc> wrote:
> %lu is not supported by our tiny argv_printf implementation and will
> trigger an ASSERT() when parsing it. Even though this particular
> ASSERT() is not critical as it happens during shutdown, we still have to
> fix it.
>
> Since in this case the code is trying to use a DWORD variable as
> argument, and we know that DWORD is an unsigned long, which in turn is
> 32bit long on MS Windows (32 and 64bit version)[1][2], simply use
> the already supported %u instead of %lu.
>
> [1]https://msdn.microsoft.com/en-us/library/aa383751(VS.85).aspx
> [2]https://en.wikipedia.org/wiki/Integer_(computer_science)#Long_integer
>
> Signed-off-by: Antonio Quartulli <a@unstable.cc>
> ---
>  src/openvpn/route.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/openvpn/route.c b/src/openvpn/route.c
> index a45a273a..209daeab 100644
> --- a/src/openvpn/route.c
> +++ b/src/openvpn/route.c
> @@ -1616,7 +1616,7 @@ add_route(struct route_ipv4 *r, const struct tuntap
*tt, unsigned int flags,
>          if (is_on_link(is_local_route, flags, rgi))
>          {
>              ai = rgi->adapter_index;
> -            argv_printf_cat(&argv, "IF %lu", ai);
> +            argv_printf_cat(&argv, "IF %u", ai);
>          }
>
>          argv_msg(D_ROUTE, &argv);

Thanks for cleaning up my mistake (commit
06ad53e067d9a8be571a27f44005fa7e8038f69e introduced this). One
purpose of that commit was to get rid of -Wformat warnings as those
are not always benign. But this will re-introduce a warning as "ai" is
unsigned long even though its the same size as unsigned.

Currently we do support building with -Werror=format (see
https://patchwork.openvpn.net/patch/234/#463), so I suggest either
cast to (unsigned)  which we know is safe here, or add "%lu" to
argv_printf_arglist().

I would prefer the latter as its easy to overlook the fact that
argv_printf_cat() can't handle %lu which is commonly used
on Windows.

Selva
<div dir="ltr">Hi,<br><br>On Fri, Jun 22, 2018 at 6:21 AM, Antonio Quartulli &lt;a@unstable.cc&gt; wrote:<br>&gt; %lu is not supported by our tiny argv_printf implementation and will<br>&gt; trigger an ASSERT() when parsing it. Even though this particular<br>&gt; ASSERT() is not critical as it happens during shutdown, we still have to<br>&gt; fix it.<br>&gt;<br>&gt; Since in this case the code is trying to use a DWORD variable as<br>&gt; argument, and we know that DWORD is an unsigned long, which in turn is<br>&gt; 32bit long on MS Windows (32 and 64bit version)[1][2], simply use<br>&gt; the already supported %u instead of %lu.<br>&gt;<br>&gt; [1]<a href="https://msdn.microsoft.com/en-us/library/aa383751(VS.85).aspx" target="_blank">https://msdn.microsoft.com/<wbr>en-us/library/aa383751(VS.85).<wbr>aspx</a><br>&gt; [2]<a href="https://en.wikipedia.org/wiki/Integer_(computer_science)#Long_integer" target="_blank">https://en.wikipedia.org/wi<wbr>ki/Integer_(computer_science)#<wbr>Long_integer</a><br>&gt;<br>&gt; Signed-off-by: Antonio Quartulli &lt;a@unstable.cc&gt;<br>&gt; ---<br>&gt;  src/openvpn/route.c | 2 +-<br>&gt;  1 file changed, 1 insertion(+), 1 deletion(-)<br>&gt;<br>&gt; diff --git a/src/openvpn/route.c b/src/openvpn/route.c<br>&gt; index a45a273a..209daeab 100644<br>&gt; --- a/src/openvpn/route.c<br>&gt; +++ b/src/openvpn/route.c<br>&gt; @@ -1616,7 +1616,7 @@ add_route(struct route_ipv4 *r, const struct tuntap *tt, unsigned int flags,<br>&gt;          if (is_on_link(is_local_route, flags, rgi))<br>&gt;          {<br>&gt;              ai = rgi-&gt;adapter_index;<br>&gt; -            argv_printf_cat(&amp;argv, &quot;IF %lu&quot;, ai);<br>&gt; +            argv_printf_cat(&amp;argv, &quot;IF %u&quot;, ai);<br>&gt;          }<br>&gt;<br>&gt;          argv_msg(D_ROUTE, &amp;argv);<br><br>Thanks for cleaning up my mistake (commit<br>06ad53e067d9a8be571a27f44005fa<wbr>7e8038f69e introduced this). One<br>purpose of that commit was to get rid of -Wformat warnings as those<br>are not always benign. But this will re-introduce a warning as &quot;ai&quot; is<br>unsigned long even though its the same size as unsigned.<br><br>Currently we do support building with -Werror=format (see<br><a href="https://patchwork.openvpn.net/patch/234/#463" target="_blank">https://patchwork.openvpn.net/<wbr>patch/234/#463</a>), so I suggest either<br>cast to (unsigned)  which we know is safe here, or add &quot;%lu&quot; to<br>argv_printf_arglist().<br><br>I would prefer the latter as its easy to overlook the fact that<br>argv_printf_cat() can&#39;t handle %lu which is commonly used<br>on Windows.<br><br>Selva<br></div>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Antonio Quartulli June 22, 2018, 2:48 p.m. | #2
Hi,

On 22/06/18 22:45, Selva Nair wrote:
[cut]
>> --- a/src/openvpn/route.c
>> +++ b/src/openvpn/route.c
>> @@ -1616,7 +1616,7 @@ add_route(struct route_ipv4 *r, const struct tuntap
> *tt, unsigned int flags,
>>          if (is_on_link(is_local_route, flags, rgi))
>>          {
>>              ai = rgi->adapter_index;
>> -            argv_printf_cat(&argv, "IF %lu", ai);
>> +            argv_printf_cat(&argv, "IF %u", ai);
>>          }
>>
>>          argv_msg(D_ROUTE, &argv);
> 
> Thanks for cleaning up my mistake (commit
> 06ad53e067d9a8be571a27f44005fa7e8038f69e introduced this). One
> purpose of that commit was to get rid of -Wformat warnings as those
> are not always benign. But this will re-introduce a warning as "ai" is
> unsigned long even though its the same size as unsigned.
> 
> Currently we do support building with -Werror=format (see
> https://patchwork.openvpn.net/patch/234/#463), so I suggest either
> cast to (unsigned)  which we know is safe here, or add "%lu" to
> argv_printf_arglist().
> 
> I would prefer the latter as its easy to overlook the fact that
> argv_printf_cat() can't handle %lu which is commonly used
> on Windows.

I was just about to test a v2 of this patch with the explicit cast to
"unsigned int". I was leaning towards the explicit cast because we
already print DWORD variables in several spots and we always cast them
to int or unsigned int.

However, if we believe supporting %lu can be more useful, I can do that.

Last opinion? :)

Cheers,
Gert Doering June 22, 2018, 3:34 p.m. | #3
Hi,

On Fri, Jun 22, 2018 at 10:48:30PM +0800, Antonio Quartulli wrote:
> However, if we believe supporting %lu can be more useful, I can do that.

I tend to add "%lu" to argv.c (which might eventually come back to bite
me, as there are still patches from Heiko pending wrt argv IIRC).

Since we make the same mistake again and again, we shouldn't cast around
it but just make it work :-) - and it's not a lot of extra code, nicely
isolated.

gert
Selva Nair June 22, 2018, 3:37 p.m. | #4
Hi,

On Fri, Jun 22, 2018 at 10:48 AM, Antonio Quartulli <a@unstable.cc> wrote:

> Hi,
>
> On 22/06/18 22:45, Selva Nair wrote:
> [cut]
> >> --- a/src/openvpn/route.c
> >> +++ b/src/openvpn/route.c
> >> @@ -1616,7 +1616,7 @@ add_route(struct route_ipv4 *r, const struct
> tuntap
> > *tt, unsigned int flags,
> >>          if (is_on_link(is_local_route, flags, rgi))
> >>          {
> >>              ai = rgi->adapter_index;
> >> -            argv_printf_cat(&argv, "IF %lu", ai);
> >> +            argv_printf_cat(&argv, "IF %u", ai);
> >>          }
> >>
> >>          argv_msg(D_ROUTE, &argv);
> >
> > Thanks for cleaning up my mistake (commit
> > 06ad53e067d9a8be571a27f44005fa7e8038f69e introduced this). One
> > purpose of that commit was to get rid of -Wformat warnings as those
> > are not always benign. But this will re-introduce a warning as "ai" is
> > unsigned long even though its the same size as unsigned.
> >
> > Currently we do support building with -Werror=format (see
> > https://patchwork.openvpn.net/patch/234/#463), so I suggest either
> > cast to (unsigned)  which we know is safe here, or add "%lu" to
> > argv_printf_arglist().
> >
> > I would prefer the latter as its easy to overlook the fact that
> > argv_printf_cat() can't handle %lu which is commonly used
> > on Windows.
>
> I was just about to test a v2 of this patch with the explicit cast to
> "unsigned int". I was leaning towards the explicit cast because we
> already print DWORD variables in several spots and we always cast them
> to int or unsigned int.
>

We used to have some printed with even %d and no cast. The last time we
cleaned up the offending ones and removed a few casts nearby.

However, if we believe supporting %lu can be more useful, I can do that.
>
> Last opinion? :)
>

I prefer "%lu" and no cast and have been replacing %u by %lu elsewhere in
the code given a chance.

There are a lots DWORD to (unsigned) in tun.c and elsewhere but there is no
compelling reason to change them. We should get rid of casts to (int) when
possible, though.

Selva
<div dir="ltr"><br><div class="gmail_extra">Hi,<br><br></div><div class="gmail_extra"><div class="gmail_quote">On Fri, Jun 22, 2018 at 10:48 AM, Antonio Quartulli <span dir="ltr">&lt;<a href="mailto:a@unstable.cc" target="_blank">a@unstable.cc</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
On 22/06/18 22:45, Selva Nair wrote:<br>
[cut]<br>
<span>&gt;&gt; --- a/src/openvpn/route.c<br>
&gt;&gt; +++ b/src/openvpn/route.c<br>
&gt;&gt; @@ -1616,7 +1616,7 @@ add_route(struct route_ipv4 *r, const struct tuntap<br>
&gt; *tt, unsigned int flags,<br>
&gt;&gt;          if (is_on_link(is_local_route, flags, rgi))<br>
&gt;&gt;          {<br>
&gt;&gt;              ai = rgi-&gt;adapter_index;<br>
&gt;&gt; -            argv_printf_cat(&amp;argv, &quot;IF %lu&quot;, ai);<br>
&gt;&gt; +            argv_printf_cat(&amp;argv, &quot;IF %u&quot;, ai);<br>
&gt;&gt;          }<br>
&gt;&gt;<br>
&gt;&gt;          argv_msg(D_ROUTE, &amp;argv);<br>
&gt; <br>
&gt; Thanks for cleaning up my mistake (commit<br>
&gt; 06ad53e067d9a8be571a27f44005fa<wbr>7e8038f69e introduced this). One<br>
&gt; purpose of that commit was to get rid of -Wformat warnings as those<br>
&gt; are not always benign. But this will re-introduce a warning as &quot;ai&quot; is<br>
&gt; unsigned long even though its the same size as unsigned.<br>
&gt; <br>
&gt; Currently we do support building with -Werror=format (see<br>
&gt; <a href="https://patchwork.openvpn.net/patch/234/#463" rel="noreferrer" target="_blank">https://patchwork.openvpn.net/<wbr>patch/234/#463</a>), so I suggest either<br>
&gt; cast to (unsigned)  which we know is safe here, or add &quot;%lu&quot; to<br>
&gt; argv_printf_arglist().<br>
&gt; <br>
&gt; I would prefer the latter as its easy to overlook the fact that<br>
&gt; argv_printf_cat() can&#39;t handle %lu which is commonly used<br>
&gt; on Windows.<br>
<br>
</span>I was just about to test a v2 of this patch with the explicit cast to<br>
&quot;unsigned int&quot;. I was leaning towards the explicit cast because we<br>
already print DWORD variables in several spots and we always cast them<br>
to int or unsigned int.<br></blockquote><br>We used to have some printed with even %d and no cast. The last time we cleaned up the offending ones and removed a few casts nearby.<br></div><div class="gmail_quote"><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
However, if we believe supporting %lu can be more useful, I can do that.<br>
<br>
Last opinion? :)<br></blockquote><div><br></div><div>I prefer &quot;%lu&quot; and no cast and have been replacing %u by %lu elsewhere in the code given a chance.<br><br>There are a lots DWORD to (unsigned) in tun.c and elsewhere but there is no compelling reason to change them. We should get rid of casts to (int) when possible, though.<br><br></div><div>Selva<br></div></div></div></div>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Antonio Quartulli June 22, 2018, 3:39 p.m. | #5
Hi

On 22/06/18 23:37, Selva Nair wrote:
>> Last opinion? :)
>>
> 
> I prefer "%lu" and no cast and have been replacing %u by %lu elsewhere in
> the code given a chance.

You and Gert both replied at the same time with the same preference of
supporting %lu. And I think all the arguments are valid - that is just
the right thing in this case so we can avoid shortcut/forced casts.

Will send v2 with this change included.

Thanks all!

Cheers,

Patch

diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index a45a273a..209daeab 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -1616,7 +1616,7 @@  add_route(struct route_ipv4 *r, const struct tuntap *tt, unsigned int flags,
         if (is_on_link(is_local_route, flags, rgi))
         {
             ai = rgi->adapter_index;
-            argv_printf_cat(&argv, "IF %lu", ai);
+            argv_printf_cat(&argv, "IF %u", ai);
         }
 
         argv_msg(D_ROUTE, &argv);