From owner-svn-src-head@freebsd.org Wed Jul 29 02:09:13 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 043C49AD14F; Wed, 29 Jul 2015 02:09:13 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id B49131B04; Wed, 29 Jul 2015 02:09:11 +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 mail108.syd.optusnet.com.au (Postfix) with ESMTPS id 9EBE01A5188; Wed, 29 Jul 2015 12:09:08 +1000 (AEST) Date: Wed, 29 Jul 2015 12:09:07 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Baptiste Daroussin cc: Bruce Evans , svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r285985 - in head/usr.sbin/pw: . tests In-Reply-To: <20150728232106.GG28638@ivaldir.etoilebsd.net> Message-ID: <20150729110330.O5544@besplex.bde.org> References: <201507282110.t6SLAx0k035167@repo.freebsd.org> <20150729080932.S5059@besplex.bde.org> <20150728232106.GG28638@ivaldir.etoilebsd.net> 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=7YfXLusrAAAA:8 a=3a5ZI3m13Z8Cm56kV6UA:9 a=1vtwie4u5U5GghCb:21 a=S4hxu90T8b2Nxqgm:21 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: Wed, 29 Jul 2015 02:09:13 -0000 On Wed, 29 Jul 2015, Baptiste Daroussin wrote: > On Wed, Jul 29, 2015 at 08:52:52AM +1000, Bruce Evans wrote: >> On Tue, 28 Jul 2015, Baptiste Daroussin wrote: >> >>> Log: >>> Check uid/gid used when creating a user/group are not larger than UID_MAX/GID_MAX >>> >>> PR: 173977 >>> Reported by: nvass@gmx.com >> >> This is broken in a different way than before. >> >>> Modified: head/usr.sbin/pw/pw.c >>> ============================================================================== >>> --- head/usr.sbin/pw/pw.c Tue Jul 28 20:52:10 2015 (r285984) >>> +++ head/usr.sbin/pw/pw.c Tue Jul 28 21:10:58 2015 (r285985) >>> @@ -269,7 +269,7 @@ main(int argc, char *argv[]) >>> } >>> if (strspn(optarg, "0123456789") != strlen(optarg)) >>> errx(EX_USAGE, "-g expects a number"); >>> - id = strtonum(optarg, 0, LONG_MAX, &errstr); >>> + id = strtonum(optarg, 0, GID_MAX, &errstr); >> >> `id' still has type long. The assignment overflows on 32-bit arches when >> the value exceeds 0x7fffffff. That is for half of all valid values. pw >> is broken in not supporting these values, but at least it detected them >> as errors in the previous version. Old versions implemented this bug >> using atoi() with no error checking. > > So writting a function like strtonum like function with that prototype > intmax_t strtonumber(const char *, intmax_t min, intmax_t max, const char **); > and an unsigned equivalent > uintmax_t strtonumber(const char *, uintmax_t min, uintmax_t max, const char **); Not completely, since this would restore some of the complications of the strto*() family (there is API explosion, and types to match). These 2 functions don't even handle all the integer ranges (with assymmetric signedness), or hex numbers, or floating point). To match the types for gid_t's with these APIs, you could write: gid_t id = strtounum(optarg, 0, GID_MAX, &errstr); or uintmax_t id = strtounum(optarg, 0, GID_MAX, &errstr); but can't mix these id types with others that need different signedness.. or use plain int/long/u_long for anything. This is not a problem for uids and gids since they usually have the same underlying type. POSIX (XSI) made a mess of this for waitid(idtype_t idtype, id_t id, ...). id can be either a pid, a uid, a gid or a session id. So id_t is specified as an integer type that can "contain" all of these. Representing all of these is generally impossible due to the sign differences, and the conversions made to "contain" seem to be unspecified. FreeBSD actually uses int64_t for id_t and an enum for idtype_t, so simple conversions preserve the value at the cost of bloat. This wouldn't work with 64-bit uid_t. A similar mess can be made for strto*num() by specifying it to work with a container type num_t and making this type opaque so that it is hard to use :-), except there no strict container exists for int64_t and uint64_t. Bruce