[Toybox] [PATCH] route: support iface names > 9 char

[David R. Hedges]

On 2016-10-03 14:34 (-0500), Rob Landley wrote:
> On 09/30/2016 04:54 PM, David R. Hedges wrote:
>> Also, try to more gracefully handle unexpectedly long lines from
>> /proc/net/ipv6_route in fscanf by matching (but not assigning) any
>> trailing characters before the newline, otherwise they're the starting
>> point for the next iteration.
> Ditto. (Isn't fscanf supposed to match arbitrary runs of whitespace any
> time you feed it whitespace automatically? Although I admit trusting
> /proc output to be sane because it requires root access to screw this
> input up.)
Whitespace, yes; other characters, no. This is unnecessary if we trust 
that /proc output is always going to adhere to our current definition, 
but it potentially makes failure a bit more graceful if it doesn't (e.g. 
the way it used to be written).

>> Here's an ipv6_route test case that demonstrated the problem:
>>
>> 260777000000001f000000013f8384c0 80 00000000000000000000000000000000 00
>> fe800000000000006512d4c76293bf99 00000000 00000001 00000005 01050003
>> rmnet_data0
>> 260777000000001f00000001d1be6a0a 80 00000000000000000000000000000000 00
>> fe800000000000006512d4c76293bf99 00000000 00000002 0000018d 01050003
>> rmnet_data0
>> 260777000000001f00000001d83ac0c2 80 00000000000000000000000000000000 00
>> fe800000000000006512d4c76293bf99 00000000 00000001 00000002 01050003
>> rmnet_data0
> This would come from /proc/net/ipv6_route? (I wonder why there are three
> ip6_* files in that directory but that one's (the only) ipv6_*?)
>
> That's a length 11 name, still 5 bytes left...
Correct; I swapped the xfopen path to point to a file containing my test 
case for validation. Indeed, I should have modified the interface name 
to actually test the maximum potential width (or potentially more, if I 
want to be mean); that was the real-life case that made me notice the bug

>> +#include <linux/if.h>   //for IFNAMSIZ
> Already defined by net/if.h which is #included by toys.h, and posix says
> that's the right header for it:
>
> http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/net_if.h.html
Sounds good; honestly, after finding that macro used in the source for 
the ipv6_route procfs entry, I just grepped through /usr/include on my 
machine.

>> +#include <sys/cdefs.h>  //for __STRING(x)
> A) Really shouldn't be needed here.
>
> B) This header is aggressively nonstandard and the fact musl
> intentionally doesn't ship it is a FAQ on their website:
>
> http://wiki.musl-libc.org/wiki/FAQ#Q:_I.27m_trying_to_compile_something_against_musl_and_I_get_error_messages_about_sys.2Fcdefs.h
I can't argue with that.

>> +#define STRINGIFY(x) __STRING(x)
>> +#define _S_IFNAMSIZ STRINGIFY( IFNAMSIZ)
> If you're going to do that why not just define STRINGIFY(x) #x (which is
> the c99 preprocessor feature that does this).
>
> And we still shouldn't need it.
__STRING(x) from <sys/cdefs.h> already is defined as #x; my 
understanding is that the C preprocessor needs the extra level of 
indirection to accomplish this (i.e. converting a #defined number into a 
const char buf). Without the extra step, the preprocessor inserts a 
literal "IFNAMSIZ" there instead of "16". Of course, this becomes 
irrelevant (/more complicated) once we address my bug of thinking the 
fscanf field width included the null terminator, so it actually needs to 
be 15.

>> -  char iface[16] = {0,}, ipv6_dest_addr[41];
>> +  char iface[IFNAMSIZ] = {0,}, ipv6_dest_addr[41];
> It's... the same? It's literally the same value?

> Anyway, I agree using the symbol is cleaner, but it shouldn't change the
> behavior?
Right. It is the same value (at least today ... and apparently since 
1993), but isn't a major point of having the macro in the first place so 
that different pieces of code can consistently refer to the same thing 
and reduce bugs (like, say, someone assuming that interface names should 
never be longer than 10 characters, or conversely allocating way more 
space than is really necessary :))?

>> +  while ((items = fscanf(fp, "%32s%x%*s%*x%32s%x%x%x%x%" _S_IFNAMSIZ "s%*[^\n]\n", ipv6_dest_addr+8,
>> +          &prefixlen, ipv6_src_addr+8, &metric, &use, &refcount, &flag,

> Personally I'd just hardwire 15 in here (and use 15 in the variable
> definition).
That's certainly the quick solution, and one that is admittedly likely 
to be fine for quite a while. At a minimum, though, I'd suggest a 
comment near the buffer with a reference to IFNAMSIZ, so it's clear why 
it is that way.

> My larger todo item here is figuring out what to do about ipv6. (Having
> to say -A ipv6 to enable ipv6 mode is kinda silly, in theory it should
> autodetect and produce both kinds of output? In practice that would
> probably break existing users and the field headers are different. Sigh...)
For what it's worth, the route utility in net-tools requires -A inet6.

In any event, thanks for incorporating a fix! Now I just have the long 
wait until this makes it to my phone (or more realistically, some future 
phone)...