Date: Mon, 1 Jun 2015 20:05:01 +0200 From: Baptiste Daroussin <bapt@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r283842 - head/usr.sbin/pw Message-ID: <20150601180501.GA7523@ivaldir.etoilebsd.net> In-Reply-To: <20150601143951.J863@besplex.bde.org> References: <201505312207.t4VM73Vh015383@svn.freebsd.org> <20150601143951.J863@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--tKW2IUtsqtDRztdT
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Mon, Jun 01, 2015 at 03:36:58PM +1000, Bruce Evans wrote:
> On Sun, 31 May 2015, Baptiste Daroussin wrote:
>=20
> > Log:
> > Remove useless cast in printf and printf-like functions:
> > use %u for uid_t and gid_t
>=20
> 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 subtle=
r).
>=20
> 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.)
Well uid_t is defined as an unsigned int (__uint32_t actually) if it is ever
going to be changed to something else they at least it will now raise a war=
ning.
I consider using %u here is less buggy than the previous casts.
>=20
> 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 practic=
e.
> 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.
>=20
> > 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
> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D
> > --- 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);
>=20
> 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.
>=20
> 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 bit=
s.
>=20
> > Modified: head/usr.sbin/pw/pw_group.c
> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D
> > --- 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 =3D gr_gidpolicy(cnf, args);
> > if (getarg(args, 'q'))
> > return next;
> > - printf("%ld\n", (long)next);
> > + printf("%u\n", next);
> > return EXIT_SUCCESS;
> > }
> >
>=20
> This also has sign changes. gids are supposed to be non-negative, so
> using signed long was probably a bug.
>=20
> 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.
I do plan to work on those atol as well, but I try to be very careful with
pw(8). So I make small modifications one by one.
>=20
> > @@ -137,7 +137,7 @@ pw_group(struct userconf * cnf, int mode
> > else if (rc !=3D 0) {
> > err(EX_IOERR, "group update");
> > }
> > - pw_log(cnf, mode, W_GROUP, "%s(%ld) removed", a_name->val, (long) g=
id);
> > + pw_log(cnf, mode, W_GROUP, "%s(%u) removed", a_name->val, gid);
> > return EXIT_SUCCESS;
> > } else if (mode =3D=3D M_PRINT)
> > return print_group(grp, getarg(args, 'P') !=3D NULL);
>=20
> 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.
well actually this code was actually used, and I checked the output before =
and
after the change and it printed the same output 4294967295.
Best regards,
Bapt
--tKW2IUtsqtDRztdT
Content-Type: application/pgp-signature
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
iEYEARECAAYFAlVsns0ACgkQ8kTtMUmk6ExM/wCgnqGE+gb3+K6o8x8qWsIpuibc
JmIAnR31xOghBTsEQ83xNzIECu2OI7rA
=ZPS8
-----END PGP SIGNATURE-----
--tKW2IUtsqtDRztdT--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150601180501.GA7523>