Reentrant problem with inet_ntoa in the kernel

[MQ]

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


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.

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