From owner-freebsd-net@FreeBSD.ORG Sun Nov 5 12:35:19 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 65D1216A4C2 for ; Sun, 5 Nov 2006 12:35:19 +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 8FE6343D98 for ; Sun, 5 Nov 2006 12:34:33 +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 6DE3B5A0EA8; Sun, 5 Nov 2006 23:34:22 +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 E43DD8C03; Sun, 5 Nov 2006 23:34:20 +1100 (EST) Date: Sun, 5 Nov 2006 23:34:20 +1100 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Brooks Davis In-Reply-To: <20061105011849.GB6669@lor.one-eyed-alien.net> Message-ID: <20061105214041.F44623@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> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: freebsd-net@FreeBSD.org, MQ 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: Sun, 05 Nov 2006 12:35:19 -0000 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 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