From owner-freebsd-bugs@FreeBSD.ORG Tue Jan 23 06:11:32 2007 Return-Path: X-Original-To: freebsd-bugs@FreeBSD.org Delivered-To: freebsd-bugs@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id F079816A403; Tue, 23 Jan 2007 06:11:31 +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 8EC7913C441; Tue, 23 Jan 2007 06:11:31 +0000 (UTC) (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 2A8EC5A1C71; Tue, 23 Jan 2007 17:11:30 +1100 (EST) Received: from besplex.bde.org (katana.zip.com.au [61.8.7.246]) by mailproxy1.pacific.net.au (Postfix) with ESMTP id 8422F8C1D; Tue, 23 Jan 2007 17:11:28 +1100 (EST) Date: Tue, 23 Jan 2007 17:11:27 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Yong Tang In-Reply-To: <200701221445.l0MEjRFA090791@www.freebsd.org> Message-ID: <20070123155916.F10998@besplex.bde.org> References: <200701221445.l0MEjRFA090791@www.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: green@freebsd.org, freebsd-bugs@FreeBSD.org, freebsd-gnats-submit@FreeBSD.org Subject: Re: misc/108211: potentially a bug for inet_aton in sys/netinet/libalias/alias_proxy.c X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Jan 2007 06:11:32 -0000 On Mon, 22 Jan 2007, Yong Tang wrote: >> Description: > In sys/netinet/libalias/alias_proxy.c, > The following code exist. > > 158 #ifdef _KERNEL > 159 static int > 160 inet_aton(cp, addr) > 161 const char *cp; > 162 struct in_addr *addr; > 163 { > > > 180 l = strtoul(c, &endptr, 0); > 181 > 182 if (l == ULONG_MAX || l == 0) > 183 return (0); > > However, if the input cp is "0.0.0.0", then it seems this function will return (0) which is considered as an error. Bleah, yet another example of how not to check for errors in the strto* family. > The reason is because 180: > l = strtoul(c, &endptr, 0); > l will return a 0 when the c is "0". > > Not quite sure if this is done purposely in FreeBSD but I have never experience similiar cases in other unix-platforms. > > Possible solution: > > change > 182 (l == ULONG_MAX || l == 0) > > into > 182 (l == ULONG_MAX || (l == 0 && (endptr == c)) Why not just use libc's inet_aton()? Well, there is a problem -- this is for the kernel, so inet_aton() must be duplicated, but it can't be duplicated literally since the strto* family is fundamentally broken in the kernel. strto*'s API depends on errno for reporting errors correctly, and errno doesn't exist in the kernel. This kernel version is a literal duplicate of the libc version except for massive bitrot. It is identical to an old libc version (libc/net/inet_addr.c 1.16) except for small changes to avoid avoid checking errno and thus implement the bug, and minor bitrot in the libc version (const poisoning has only been done in the kernel). The massive bitrot in the current libc version (libc/inet/inet_addr.c 1.3) is mainly to implement a home made broken version of strtoul() inline. Perhaps strtoul() is too inflexible, but I doubt it. The must obvious bug in the new version is the usual overflow in home made implementations of atoi/strto*: "val = val * base + 'c' - '0'" may overflow, but there is no overflow checking. Thus the garbage input "4294967297.1.1.1" is now treated as "1.1.1.1" on systems with 32-bit unsigned longs although it is detected as being garbage on systems with 64-bit unsigned longs. Some style bugs are also obvious. Possibly simpler solution for 1 kernel version and 2 userland versions: - remove all traces of errno and related bogus checks for (l == ULONG_MAX) and (l == 0). Checking (errno == ERANGE) isn't wrong, but it isn't needed since the value will be ULONG_MAX after a range error, and since ULONG_MAX is too large for part of a dotted quad address it will be detected as an error later. Checking (l == ULONG_MAX) is wrong in general since it might not be a the result of a range error, but here the range is smaller so the check isn't wrong. However, the check is unnecessary since all values between 256 and ULONG_MAX inclusive will be dected as errors later. Checking (l == 0) is just a bug since 0 is within range. - remove broken home made strtol() in libc version, and otherwise merge versions and fix bitrot. The "new" libc version is actually not newer, but just bitrot back to the vendor version. FreeBSD fixed "old" version to use strtoul() in rev.1.8 in 1999, but the changes were lost when the vendor-version was reimported in a different directory in 2006. FreeBSD also made a few fixes to the old version between 1.8 and 1.16. The main one is related: 1.8 showed yet another way of how not to check for errors in strto* -- it checked (l == ERANGE) instead of (errno == ERANGE). Some of its other changes are not quite right. Bruce