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

Andrew J. Schorr aschorr at telemetry-investments.com
Fri Apr 1 18:24:38 BST 2005


On Fri, Apr 01, 2005 at 06:05:50PM +0100, Paul Jakma wrote:
> On Fri, 1 Apr 2005, Andrew J. Schorr wrote:
> 
> >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.
> 
> What else could it match?

In my opinion, those two interface names do not match.  Do you feel
that they should match?  The problem is that the comparison
code silently truncates the string being searched for.  This sort
of unexpected behavior seems like bad API design to me.

It also exposes us to possible bugs where the string being searched
for is actually a counted string of length IFNAMSIZ (not INTERFACE_NAMSIZ),
so we may run off the end by mistake.

> >if_lookup_by_name with a new function (adding a "c" suffix to 
> >indicate "counted", but I'm open to better suggestions):
> 
> how about '_len'? for length? or ye olde standard suffic of 'n'?

They're all fine with me.  Brevity is good.  I just thought
"if_lookup_by_namen" seemed like a strange name.  Or would you put
the "n" somewhere else in the name?  Perhaps if_lookup_by_name_len is most
transparent...

> > 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.
> 
> Seems a plan, but does it matter? We always ensure the array is nul 
> terminated, so other than the off-by-one assertion...

Yes, it does matter.  The issue is not the string inside the struct interface.
The potential bugs relate to the string being searched for, which could
be longer than INTERFACE_NAMSIZ (if passed in from a config file or
command line), or shorter (if coming from kernel).

> >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.
> 
> Ok.
> 
> But how will the caller know what length to search to, if not 
> INTERFACE_NAMSIZ?

If the caller doesn't know the length of the string, then there's no
hope. :-)  Who else could know?  If the interface name is coming from
the kernel, then the proper call might be:

   if_lookup_by_name_len(name, strnlen(name, IFNAMSIZ))

But if it is coming from the command line or a config file, the proper
call is (since there is no limit on the string length):

   if_lookup_by_name_len(name, strlen(name))

And if it is coming through the zserv internal protocol, the proper call is:

   if_lookup_by_name_len(name, strnlen(name, INTERFACE_NAMSIZ))

Does that make sense?  There are 2 goals here: 1. avoid false matches
(where the library silently truncates the string being searched for);
and 2. avoid running off the end of a buffer that may only be IFNAMSIZ
bytes in length.

Regards,
Andy



More information about the Quagga-dev mailing list