[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openbsd-tech
Subject:    Re: route(4) manual and sockaddrs; ROUNDUP()
From:       Vadim Penzin <vadim () penzin ! net>
Date:       2019-04-27 18:19:41
Message-ID: b3add9b9-998c-2ce6-f083-70307ec1823f () penzin ! net
[Download RAW message or body]

Well, the manual shall tell the truth, whatever it is:

     Messages are formed by a header followed by a small number of
     sockaddr structures of variable length. The size of every
     sockaddr structure can be computed by rounding the value of the
     `sa_len' field of the current structure up to sizeof(long)
     bytes.

I provided a reference to the code in question in my original message:

> It took me some time to figure out that, apparently, I am looking at
> the combined effect of in_socktrim() (see src/sys/netinet/in.c) and
> ROUNDUP() (see src/sys/net/rtsock.c) applied to the netmask of
> 0xff000000 that was stored as sockaddr_in'.

The following is a more precise description of its location:

See 
https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/net/rtsock.c?annotate=1.285

The macro ROUNDUP is defined on lines 1347-1348
It is also used in the definition of the macro ADVANCE on line 1349
The first direct use of ROUNDUP that I quoted is on line 1431.
The second is on line 1476.

--Vadim

On 4/26/19 6:14 PM, Claudio Jeker wrote:
> On Fri, Apr 26, 2019 at 01:55:52PM +0300, Vadim Penzin wrote:
>> Greetings,
>>
>>
>> 1. The manual of route(4) explains the structure of its messages thus:
>>
>>      `Messages are formed by a header followed by a small number of
>>      sockaddr structures (which are variable length), interpreted by
>>      position, and delimited by the length entry in the sockaddr.'
>>
>> (That phrase is quite unfortunate in itself: `sa_len' is supposed to
>> /determine/ the length of a structure.  To me, the word `delimited'
>> implies the presence of a memory object between two adjacent
>> structures, which certainly was not the intent of the author.)
> 
> Yes this is not quite right since the lenght is rounded to the next long
> word to ensure that structures are properly aligned on strict align
> architectures.
>   
>> Armed with that knowledge, I wrote a parser the other day.  I very
>> much enjoyed OpenBSD until my program fatally stumbled on a peculiar
>> structure that does not fit the above description:
>>
>>      05 00 00 00 ff 00 00 00
>>
>> (The value of `sa_len' of that `sockaddr' is nothing that I am
>> familiar with, the address family is `AF_UNSPEC', and the most
>> disturbing: `sa_len' does not match the physical size of this
>> structure. These eight bytes were immediately followed by a perfectly
>> valid `sockaddr_in'.)
> 
> This is a probably a netmask which are special in many senses from the old
> time of the radix routing tree. For AF_UNSPEC it defines how many bytes
> are used to store the netmask. It is wierd I agree.
>   
>> It took me some time to figure out that, apparently, I am looking at
>> the combined effect of in_socktrim() (see src/sys/netinet/in.c) and
>> ROUNDUP() (see src/sys/net/rtsock.c) applied to the netmask of
>> 0xff000000 that was stored as sockaddr_in'.
>>
>> I believe that manuals should be more merciful.  In the absence of
>> proper documentation, the part of my program that handles this case looks
>> like a mysterious ritual; it should not be that way.
>   
> Please send suggestions to better documentation to this list.
> route(4) is not perfect for sure and could use some work.
>   
>> 2. The definition of ROUNDUP() somewhat surprised me: it silently handles
>> zero. The macro is local to the compilation unit; it is used
>> this way:
>>
>>          if (rtinfo == NULL || (sa = rtinfo->rti_info[i]) == NULL)
>> 	        continue;
>> 	rtinfo->rti_addrs |= (1 << i);
>> 	dlen = ROUNDUP(sa->sa_len);
>>
>> and this way:
>>
>> 	if ((sa = rtinfo->rti_info[i]) == NULL)
>>                  continue;
>> 	rtinfo->rti_addrs |= (1 << i);
>> 	dlen = ROUNDUP(sa->sa_len);
> 
> What code are you talking about?
>   
>> (In the second case, the programmer does not check `rtinfo' for being
>> NULL.  I only hope that there exists a good explanation to that.)
>>
>> Since `sa_len' describes the size of a `sockaddr' (or one of its
>> derivatives) /including/ `sa_len' (maybe I am wrong, but this is my
>> interpretation of the comment `total length' that appears near the
>> definition of `struct sockaddr' in <sys/socket.h>), `sa_len' just
>> cannot be zero.
> 
> Yes, it can not be zero.
>   
>> The current definition of ROUNDUP() might hide a bug.  In addition, on
>> some machines, it disturbs the pipeline of the CPU by introducing a
>> branch (for no real reason, as it seems, while I might be
>> nitpicking). At very least, it looks confusing.
> 
> 

[prev in list] [next in list] [prev in thread] [next in thread]