From owner-freebsd-net@FreeBSD.ORG Tue Feb 26 17:28:53 2008 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 363C61065670 for ; Tue, 26 Feb 2008 17:28:53 +0000 (UTC) (envelope-from ithilgore.fbsd@gmail.com) Received: from ug-out-1314.google.com (ug-out-1314.google.com [66.249.92.171]) by mx1.freebsd.org (Postfix) with ESMTP id B1E2113C4DB for ; Tue, 26 Feb 2008 17:28:52 +0000 (UTC) (envelope-from ithilgore.fbsd@gmail.com) Received: by ug-out-1314.google.com with SMTP id y2so1456615uge.37 for ; Tue, 26 Feb 2008 09:28:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:message-id:date:from:user-agent:mime-version:to:cc:subject:references:in-reply-to:content-type:content-transfer-encoding; bh=jtWP6pSelAzE4wngpnIh51XNzeDhenmtFF1x35XTFxg=; b=PH47Pbacx9nQBpeMjDlOHyPFSIHu9KPjbc7UqPN6BzxjlbsJj+zxCBn7XewUxLtLxmugTYkYuBEOEeeuFLcFIVQh+xiKLDzWjX+j9CLnp6TclD0BSp2mn2u/rD6y1NA5evustTq1nXel66crxueWobXgwbOwLpgISUahZmbSQg4= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:user-agent:mime-version:to:cc:subject:references:in-reply-to:content-type:content-transfer-encoding; b=wxa5iXQ7o5CnhYiFprGIoniQ2cjzyMSm4OWVDTQzTFEjjoKlqaWh9rF2ZkpmDYiExZ7rNM13CYBgQhxwx9WmrJEhxfIaaKOK1rJ1BJ1qxmhP86JM31Utulc0skM7XF6mRTn3wkbZB2yiwT1DnFBcFz5I51EYIbN3aP0CBYm1n1E= Received: by 10.78.184.2 with SMTP id h2mr3339522huf.54.1204046930813; Tue, 26 Feb 2008 09:28:50 -0800 (PST) Received: from ?10.0.0.12? ( [62.1.229.152]) by mx.google.com with ESMTPS id i5sm10493345mue.7.2008.02.26.09.28.47 (version=TLSv1/SSLv3 cipher=RC4-MD5); Tue, 26 Feb 2008 09:28:48 -0800 (PST) Message-ID: <47C4D82E.5020209@gmail.com> Date: Tue, 26 Feb 2008 19:25:34 -0800 From: ithilgore User-Agent: Thunderbird 2.0.0.9 (X11/20071212) MIME-Version: 1.0 To: Bruce Evans References: <47BFF17B.5080205@gmail.com> <47BFF74E.4010608@gmail.com> <20080226040438.GA2676@kobe.laptop> <47C46DCF.6050202@gmail.com> <20080227023848.E48510@delplex.bde.org> In-Reply-To: <20080227023848.E48510@delplex.bde.org> Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Cc: Giorgos Keramidas , freebsd-net@freebsd.org Subject: Re: question about change in inet_ntoa.c 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: Tue, 26 Feb 2008 17:28:53 -0000 Bruce Evans wrote: > On Tue, 26 Feb 2008, ithilgore wrote: > >> Giorgos Keramidas wrote: >>> On 2008-02-23 02:37, ithilgore wrote: >>> >>>> ithilgore wrote: >>>> >>>>> I was looking at the differences between some old FreeBSD code >>>>> and the one of 7.0-RC1 and was wondering about a change >>>>> in inet_ntoa.c >>>>> >>>>> /***** 7.0-RC1 **************/ >>>>> >>>>> sprintf(buf, "%d.%d.%d.%d", >>>>> ucp[0] & 0xff, >>>>> ucp[1] & 0xff, >>>>> ucp[2] & 0xff, >>>>> ucp[3] & 0xff); > > This version in libkern is best (except `buf' is static, so it is not > reentrant and thus quite broken). ucp[N] is unsigned char, but masking > with 0xff is needed to support the (unsupported) machines with more > than 8 bits in their chars. On normal machines with 8-bit chars, the > compiler will optimize away the masks so their only cost is increased > portability of the source code. POSIX now requires 8-bit chars, but > didn't when the above was written. %d is good enough after masking, > since the result is nonnegative and <= INT_MAX. %u would be a style > bug except on exotic machines with sizeof(char) == sizeof(int), since > the default promotions normally turn all the numeric printf args into > ints (not u_ints) thanks to C90's broken "value-preserving" promotion > rules. > >>>>> /****** 4.11-RELEASE ***********/ >>>>> >>>>> >>>>> static const char fmt[] = "%u.%u.%u%u"; >>>>> if ((size_t)snprintf(dst, size, fmt, src[0], src[1], src[2], src[3]) >>>>> >= size) { > > This is actually in somewhere/inet_ntop.c:inet_ntop4(). > > Bugs in this version include: > - benign type mismatch between %u and the promoted args except on exotic > machines > - broken on exotic machines. UCHAR_MAX can be arbitarily large. Then > large values will e put in the string. sprintf() could then overrun > the buffer, but using snprintf() avoids that. > - bogus cast of snprintf's return value. > > -current has a different version in libc, without the bogus cast. Many > files in libc/net including inet_ntop.c moved to libc/inet, and some > nearby FreeBSD cleanups were lost (ISC cleaned things up differently > and not so well in one case that I looked at). > >>>>> .... >>>>> .... >>>>> >>>>> Was there a specific purpose of changing the more easy and simple way >>>>> of %u instead of the combination of %d and and-ing with 0xff ?? >>>>> It essentially gives the same result but increases overhead (i >>>>> think) in the more >>>>> recent version. > > I think the libkern version was written later, and it is better because > its author knows what is portable. It's also much simpler. After and-ing > with 0xff, we know the range of the values and don't have to understand > UCHAR_MAX to know that our code is only broken on unsupported/exotic > machines. > >> On the other hand, in version 4.11 RELEASE in >> /usr/src/lib/libc/net/inet_ntoa.c & >> inet_ntop.c (actually it is inet_ntop.c code but with the same >> functionality) > > But hard to find if you are looking for inet_ntoa.c. > > There is also libstand/inet_ntoa.c. It has different bugs and style bugs. > At least in old versions, it combines the worst features of libkern and > libc (static buffer; uses sprintf() and thus can overrun on exotic > machines; > otherwise mainly style bugs like libc). > >> which is >> called by inet_ntoa there is : >> >> static const char * >> inet_ntop4(src, dst, size) >> const u_char *src; >> char *dst; >> size_t size; >> { >> static const char fmt[] = "%u.%u.%u.%u"; >> >> if ((size_t)snprintf(dst, size, fmt, src[0], src[1], src[2], src[3]) >> >= size) { > > Another dubious cleanup in the -current version is to first snprintf() > to a > static buffer of length 16 and copy to the caller's buffer from there. > snprintf() makes that unnecessary unless the API requires not touching > the > caller's buffer on error. > >> errno = ENOSPC; >> return (NULL); >> } >> return (dst); >> } > > Bruce That was enlightening. Thanks.