route: fix format string passed to argv_printf

%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

---
 src/openvpn/route.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index a45a273a..209daeab 100644
@@ -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);

2.17.1

Hi,

Hi,

On 22/06/18 22:45, Selva Nair wrote:
[cut]

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,

Antonio Quartulli

Hi,

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

"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             [email protected]...

Hi,

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.

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

Hi

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,

Antonio Quartulli

%lu is not supported by our tiny argv_printf implementation, therefore it will trigger an ASSERT() when parsing it at route.c:1638.

Add support for '%lu' in argv_print() and prevent the ASSERT from being triggered.

---

It seems I managed to trigger this ASSERT when connecting to my v6-only server with "gateway-redirect !ipv4 ipv6" set. Not sure why this issue hasn't surfaced earlier though.

Changes from v1:
- keep format string as "%lu" but add support for it in argv_printf()
- tested on Windows 10 on top of the ipv6-only branch

 src/openvpn/argv.c | 7 +++++++
 1 file changed, 7 insertions(+)

index dec7e3bf..9100a196 100644
@@ -251,6 +251,13 @@ argv_printf_arglist(struct argv *a, const char *format, va_list arglist)
                 openvpn_snprintf(numstr, sizeof(numstr), "%u", va_arg(arglist, unsigned int));
                 argv_append(a, string_alloc(numstr, NULL));
             }
+            else if (!strcmp(term, "%lu"))
+            {
+                char numstr[64];
+                openvpn_snprintf(numstr, sizeof(numstr), "%lu",
+                                 va_arg(arglist, unsigned long));
+                argv_append(a, string_alloc(numstr, NULL));
+            }
             else if (!strcmp(term, "%s/%d"))
             {
                 char numstr[64];

2.17.1

The code is trivially correct, and it does what it says on the lid :-)
- and to verify, I've added a unit test to test_argv.c - attached below, we might consider adding something like this, or something more elaborate for all supported argv*() arguments...

Your patch has been applied to the master and release/2.4 branch ("someone will fix something else some day, relying on the existance of %lu, and then 2.4 will surprisingly explode").

commit e38d3a004195f33c5c04fe7c04db5d66c53241bc (master) commit 61dcf1513d69ddf1261402744496436923ec14bb (release/2.4)

Author: Antonio Quartulli
Date:   Sat Jun 23 10:11:47 2018 +0800

     add support for %lu in argv_printf and prevent ASSERT

     Signed-off-by: Antonio Quartulli <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg17115.html
     Signed-off-by: Gert Doering <[email protected]...>

----------

index 4a3ba559..0e7a4513 100644
@@ -116,8 +116,12 @@ argv_str__multiple_argv__correct_output(void **state)
     argv_printf(&a, "%s%sc", PATH1, PATH2);
     argv_printf_cat(&a, "%s", PARAM1);
     argv_printf_cat(&a, "%s", PARAM2);
+    argv_printf_cat(&a, "%d", -1);
+    argv_printf_cat(&a, "%u", -1);
+    argv_printf_cat(&a, "%lu", (unsigned long) (1L<<33) );
     output = argv_str(&a, &gc, PA_BRACKET);
-    assert_string_equal(output, "[" PATH1 PATH2 "] [" PARAM1 "] [" PARAM2 "]");
+    assert_string_equal(output, "[" PATH1 PATH2 "] [" PARAM1 "] [" PARAM2 "]"
+				" [-1] [4294967295] [8589934592]");
 
     argv_reset(&a);
     gc_free(&gc);

--
kind regards,

Gert Doering

Some basic integer tests to verify signed, unsigned and long unsigned (2^33) printing.

---
 tests/unit_tests/openvpn/test_argv.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

index 4a3ba559..0e7a4513 100644
@@ -116,8 +116,12 @@ argv_str__multiple_argv__correct_output(void **state)
     argv_printf(&a, "%s%sc", PATH1, PATH2);
     argv_printf_cat(&a, "%s", PARAM1);
     argv_printf_cat(&a, "%s", PARAM2);
+    argv_printf_cat(&a, "%d", -1);
+    argv_printf_cat(&a, "%u", -1);
+    argv_printf_cat(&a, "%lu", (unsigned long) (1L<<33) );
     output = argv_str(&a, &gc, PA_BRACKET);
