From owner-freebsd-net@FreeBSD.ORG Sat Nov 11 01:49:32 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 93CF416A403 for ; Sat, 11 Nov 2006 01:49:32 +0000 (UTC) (envelope-from antinvidia@gmail.com) Received: from nf-out-0910.google.com (nf-out-0910.google.com [64.233.182.191]) by mx1.FreeBSD.org (Postfix) with ESMTP id DF47B43D45 for ; Sat, 11 Nov 2006 01:49:31 +0000 (GMT) (envelope-from antinvidia@gmail.com) Received: by nf-out-0910.google.com with SMTP id p77so996850nfc for ; Fri, 10 Nov 2006 17:49:30 -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=nx8vtOMZ5XUGKBoXwsj6V0oca23RXJsmiRxcoTLNcgfLH2/Dsk8aLEsaimFCnFpprImeQ3wURt0NPdth/9uabubvTNp74nrBvOQtDOQl6lUYf2Nqys0hLeSWt2Q1/UnfFu00WzvL6tXKBAipo7CBsHufB9ERYMgEFZqzb8sg8jA= Received: by 10.78.204.7 with SMTP id b7mr3354152hug.1163209770057; Fri, 10 Nov 2006 17:49:30 -0800 (PST) Received: by 10.78.167.2 with HTTP; Fri, 10 Nov 2006 17:49:29 -0800 (PST) Message-ID: Date: Sat, 11 Nov 2006 01:49:29 +0000 From: MQ To: "Bruce Evans" In-Reply-To: <20061110210358.B64521@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> <20061110210358.B64521@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: Sat, 11 Nov 2006 01:49:32 -0000 2006/11/10, Bruce Evans : > > 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 > 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.