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

Paul Jakma paul at clubi.ie
Fri Apr 1 18:05:50 BST 2005


On Fri, 1 Apr 2005, Andrew J. Schorr wrote:

> 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

> Yes, that is 100% correct.

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

Up to INTERFACE_NAMSIZ is the only portion which is relevant. Even if 
we removed this limitation within zebra, we'll just see the same 
problem kernel side. However, your point below makes this moot.

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

Seems sensible, yes.

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

Ah, indeed. That seems to be the case yes.

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

Yeah..

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

how about '_len'? for length? or ye olde standard suffic of 'n'?

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

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

regards,
-- 
Paul Jakma	paul at clubi.ie	paul at jakma.org	Key ID: 64A2FF6A
Fortune:
grep me no patterns and I'll tell you no lines.



More information about the Quagga-dev mailing list