From owner-freebsd-net@FreeBSD.ORG Fri Nov 10 10:39:07 2006 Return-Path: X-Original-To: freebsd-net@freebsd.org Delivered-To: freebsd-net@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 2C2BA16A415 for ; Fri, 10 Nov 2006 10:39:07 +0000 (UTC) (envelope-from bde@zeta.org.au) Received: from mailout1.pacific.net.au (mailout1-3.pacific.net.au [61.8.2.210]) by mx1.FreeBSD.org (Postfix) with ESMTP id 820E243D5A for ; Fri, 10 Nov 2006 10:39:06 +0000 (GMT) (envelope-from bde@zeta.org.au) Received: from mailproxy1.pacific.net.au (mailproxy1.pacific.net.au [61.8.2.162]) by mailout1.pacific.net.au (Postfix) with ESMTP id EBFD35A1EE2; Fri, 10 Nov 2006 21:39:04 +1100 (EST) Received: from katana.zip.com.au (katana.zip.com.au [61.8.7.246]) by mailproxy1.pacific.net.au (Postfix) with ESMTP id 765638C18; Fri, 10 Nov 2006 21:39:03 +1100 (EST) Date: Fri, 10 Nov 2006 21:38:57 +1100 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: MQ In-Reply-To: Message-ID: <20061110210358.B64521@delplex.bde.org> References: <20061102142543.GC70915@lor.one-eyed-alien.net> <20061103141732.GA87515@lor.one-eyed-alien.net> <20061105011849.GB6669@lor.one-eyed-alien.net> <20061105214041.F44623@delplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: freebsd-net@freebsd.org Subject: Re: Reentrant problem with inet_ntoa in the kernel X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 10 Nov 2006 10:39:07 -0000 On Fri, 10 Nov 2006, MQ wrote: > 2006/11/5, 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 : >> >> >>> 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