From owner-svn-src-head@freebsd.org Thu Dec 1 23:30:34 2016 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id F1B19C61843; Thu, 1 Dec 2016 23:30:34 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail110.syd.optusnet.com.au (mail110.syd.optusnet.com.au [211.29.132.97]) by mx1.freebsd.org (Postfix) with ESMTP id BDB171A87; Thu, 1 Dec 2016 23:30:34 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c110-21-100-78.carlnfd1.nsw.optusnet.com.au [110.21.100.78]) by mail110.syd.optusnet.com.au (Postfix) with ESMTPS id DC3E67859BC; Fri, 2 Dec 2016 10:30:31 +1100 (AEDT) Date: Fri, 2 Dec 2016 10:30:31 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Eric van Gyzen cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r309364 - head/usr.bin/locale In-Reply-To: <201612011654.uB1Gs2s4023355@repo.freebsd.org> Message-ID: <20161202094516.Q1025@besplex.bde.org> References: <201612011654.uB1Gs2s4023355@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=K7JSJ2eI c=1 sm=1 tr=0 a=uGjuzT6u7JdBDS7kH8taPg==:117 a=uGjuzT6u7JdBDS7kH8taPg==:17 a=kj9zAlcOel0A:10 a=Yu1A3NVHZ7LbY86qSIgA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 01 Dec 2016 23:30:35 -0000 On Thu, 1 Dec 2016, Eric van Gyzen wrote: > Log: > locale: fix buffer management > > Also, handle signed and unsigned chars, and more gracefully handle > invalid input. > > Submitted by: bde in response to r309331 > MFC after: 1 week > Sponsored by: Dell EMC Thanks. > Modified: head/usr.bin/locale/locale.c > ============================================================================== > --- head/usr.bin/locale/locale.c Thu Dec 1 15:46:26 2016 (r309363) > +++ head/usr.bin/locale/locale.c Thu Dec 1 16:54:02 2016 (r309364) > @@ -495,29 +495,29 @@ format_grouping(const char *binary) > { > static char rval[64]; > const char *cp; > - size_t len; > + size_t roff; > + int len; > ... > + if (*cp < 0) > + break; /* garbage input */ > + len = snprintf(&rval[roff], sizeof(rval) - roff, "%u;", *cp); > + if (len < 0 || (unsigned)len >= sizeof(rval) - roff) > + break; /* insufficient space for output */ I don't like casting len to fix compiler warnings, and intentionally left out this cast. len < 0 ensures that there is no sign extension bug or dependency on sign extension bugs. snprintf() returns < 0 if an encoding error occurs, and the check is mainly to detect this unlikely error (the comment is too short to describe this), but the check also allows compilers to easily see that there is no sign extension bug and only low-quality ones warn. I used the bogus type size_t instead of int for roff since I didn't want to fight possible compiler warnings about sign extension in the expression sizeof(rval) - roff. Here it is not so easy to see that roff <= sizeof(rval). You didn't do anything to avoid bad -Wtautological-compare compiler warnings for (*cp < 0). I think such warnings occur at high WARNS on arches with unsigned char. These warnings are bad since the comparison is only tautological on some arches so it is not really tautological. Avoiding it requires code like '#if CHAR_MIN < 0', but that is equally tautological and only escapes the warning because cpp expressions are weaker than C expressions. clang -Wall -Wtautological-compare doesn't actually warn for either (*cp < 0) (where cp is unsigned char *) or (1 < 0). gcc-4.2.1 -Wall warns for the former but not for the latter. This seems sort of backwards, but ISTR discussions in gcc lists ~20 years ago about it being intentional. Warning for (1 < 0) mainly breaks conditional compilation where the condtions are intentially written using C const expressions instead of cpp expressions since cpp is harder to work with. (*cp < 0) is more likely to be a bug. Bruce