-    assert_string_equal(output, "[" PATH1 PATH2 "] [" PARAM1 "] [" PARAM2 "]");
+    assert_string_equal(output, "[" PATH1 PATH2 "] [" PARAM1 "] [" PARAM2 "]"
+				" [-1] [4294967295] [8589934592]");
 
     argv_reset(&a);
     gc_free(&gc);

2.16.4

Hi,

Ditch that patch... this only works on 64bit systems.

What I wanted to see is "yes, it really does long math", but on systems where sizeof(int) = sizeof(long) = 4, this will just not do the right thing.

Do we care?

Or shall I just have it print "1", so we know "%lu works and prints its argument nicely"?

gert

"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             [email protected]...

Hi,

Most 64 bit systems (excluding Windows)

That should be enough, but we could get fancy and use -1L and compare the result against sprintf(str, "%lu", -1L)

Selva

Hi,

I just recommended something similar on IRC.
I think it would make sense to "validate" argv_printf* against sprintf. Not just for %lu but also for the other formats. This way we know our code is doing what sprintf would also do.

Cheers,

Antonio Quartulli

Some basic integer tests to verify signed, unsigned and long unsigned (2^33) printing.

--
v2: test %lu with "1" as windows and linux/i386 do not have 64 bit long

---
 tests/unit_tests/openvpn/test_argv.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

index 4a3ba559..202a8ff5 100644
@@ -116,8 +116,12 @@ argv_str__multiple_argv__correct_output(void **state)
     argv_printf(&a, "%s%sc", PATH1, PATH2);
     argv_printf_cat(&a, "%s", PARAM1);
     argv_printf_cat(&a, "%s", PARAM2);
+    argv_printf_cat(&a, "%d", -1);
+    argv_printf_cat(&a, "%u", -1);
+    argv_printf_cat(&a, "%lu", 1 );
     output = argv_str(&a, &gc, PA_BRACKET);
-    assert_string_equal(output, "[" PATH1 PATH2 "] [" PARAM1 "] [" PARAM2 "]");
+    assert_string_equal(output, "[" PATH1 PATH2 "] [" PARAM1 "] [" PARAM2 "]"
+				" [-1] [4294967295] [1]");
 
     argv_reset(&a);
     gc_free(&gc);

2.16.4

Hi,

I'll go for the "just 1" thing so we have something.

If one of you feels bored, I'm happy to test something more fancy :-) - but I do not feel particularily like "coming up with fancy tests for argv.c" today.

gert

"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             [email protected]...

Hi,

This is good enough, but (there is always a but :)

That 2^33 still lurking in the commit message could go..

ACK otherwise.

Good luck with the game.

Selva

Some basic integer tests to verify signed, unsigned and long unsigned (1L) printing.

--
v2: test %lu with "1" as windows and linux/i386 do not have 64 bit long
v3: fix 2^33 in commit message, use "1L" for proper data type (Selva)

---
 tests/unit_tests/openvpn/test_argv.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

index 4a3ba559..0fdd3f0a 100644
@@ -116,8 +116,12 @@ argv_str__multiple_argv__correct_output(void **state)
     argv_printf(&a, "%s%sc", PATH1, PATH2);
     argv_printf_cat(&a, "%s", PARAM1);
     argv_printf_cat(&a, "%s", PARAM2);
+    argv_printf_cat(&a, "%d", -1);
+    argv_printf_cat(&a, "%u", -1);
+    argv_printf_cat(&a, "%lu", 1L );
     output = argv_str(&a, &gc, PA_BRACKET);
-    assert_string_equal(output, "[" PATH1 PATH2 "] [" PARAM1 "] [" PARAM2 "]");
+    assert_string_equal(output, "[" PATH1 PATH2 "] [" PARAM1 "] [" PARAM2 "]"
+				" [-1] [4294967295] [1]");
 
     argv_reset(&a);
     gc_free(&gc);

2.16.4

Hi,

Looks good now. An unconditional ACK this time.

Selva

Patch has been applied to the master and release/2.4 branch.

commit 4376805d8fd2a5d4a3a7c5e4f60948a3ef76ff3b (master) commit 6b27704dafcb60e995601178bbcfac751207e93e (release/2.4)

Author: Gert Doering
Date:   Sat Jun 23 21:15:38 2018 +0200

     Add %d, %u and %lu tests to test_argv unit tests.

     Signed-off-by: Gert Doering <[email protected]...>
     Acked-by: Selva Nair <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/search?l=mid&[email protected]...
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering