From owner-freebsd-net@FreeBSD.ORG Fri Nov 10 08:55:41 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 92BED16A40F for ; Fri, 10 Nov 2006 08:55:41 +0000 (UTC) (envelope-from antinvidia@gmail.com) Received: from ug-out-1314.google.com (ug-out-1314.google.com [66.249.92.170]) by mx1.FreeBSD.org (Postfix) with ESMTP id D34BE43D4C for ; Fri, 10 Nov 2006 08:55:40 +0000 (GMT) (envelope-from antinvidia@gmail.com) Received: by ug-out-1314.google.com with SMTP id o2so425190uge for ; Fri, 10 Nov 2006 00:55:40 -0800 (PST) DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=beta; d=gmail.com; h=received:message-id:date:from:to:subject:cc:in-reply-to:mime-version:content-type:references; b=GUOPKByo7pq+9lqQE4PiiSjITcO71szLwRIY1K7B1O8qaumwcLkmheYtEM4HxcTxsH4MJqbEiKiBf3xS4uPs2mi1NOwBsHisFLKgcVOHEFAjPTzvjzkEkEPDIlypsYdeL/7VJQioMjofU4Pd9Eao3VdS9FOCgJTDrOJVwJ6pmT4= Received: by 10.78.117.10 with SMTP id p10mr2385984huc.1163148940039; Fri, 10 Nov 2006 00:55:40 -0800 (PST) Received: by 10.78.167.2 with HTTP; Fri, 10 Nov 2006 00:55:39 -0800 (PST) Message-ID: Date: Fri, 10 Nov 2006 08:55:39 +0000 From: MQ To: "Bruce Evans" In-Reply-To: <20061105214041.F44623@delplex.bde.org> MIME-Version: 1.0 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> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-Disposition: inline X-Content-Filtered-By: Mailman/MimeDel 2.1.5 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 08:55:41 -0000 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 > 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.