[Toybox] More ifconfig cleanup!

[Rob Landley]

http://landley.net/hg/toybox/rev/1128

So I'm trying to make struct sockaddr_with_len go away. It's a union of  
three data types:

   typedef struct sockaddr_with_len {
     union {
       struct sockaddr sock;
       struct sockaddr_in sock_in;
       struct sockaddr_in6 sock_in6;
     } sock_u;
   } sockaddr_with_len;

(You'll note the actual "len" part was removed in an earlier cleanup.)

This structure is returned by get_sockaddr(), and it's obviously wrong  
because the first thing get_sockaddr does is check for "local:" and if  
so it typecasts sock_u.sock to struct sockaddr_un and starts writing to  
it. That's a 110 byte struct, and the three structs in the union are  
16, 16, and 28 bytes respectively. So we're writing off the edge of our  
allocation.

get_sockaddr() is only called from two places: One is in set_address(),  
and the other is in the "add/del" ipv6 address manipulation.  
set_address() itself is called from two places, which collectively  
handle "pointopoint", "broadcast", "netmask", "dstaddr", and ifconfig  
without a command (to set the interface address directly). I _think_  
all of those only handle ipv4 addresses. (So what does specifying  
local: mean, anyway? What ifconfig command line would trigger that  
codepath, and what would it mean?)

Let's back up and figure out what this get_sockaddr function does:

1) This local: stuff. The ifconfig man page does not contain the word  
"local", so I have NO CLUE what this is. If this is used, it can  
corrupt memory as described above, because the union doesn't include  
sockaddr_un but we typecast it to that and strncpy up to UNIX_PATH_MATH  
(108 bytes) into a 28 byte structure.

2) Parsing some sort of [ipv6]:port syntax. Neither of the callers use  
the port field of the returned struct, and the port part is not  
currently hooked up because both callers pass in 0 for port, and then  
we set port but it's never used. There's a port_num but it's only ever  
set from the value passed in during variable initialization (I.E.  
always 0). So it looks like I once again broke stuff I couldn't test  
during earlier cleanups. And I still can't test it because the only  
occurrence of the word "port" in the ifconfig man page has to do with  
"media type" which looks deeply obsolete. (10base2?) I'll happily  
accept a patch to put this back, but for right now I can't find an  
example of anything using this syntax.

3) A dns lookup using the function that handles both ipv4 and ipv6.  
Except that if your "hints" are AF_INET it should only return AF_INET  
addresses, and AF_INET6 should only return AF_INET6, correct? If you  
want both, the hint is AF_UNSPEC, which we don't pass. So there's only  
one codepath in play at a time here.

Now I'm curious. In Ubuntu:

   $ ifconfig eth0 inet6 1:2:3:4:5:6:7:8
   Don't know how to set addresses for family 10.

It looks like only "add" and "del" handle ipv6 addresses,

   $ ifconfig eth0 add 1.2.3.4 && ifconfig eth0
   eth0      Link encap:Ethernet  HWaddr dc:0e:a1:52:92:77
             UP BROADCAST MULTICAST  MTU:1500  Metric:1
             RX packets:0 errors:0 dropped:0 overruns:0 frame:0
             TX packets:0 errors:0 dropped:0 overruns:0 carrier:0
             collisions:0 txqueuelen:1000
             RX bytes:0 (0.0 B)  TX bytes:0 (0.0 B)
             Interrupt:42

and those ONLY handle ipv6 addresses, and instead of an error will  
silently ignore an ipv4 address. (*boggle*).

Ok, let's rename get_sockaddr() to get_addrinfo() and remove all this  
port stuff, and the square bracket syntax I can't find an example of.  
There are two callers: the add/del clause in main(), and set_address().

The reason there are two callers of set_address() is I never quite  
merged all the ifconfig_main() argument parsing into the big table.  
You'll notice the block at the end from "hw" through "default" is  
mis-indented, that's once again me using stale whitespace as a  
note-to-self that this section hasn't been finished yet.

So let's do two things:

1) Copy the misindented if/else staircase before the for(;;) loop  
instead of after it.

2) Merge the isdigit/default case into the table. It's similar to the  
"up" entry except for off being SIOCSIFADDR, special casing the name  
(so it's its' own argument), and this funky behavior with ":" not  
setting the flag.

So in try[] we add {0, IFF_UP|IFF_RUNNING, SIOCSIFADDR}, then in the  
loop:

   if (!t->name && (isdigit(**argv) || !strcmp(*argv, "default")))  
argv--;
   else if (strcmp(t->name, s)) continue;

(The argv-- is so it's treated as its own address argument.) And then  
add the alias check to the set_address call:

   } else if (t->name || !strchr(ifre.ifr_name, ':')) {
     set_address(*argv, &ifre, off);
   }

Now we can zap the old if (isdigit/default) stanza and then block copy  
in the contents of the set_address() function, since this is now the  
only caller. While simplifying, we can eliminate the memset() because  
get_addrinfo() will error_exit() if it can't memset the address with a  
found value.

So that's one of the callers of get_addrinfo() cleared up (and its  
function removed). The other caller is the add/del case of the old  
if/else staircase we just relocated to before the for(;;) loop. And now  
that we've merged "default" into the loop, there are only two clauses  
left in that staircase. The other is "hw".

The reason "hw" is still separate is it takes another argument:

   ifconfig hw ether 11:22:33:44:55:66
   ifconfig hw infiniband \
     11:22:33:44:55:66:77:88:99:00:11:22:33:44:55:66:77:88:99:00

And of course infiniband is problematic because the ioctl only has room  
for 14 bytes of the 20 byte payload, ala

    
http://lists.landley.net/pipermail/toybox-landley.net/2013-June/001043.html
   http://projects.puppetlabs.com/issues/1415

So you _have_ to much about in /sys to get or set infiniband addresses.  
(Of course the non-table version isn't doing that properly right now  
either, and the display code isn't querying it, but that's a remaining  
todo item for the cleanup.)

So let's see... merging hw into the table probably isn't worth it,  
there's just too much oddball behavior. There are also bugs in the  
current version, because "ifconfig hw ether" with no address will  
probably dereference a null pointer and segfault. Ok, let's add in the  
"write to sysfs" behavior, and while I'm at it throw in a check for "/"  
in the interface name so nobody gets clever ideas about  
"../../../blah\0" as an interface name. (Don't have to check the  
length, we did an xstrncpy that would die if the argument was too long,  
because exiting noisily is better than silently truncating invalid  
input.)

The "add/del" ipv6 stuff also has oddball behavior (opening the ipv6  
socket). There's no reason to have the struct definition way far away  
fromits only user. And "ifrinte6_addr" is clearly a typo, yet its only  
user repeated the same typo.

Hmmm, ipv6 mask length should default to 128, not 0.
 1385793438.0