From owner-freebsd-net@FreeBSD.ORG Tue Feb 26 18:46:26 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 E88A01065677 for ; Tue, 26 Feb 2008 18:46:26 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from fallbackmx10.syd.optusnet.com.au (fallbackmx10.syd.optusnet.com.au [211.29.132.251]) by mx1.freebsd.org (Postfix) with ESMTP id 9FC4B13C459 for ; Tue, 26 Feb 2008 18:46:25 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail15.syd.optusnet.com.au (mail15.syd.optusnet.com.au [211.29.132.196]) by fallbackmx10.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id m1QG5oEv020666 for ; Wed, 27 Feb 2008 03:05:51 +1100 Received: from c220-239-252-11.carlnfd3.nsw.optusnet.com.au (c220-239-252-11.carlnfd3.nsw.optusnet.com.au [220.239.252.11]) by mail15.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id m1QG5fld008480 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 27 Feb 2008 03:05:44 +1100 Date: Wed, 27 Feb 2008 03:05:41 +1100 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: ithilgore In-Reply-To: <47C46DCF.6050202@gmail.com> Message-ID: <20080227023848.E48510@delplex.bde.org> References: <47BFF17B.5080205@gmail.com> <47BFF74E.4010608@gmail.com> <20080226040438.GA2676@kobe.laptop> <47C46DCF.6050202@gmail.com> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed 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 18:46:27 -0000 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