Date: Sat, 11 Nov 2006 01:49:29 +0000 From: MQ <antinvidia@gmail.com> To: "Bruce Evans" <bde@zeta.org.au> Cc: freebsd-net@freebsd.org Subject: Re: Reentrant problem with inet_ntoa in the kernel Message-ID: <be0088ce0611101749v452476e3i982ae6d1bda72cd9@mail.gmail.com> In-Reply-To: <20061110210358.B64521@delplex.bde.org> References: <be0088ce0611020026y4fe07749pd5a984f8744769b@mail.gmail.com> <20061102142543.GC70915@lor.one-eyed-alien.net> <be0088ce0611030146u5e97e08cmbd36e94d772c8a94@mail.gmail.com> <20061103141732.GA87515@lor.one-eyed-alien.net> <be0088ce0611031846l469b096bl536fec1d243da13f@mail.gmail.com> <20061105011849.GB6669@lor.one-eyed-alien.net> <20061105214041.F44623@delplex.bde.org> <be0088ce0611100055i24d4e309k156aa69147488bfc@mail.gmail.com> <20061110210358.B64521@delplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
2006/11/10, Bruce Evans <bde@zeta.org.au>:
>
> On Fri, 10 Nov 2006, MQ wrote:
>
> > 2006/11/5, Bruce Evans <bde@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@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
>
I see. Though inet_ntoa in the userland will never result in a buffer
overflow, it may return a invalid result to the caller. The inet_ntoa in the
kernel use an and with 0xff to ensure the reuslt is always a IPv4 Address,
so in this aspect, it's better than that in the userland. I really agree
with you that the use of a hard-coded macro is a bad idea. It's still
confusing and less portable than that calculated by the compiler.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?be0088ce0611101749v452476e3i982ae6d1bda72cd9>