Reentrant problem with inet_ntoa in the kernel

[Bruce Evans]

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 spaces
around binary operator, space after sizeof, and missing parentheses for
sizeof) and depends on the storage for a '.' being the same as the storage
for the the '\0' terminator.  I would write it as sizeof("255.255.255.255").

>> Oh, I see. This kind of definition is really annoying, and hard to keep all
>> the
>> occurrences consistent. Maybe a better way is use a macro to make that
>> clear?
>>
>> #define IPV4_ADDRSZ (4 * sizeof "123")
>> char buf[IPV4_ADDRSZ];

This is less clear, since it takes twice as much code to obfuscate the
size in a macro for no benefits since the macro is only used once.

>> This "ugly" definition comes from inet_ntoa() in /sys/libkern/inet_ntoa.c,
>> I just copied the style without too much consideration, it's my fault.
>
> 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 */
 	if ((u_int)n >= size) {
 		errno = ENOSPC;
 		return (NULL);
 	}
 	return (dst);
}
%%%

This is closer to the version in RELENG_6 than the current version.  It
doesn't use tmp[]] to preserve dst on error, and fixes the bounds checking
without introducing several style bugs and not actually fixing the bounds
checking.  The old version was:

 	if ((socklen_t)snprintf(dst, size, fmt, src[0], src[1], src[2], src[3]
 	    >= size) {
 		...
 	}

This is good enough since 0 < l <= 16 implies that the preposterou case
(l <= 0) and the preposterous broken case ((socklen_t)l != l) can't
happen, but it is easier to use correct bounds checking than to understant
why bugs in the bounds checking are harmless.

Bruce