From owner-svn-src-all@FreeBSD.ORG Mon Jun 1 05:37:04 2015 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 6DB2F6BB; Mon, 1 Jun 2015 05:37:04 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail104.syd.optusnet.com.au (mail104.syd.optusnet.com.au [211.29.132.246]) by mx1.freebsd.org (Postfix) with ESMTP id 765FF1015; Mon, 1 Jun 2015 05:37:03 +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 mail104.syd.optusnet.com.au (Postfix) with ESMTPS id 93B3142157E; Mon, 1 Jun 2015 15:36:59 +1000 (AEST) Date: Mon, 1 Jun 2015 15:36:58 +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: r283842 - head/usr.sbin/pw In-Reply-To: <201505312207.t4VM73Vh015383@svn.freebsd.org> Message-ID: <20150601143951.J863@besplex.bde.org> References: <201505312207.t4VM73Vh015383@svn.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=QeFf4Krv c=1 sm=1 tr=0 a=KA6XNC2GZCFrdESI5ZmdjQ==:117 a=PO7r1zJSAAAA:8 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=xqHopV17G1f5rAXcRQkA:9 a=fZT5EF8Kno1c1QlV:21 a=XF-A_rSnECe766qY:21 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.20 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: Mon, 01 Jun 2015 05:37:04 -0000 On Sun, 31 May 2015, Baptiste Daroussin wrote: > Log: > Remove useless cast in printf and printf-like functions: > use %u for uid_t and gid_t The cast was not useless. It was to avoid the assumption that the default promotion of uid_t and gid_t is anything in particular. Now it is assumed that the default promotion is unsigned (int works too, but this is subtler). uids and gids are only guaranteed to have non-negative values. In POSIX before about 2001, uid_t and gid_t can be any type that can represent all the values taken on, so can be floating point. Floating point was rarely used for POSIX types, and most programs make too many assumptions about types, so POSIX now requires uid_t and gid_t to be integer types. Then can still be signed integer types IIRC. Nornally it is a bug to print signed integer types with unsigned integer formats, but since uids and gids are guaranteed to be non-negative both formats work. (pids require different handling since they are overloaded to hold process group ids as negative values, so pid_t is signed and %u format is very broken for printing general pids.) The program assumed that uids and gids are not too large to be represented by unsigned long. This was the only way to print them in C90 and before. C99 broke this by breaking the promise that unsigned long is the largest unsigned integer type. This broke all code that does careful casts to unsigned long. However, unsigned long is usually large enough in practice. Careful code now has to cast to uintmax_t, but that is usually excessive (but doesn't actually work for __uint128_t). Even plain unsigned usually works on vaxes. > Modified: > head/usr.sbin/pw/pw_conf.c > head/usr.sbin/pw/pw_group.c > head/usr.sbin/pw/pw_user.c > > Modified: head/usr.sbin/pw/pw_conf.c > ============================================================================== > --- head/usr.sbin/pw/pw_conf.c Sun May 31 21:44:09 2015 (r283841) > +++ head/usr.sbin/pw/pw_conf.c Sun May 31 22:07:03 2015 (r283842) > @@ -453,19 +453,19 @@ write_userconfig(char const * file) > config.default_class : ""); > break; > case _UC_MINUID: > - sbuf_printf(buf, "%lu", (unsigned long) config.min_uid); > + sbuf_printf(buf, "%u", config.min_uid); This works accidentally even on some non-vaxes. All supported arches in FreeBSD have 32-bit unsigned's, and POSIX was broken in about 2007 to disallow 16-bit unsigned's. uid_t is uint32_t in FreeBSD. However, it is planned to change ino_t and some other types to 64 bits. I forget if the plans include uid_t. Even 64-bit ino_t is too large for me. The version with the cast to unsigned long is not really better. Suppose uid_t is expanded to 16 bits. Then without the cast, the size mismatch is detected. With it, everything still works if unsigned long is 64 bits, but if unsigned long is 32 bits then the cast just breaks detection of the mismatch and gives wrong results for values that actually need 64 bits. > Modified: head/usr.sbin/pw/pw_group.c > ============================================================================== > --- head/usr.sbin/pw/pw_group.c Sun May 31 21:44:09 2015 (r283841) > +++ head/usr.sbin/pw/pw_group.c Sun May 31 22:07:03 2015 (r283842) > @@ -83,7 +83,7 @@ pw_group(struct userconf * cnf, int mode > gid_t next = gr_gidpolicy(cnf, args); > if (getarg(args, 'q')) > return next; > - printf("%ld\n", (long)next); > + printf("%u\n", next); > return EXIT_SUCCESS; > } > This also has sign changes. gids are supposed to be non-negative, so using signed long was probably a bug. However, the program is elsewhere clueless about types, and I think you did't change the parts where it uses atol() to misparse input. atol() can't even handle 32-bit uids on 32-bit arches. It truncates large positive ones to LONG_MAX. It returns negative values (truncated to LONG_MIN if necessary). The misparsing then doesn't detect the error of negative ids, but blindly casts to uid_t or gid_t, of course using lexical style bugs in the casts. The magic ids -1 and -2 are sometimes needed (perhaps not as input in pw, but -1 is used as an error value and this value can be input). These values don't really exist as ids. They must be cast before use. The actual values just cannot be entered, since atol() misparses them (it truncates large values, and also doesn't support hex, so (uid_t)-2 must be written as a long decimal number to input it for misparsing. > @@ -137,7 +137,7 @@ pw_group(struct userconf * cnf, int mode > else if (rc != 0) { > err(EX_IOERR, "group update"); > } > - pw_log(cnf, mode, W_GROUP, "%s(%ld) removed", a_name->val, (long) gid); > + pw_log(cnf, mode, W_GROUP, "%s(%u) removed", a_name->val, gid); > return EXIT_SUCCESS; > } else if (mode == M_PRINT) > return print_group(grp, getarg(args, 'P') != NULL); If the error gid (gid_t)-1 is ever printed, now %u format prints it as a cryptic decimal number where %ld format printed it as -1. Bruce