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

[Rob Landley]

On 09/30/2016 04:54 PM, David R. Hedges wrote:
> The interface name listed by /proc/net/ipv6_route is a char
> buf[IFNAMSIZ], which can be longer than 10.

Specifically, they can be 15 bytes long (16 plus a null terminator):

/usr/include/linux/if.h:#define	IFNAMSIZ	16

Which matches the FSF's ludicrous:

/usr/include/net/if.h:#define IF_NAMESIZE	16
/usr/include/net/if.h:# define IFNAMSIZ	IF_NAMESIZE

> Change our iface buffer to that same length, and fscanf a maximum string
> of that many characters (including \0).

There's a reason route's still in pending, I haven't done a proper
cleanup pass on it yet. (Largely because I need to set up network tests,
which requires a known vm test environment, and aboriginal linux has
been in pieces for about a year now due to toolchain issues. It's on the
todo list...)

> 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.)

> 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...

>  toys/pending/route.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)

Let's see...

> diff --git a/toys/pending/route.c b/toys/pending/route.c
> index 0126dc9..f0a1d54 100644
> --- a/toys/pending/route.c
> +++ b/toys/pending/route.c
> @@ -24,6 +24,8 @@ config ROUTE
>  #define FOR_route
>  #include "toys.h"
>  #include <net/route.h>
> +#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

> +#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

>  GLOBALS(
>    char *family;
> @@ -32,6 +34,8 @@ GLOBALS(
>  #define DEFAULT_PREFIXLEN 128
>  #define INVALID_ADDR 0xffffffffUL
>  #define IPV6_ADDR_LEN 40 //32 + 7 (':') + 1 ('\0')
> +#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.

>  struct _arglist {
>    char *arg;
> @@ -393,7 +397,7 @@ static void ipv6_addr_formating(char *ptr, char *addr)
>  
>  static void display_routes6(void)
>  {
> -  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? It's been hardwired to
16 in the kernel since June 17, 1993:

  Linux-0.99.10 (June 7, 1993)

  People finally gave up on net-1, Ross Biro grew tired of the
  flames, and net-2 appears with Fred van Kempen as maintainer.
  This is the big switch-over version.

Did I mention I have a kernel git repo that goes back to 0.0.1? Handy
thing to have, you can:

  wget http://landley.net/kdocs/local/linux-fullhist.tar.bz2
  tar xvjf linux-fullhist.tar.bz2
  cd linux-fullhist
  git checkout -f
  git pull

And it should be up to date and ready to use, and you can "git annotate"
all the way back. (In this tree, linux-0.99.10 is commit 54c93c05f69e.)

Anyway, I agree using the symbol is cleaner, but it shouldn't change the
behavior?

>    char ipv6_src_addr[41], flag_val[10], buf2[INET6_ADDRSTRLEN];
>    int prefixlen, metric, use, refcount, flag, items = 0;
>    unsigned char buf[sizeof(struct in6_addr)];
> @@ -403,8 +407,8 @@ static void display_routes6(void)
>    xprintf("Kernel IPv6 routing table\n"
>        "%-43s%-40s Flags Metric Ref    Use Iface\n", "Destination", "Next Hop");
>  
> -  while ((items = fscanf(fp, "%32s%x%*s%*x%32s%x%x%x%x%10s\n", ipv6_dest_addr+8,
> -          &prefixlen, ipv6_src_addr+8, &metric, &use, &refcount, &flag, 

There's your bug, that %10s should be %16s.

> +  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,

Except it needs to be 15 because it doesn't include the length of the
null terminator.

Personally I'd just hardware 15 in here (and use 15 in the variable
definition). I really doubt that if we built on freebsd or macos (do
solaris and aix still exist in a meaningful sense?) without a Linux
emulation layer (which everything but macos has anyway), we'd get the
right /proc file with the same data format anyway, so the name size
constant changing (which would break the ABI, something Linux is _proud_
it hasn't done since 0.0.1, modulo Greg KH and Kay Sievers being stupid
with /proc and /sys and Kay at least got chewed out about it by Linus)
would be the least of our problems.

>            iface)) == 8)
>    {
>      if (!(flag & RTF_UP)) continue; //skip down interfaces.
> -- 
> 2.7.4
> 

Thanks for the bug report. For now I'm just changing the value in the
fscanf to 15.

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...)

Rob