[quagga-dev 3071] Re: bugs related to length of interface names

Andrew J. Schorr aschorr at telemetry-investments.com
Fri Apr 1 14:54:52 BST 2005


On Fri, Apr 01, 2005 at 01:38:38PM +0100, Paul Jakma wrote:
> - the string should be no more than INTERFACE_NAMSIZ
>   - its the largest known length of interface name on any platform we
>     support
> - the storage for the string should be INTERFACE_NAMSIZ+1
> - hence we should truncate strings at INTERFACE_NAMSIZ
> - storage of string should set string[INTERFACE_NAMSIZ] = '\0' for
>   safety, however:
>   - kernel interfaces do not need nul terminator
>   - zserv protocol is not null terminated
> 
> Seem right?

Yes, that is 100% correct.

> >Interface reallylonginterfacen is down
> > Description: test
> > index -1 inactive interface
> >
> >The name is being silently truncated.
> 
> Seems roughly right, we cant support arbitrary length interface 
> names, even if we did kernel's usually dont.

But I don't think the current behavior is quite right, because
if_lookup_by_name may give false matches.  In other words, if the
code calls if_lookup_by_name("reallylonginterfacename0"), it will
get a match with the "reallylonginterfacen" interface.  This seems
dangerous to me, I think there should be no match in that case.

I also think that we should give an error message and not allow
the user to create an interface whose name length exceeds the maximum.
That seems safer to me than silently truncating the name.

> The only possible 
> confusion is here that kernel may have a smaller interface name size 
> and that it will truncate the name to an even smaller one.

Yes, that's the sort of confusion I'm worried about.  Generally, I think
that IFNAMSIZ (kernel maximum name length) is less than INTERFACE_NAMSIZ
(quagga's maximum name length).  So this is inviting bugs where kernel
names may be passed to these lookup functions.  If you look in
the current zebra/kernel_socket.c:ifm_read() function, you will
see that the code has to copy the interface into another buffer so
that it can add a '\0' termination char before calling if_lookup_by_name
because of this issue.  This seems like poor interface design to me.

> Defining interface name in terms of string,length and getting rid of 
> INTERFACE_NAMSIZE might be even better, yes.

That is exactly what I am proposing.  I would replace if_lookup_by_name
with a new function (adding a "c" suffix to indicate "counted", but I'm
open to better suggestions):

  struct interface *if_lookup_by_namec(const char *name, size_t namelen)

and I would add a macro that gives the current behavior:

  #define if_lookup_by_name(N) \
  	  if_lookup_by_namec((N), strnlen((N), INTERFACE_NAMSIZ))

Similarly for if_get_by_name.  Then I think we should review existing
calls to see if the INTERFACE_NAMSIZ assumption is actually correct.

> But if_create sets a terminator. Where is ifname created without a 
> terminator?

You are correct, the name inside struct interface is always terminated
with a NUL char.  The problem is that the lookup functions do not know
the length of the name they are searching for.  They assume that the
maximum length is INTERFACE_NAMSIZ, but this is not true in some cases.

Regards,
Andy



More information about the Quagga-dev mailing list