Subject: Re: racoon broken in -current: grab_myaddrs and SIOCGIFCONF
To: enami tsugutomo <enami@netbsd.org>
From: Greg Troxel <gdt@ir.bbn.com>
List: tech-net
Date: 08/30/2007 11:48:53
racoon doesn't work in current because SIOCGIFCONF is broken. (I know
racoon should probably not use SIOCGIFCONF, but instead getifaddrs.  I
won't argue, but SIOCGIFCONF should work in any case.)

As far as I can tell there are two things wrong:

* commit to sys/net/if.h to include sockaddr_storage in struct ifreq.
  This breaks ABI compatability and things don't seem versioned.  I
  don't understand the rationale for this change.

* sys/net/if.c:ifconf() uses struct sockaddr for size tests when the
  dominant member of the union is sockaddr_storage.  This used to be
  correct (or rather a latent bug) before the change to sockaddr_storage
  in if.h.

Right now, sockaddrs that fit in struct sockaddr cause a struct ifreq to
be the full size (144 bytes).  But ones that don't (AF_LINK, AF_INET6)
are made to fit exactly, which makes them shorter.  I have a kludge in
racoon tomatch this, and it then works.

I propose the following:

1. revert addition of sockaddr_storage.  I would claim that using struct
sockaddr to place longer things is wrong; that's what sockaddr_storage
is for.  But perhaps other uses of struct ifreq don't have this issue.
This restores ABI compat for SIOCGIFCONF, and makes step 2 protective
rather than critical.

2. fix the code in if.c:ifconf() to use sizeof(struct ifconf.ifr_ifru)
instead of knowing the internal structure, so that each reply will be
bigger than a struct ifreq only if the address doesn't fit (the original
API).

If step 1 is also done, step 2 will guard against unintentional changes
restore ABI compatibility.  If 1 is not done, it will at least restore
API compatability.


Christos: can you explain which ioctls were trouble?  I don't follow
from your commit message below.

    Thanks,
    Greg


src/sys/net/if.h:
revision 1.124
date: 2007/05/29 21:32:29;  author: christos;  state: Exp;  lines: +2 -1
Add a sockaddr_storage member to "struct ifreq" maintaining backwards
compatibility with the older ioctls. This avoids stack smashing and
abuse of "struct sockaddr" when ioctls placed "struct sockaddr_foo's" that
were longer than "struct sockaddr".
XXX: Some of the emulations might be broken; I tried to add code for
them but I did not test them.

src/sys/net/if.c:
revision 1.190
date: 2007/06/01 09:35:47;  author: enami;  state: Exp;  lines: +26 -32
Fix some bugs in ifconf():
- maintain space left correctly.  the pointer is advanced by the size
  of struct ifreq when length of address is small.
- single sizeof operator is enough to take the size of struct.
- the type of `sz' must be singed type since it is/was compared against to
  the variable which may become negative.
- no need to traverse rest of interfaces once we got an error.  note that
  the latter `break' statement was inside inner loop.