From owner-svn-src-all@FreeBSD.ORG Thu Nov 15 16:53:03 2012 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 53C4BF87; Thu, 15 Nov 2012 16:53:03 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail06.syd.optusnet.com.au (mail06.syd.optusnet.com.au [211.29.132.187]) by mx1.freebsd.org (Postfix) with ESMTP id D49E98FC12; Thu, 15 Nov 2012 16:53:02 +0000 (UTC) Received: from c122-106-175-26.carlnfd1.nsw.optusnet.com.au (c122-106-175-26.carlnfd1.nsw.optusnet.com.au [122.106.175.26]) by mail06.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id qAFGqwKg030658 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 16 Nov 2012 03:52:59 +1100 Date: Fri, 16 Nov 2012 03:52:58 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Eitan Adler Subject: Re: svn commit: r243076 - head/usr.sbin/chkgrp In-Reply-To: Message-ID: <20121116032851.I44199@besplex.bde.org> References: <201211151506.qAFF63v0003848@svn.freebsd.org> <20121115153030.GD73505@kib.kiev.ua> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-Cloudmark-Score: 0 X-Optus-Cloudmark-Analysis: v=2.0 cv=XbrRV/F5 c=1 sm=1 a=IpyuoPnKT5oA:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=hf-KJMUURdMA:10 a=pGLkceISAAAA:8 a=6I5d2MoRAAAA:8 a=bIPi5WGLedxrlG5P314A:9 a=CjuIK1q_8ugA:10 a=MSl-tDqOz04A:10 a=5swDdGOMujkHuVB2:21 a=RFbyZM59x8Qu5x2j:21 a=bxQHXO5Py4tHmhUgaywp5w==:117 Cc: Konstantin Belousov , svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 15 Nov 2012 16:53:03 -0000 On Thu, 15 Nov 2012, Eitan Adler wrote: > On 15 November 2012 10:30, Konstantin Belousov wrote: >> On Thu, Nov 15, 2012 at 03:06:03PM +0000, Eitan Adler wrote: >>> Author: eadler >>> Date: Thu Nov 15 15:06:03 2012 >>> New Revision: 243076 >>> URL: http://svnweb.freebsd.org/changeset/base/243076 >>> >>> Log: >>> Check the range of the gid >>> >>> Approved by: cperciva >>> MFC after: 1 week >>> >>> Modified: >>> head/usr.sbin/chkgrp/chkgrp.c >>> >>> Modified: head/usr.sbin/chkgrp/chkgrp.c >>> ============================================================================== >>> --- head/usr.sbin/chkgrp/chkgrp.c Thu Nov 15 15:06:00 2012 (r243075) >>> +++ head/usr.sbin/chkgrp/chkgrp.c Thu Nov 15 15:06:03 2012 (r243076) >>> @@ -30,7 +30,10 @@ >>> __FBSDID("$FreeBSD$"); >>> >>> #include >>> +#include >>> #include >>> +#include >>> +#include >>> #include >>> #include >>> #include >>> @@ -150,6 +153,18 @@ main(int argc, char *argv[]) >>> warnx("%s: line %d: GID is not numeric", gfn, n); >>> e++; >>> } >>> + >>> + /* check the range of the group id */ >>> + errno = 0; >>> + unsigned long groupid = strtoul(f[2], NULL, 10); >> And this violates style. I see about 5 style violations here and 5 in other lines. >> The checks for strtoul failure are not exhaustive. > > from the strtoul man page: > > ==== > ... In all cases, errno is set to ERANGE. > If no conversion could be performed, 0 is returned and > the global variable errno is set to EINVAL (the last feature is not por- > table across all platforms). > === > > What is missing? Is there a case where strtoul fails but errno == 0 ? strtoul("1garbage", NULL, 10) succeeds and returns value 1, but the input is garbage. As the man page says, the EINVAL feature is unportable. It is almost useless, since to detect garbage after the number you have to pass an endptr to strtoul(), and then the check for no conversion (that is, for garbage at the beginning) is just as easy as the check for garbage at the end. The error handling is poor. First, warnx() is used instead of warn() so as to not tell the user about the errno that we are depending on. Fully correct and laborious strto*() error reporting has to use warnx() only for the case(s) where the input is garbage but strtoul() doesn't set errno. High quality strto*() error reporting also avoids using strerror(errno) in other cases to get a uniform style of error messages. The only portable other case is ERANGE for a range error. strerror(errno) is quite good for that. However, we do another range check later and should use the same error message for both. strerror(EINVAL) is cryptic for garbage at the beginning of the input as well as unportable. Second, we continue using the garbage value after detecting that it is garbage. strtonum() and expand_number() handle some of these problems internally but create others. For example, if you don't like the error string created by strtonum(), then you have to parse it to see what it says in order to write a better one. This is much more work than doing your own range checking. Bruce