| Message ID | 20180622102110.21888-1-a@unstable.cc |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] route: fix format string passed to argv_printf | expand |
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 <a@unstable.cc> wrote:<br>> %lu is not supported by our tiny argv_printf implementation and will<br>> trigger an ASSERT() when parsing it. Even though this particular<br>> ASSERT() is not critical as it happens during shutdown, we still have to<br>> fix it.<br>><br>> Since in this case the code is trying to use a DWORD variable as<br>> argument, and we know that DWORD is an unsigned long, which in turn is<br>> 32bit long on MS Windows (32 and 64bit version)[1][2], simply use<br>> the already supported %u instead of %lu.<br>><br>> [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>> [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>><br>> Signed-off-by: Antonio Quartulli <a@unstable.cc><br>> ---<br>> src/openvpn/route.c | 2 +-<br>> 1 file changed, 1 insertion(+), 1 deletion(-)<br>><br>> diff --git a/src/openvpn/route.c b/src/openvpn/route.c<br>> index a45a273a..209daeab 100644<br>> --- a/src/openvpn/route.c<br>> +++ b/src/openvpn/route.c<br>> @@ -1616,7 +1616,7 @@ add_route(struct route_ipv4 *r, const struct tuntap *tt, unsigned int flags,<br>> if (is_on_link(is_local_route, flags, rgi))<br>> {<br>> ai = rgi->adapter_index;<br>> - argv_printf_cat(&argv, "IF %lu", ai);<br>> + argv_printf_cat(&argv, "IF %u", ai);<br>> }<br>><br>> argv_msg(D_ROUTE, &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 "ai" 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 "%lu" 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'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
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,
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
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"><<a href="mailto:a@unstable.cc" target="_blank">a@unstable.cc</a>></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>>> --- a/src/openvpn/route.c<br> >> +++ b/src/openvpn/route.c<br> >> @@ -1616,7 +1616,7 @@ add_route(struct route_ipv4 *r, const struct tuntap<br> > *tt, unsigned int flags,<br> >> if (is_on_link(is_local_route, flags, rgi))<br> >> {<br> >> ai = rgi->adapter_index;<br> >> - argv_printf_cat(&argv, "IF %lu", ai);<br> >> + argv_printf_cat(&argv, "IF %u", ai);<br> >> }<br> >><br> >> argv_msg(D_ROUTE, &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 "ai" 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" rel="noreferrer" 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 "%lu" 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't handle %lu which is commonly used<br> > on Windows.<br> <br> </span>I was just about to test a v2 of this patch with the explicit cast to<br> "unsigned int". 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 "%lu" 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
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,
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);
%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(-)