Reentrant problem with inet_ntoa in the kernel

[Bruce Evans]

On Fri, 10 Nov 2006, MQ wrote:

> 2006/11/5, Bruce Evans <bde at zeta.org.au>:
>> 
>> On Sat, 4 Nov 2006, Brooks Davis wrote:
>> 
>> > On Sat, Nov 04, 2006 at 02:46:30AM +0000, MQ wrote:
>> >> 2006/11/3, Brooks Davis <brooks at one-eyed-alien.net>:
>> 
>> >>> The particular definition used is excedingly ugly.  At a minimum there
>> >>> needs to be a define or a constant "16" for the lenght rather than the
>> >>> 4*sizeof("123") nonsense.
>> 
>> The `4*sizeof "123"' is not nonsense.  It is better than the userland
>> version
>> at the time it was committed.  The userland version hard-coded the size as
>> 18 (sic).  The current userland version still hard-codes 18, but now
>> actually needs it to print an error message of length 17.  The uglyness in
>> `4*sizeof "123"' is just that it has 3 formatting style bugs (missing
>> ...

>> > I'd just use 16.  The inet_ntoa function is frankly inane.  It attempts
>> > to support chars that aren't 8 bits which would break so much stuff it
>> > isn't funny.
>> 
>> No, it assumes 8-bit chars.  It's masking with 0xff is apparently copied
>> from an old implementation that used plain chars.  The userland
>> implementation at the time it was committed does that, but uses a macro
>> to do the masking and is missing lots of style bugs.
>> 
>> The userland version now calls inet_ntop().  This is missing the design
>> bug of using a static buffer.  It calls inet_ntop4() for the ipv4 case.
>> This is closer to being non-ugly:
>> 
>> % static const char *
>> % inet_ntop4(const u_char *src, char *dst, socklen_t size)
>> % {
>> %       static const char fmt[] = "%u.%u.%u.%u";
>> %       char tmp[sizeof "255.255.255.255"];
>> %       int l;
>> %
>> %       l = snprintf(tmp, sizeof(tmp), fmt, src[0], src[1], src[2],
>> src[3]);
>> %       if (l <= 0 || (socklen_t) l >= size) {
>> %               errno = ENOSPC;
>> %               return (NULL);
>> %       }
>> %       strlcpy(dst, tmp, size);
>> %       return (dst);
>> % }
>> 
>> I would write this as:
>> 
>> %%%
>> CTASSERT(CHAR_BIT == 8);        /* else src would be misintepreted */
>> 
>> static const char *
>> inet_ntop4(const u_char *src, char *dst, socklen_t size)
>> {
>>         int n;
>> 
>>         n = snprintf(dst, size, "%u.%u.%u.%u", src[0], src[1], src[2],
>> src[3]);
>>         assert(n >= 0);         /* CHAR_BIT == 8 imples 0 < n <= 16 */
>> ...

> I don't know why the ret array in the userland version of the inet_ntoa
> should be 17. The length of the message itself is 17, and an \0 is needed
> for the str* to work.

Yes, it needs to be 18 for the error message.  I wrote "18 (sic)" because
18 is a surprising value.  Anyone who knows what an inet address is would
expect a length of 16.  But programmers shouldn't be counting bytes in
strings and mentally computing max(16, 18) and hard-coding that.  The
magic 16 won't change, but the 18 might.  Spelling 16/18 as a macro
and hard-coding 16/18 in the macro would be even worse, since the value is
only used onece.

> By the way, 4 * sizeof("123") chars should be always enough to contain an
> IPv4 address, no matter how many bits consititute a char.

It's enough for an ipv4 address, but inet_ntop4() is a library routine
so it shouldn't crash on invalid input.  With 8-bit chars, it happens
that there is no invalid input for u_char *src (except a src array with
less than 4 chars in it).  With >= 10-bit chars, the result could be
"1023.1023.1023.1023", which isn't an ipv4 address and is too large
for a 16-18 byte buffer.  inet_ntop4() needs to ensure that this and
some other errors don't occur.  It uses mainly snprintf(), but with
snprintf() another set of errors and out-of-bounds values can occur
in theory, and inet_ntop4()'s handling of these is not quite right.

Bruce