From owner-svn-src-head@freebsd.org Sat Aug 1 10:36:00 2015 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 10C789B0915; Sat, 1 Aug 2015 10:36:00 +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 4305B15C3; Sat, 1 Aug 2015 10:35:59 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c211-30-166-197.carlnfd1.nsw.optusnet.com.au (c211-30-166-197.carlnfd1.nsw.optusnet.com.au [211.30.166.197]) by mail110.syd.optusnet.com.au (Postfix) with ESMTPS id 9FA6A781F9A; Sat, 1 Aug 2015 20:35:57 +1000 (AEST) Date: Sat, 1 Aug 2015 20:35:56 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Baptiste Daroussin cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r286148 - head/usr.sbin/chkgrp In-Reply-To: <201508010835.t718ZKiH035944@repo.freebsd.org> Message-ID: <20150801200929.F2212@besplex.bde.org> References: <201508010835.t718ZKiH035944@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.1 cv=XMDNMlVE c=1 sm=1 tr=0 a=KA6XNC2GZCFrdESI5ZmdjQ==:117 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=kj9zAlcOel0A:10 a=dXZubP7t6yK9USa-kFYA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.20 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: Sat, 01 Aug 2015 10:36:00 -0000 On Sat, 1 Aug 2015, Baptiste Daroussin wrote: > Log: > Use strtoumax instead of strtoul This does nothing good, and breaks 32-bit arches. > Modified: head/usr.sbin/chkgrp/chkgrp.c > ============================================================================== > --- head/usr.sbin/chkgrp/chkgrp.c Sat Aug 1 07:51:48 2015 (r286147) > +++ head/usr.sbin/chkgrp/chkgrp.c Sat Aug 1 08:35:20 2015 (r286148) > @@ -169,9 +170,9 @@ main(int argc, char *argv[]) > > /* check the range of the group id */ > errno = 0; > - gid = strtoul(f[2], NULL, 10); > + gid = strtoumax(f[2], NULL, 10); gid still has type u_long (spelled verbosely as unsigned long.) This used to work on arches where gid_t is no larger than u_long, which is the case on all supported arches. Now the value returned by strtoumax() is corrupted by blindly assigning it to u_long. E.g., 0x100000001 becomes 1. > if (errno != 0) { > - warnx("%s: line %d: strtoul failed", gfn, n); > + warnx("%s: line %d: strtoumax failed", gfn, n); This has a less usual subset of common bugs in using the strtoul() family. The most common bug is to not check ERANGE. This does check ERANGE, but doesn't check for garbage following the integer (it doesn't even pass endptr). It uses a POSIX extension to check that the integer has some digits. > } else if (gid > GID_MAX) { This used to work. Now it checks the corrupted value. > warnx("%s: line %d: group id is too large (%ju > %ju)", > gfn, n, (uintmax_t)gid, (uintmax_t)GID_MAX); This format is bogus for printing `gid'. gid should have type gid_t, but actually has type u_long. u_long should be printed using %lu and no cast. GID_MAX is undocumented, so no one knows its type. Its type should be gid_t, but is actually u_int. gid_t is logically different again -- it is __uint32_t. GID_MAX is defined in sys/limits.h, so there are namespace problems in matching its type with that of gid_t. The current implementation is best until gid_t is expanded. So casting GID_MAX to uintmax_t is good for safety and portability. However, chckgrp.c already assumed that gid_t is no larger than u_long. Otherwise it would have been broken in the reverse way to this commit -- values would have been clamped by strtoul(). The bounds checking would have worked, but large values would have been unsupported. The old value might as well have cast to u_long. strtoumax() can support any size for gid_t. After fixing all the other type errors, the types in the warnx() become correct. Bruce