Date: Mon, 1 Jun 2015 15:36:58 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Baptiste Daroussin <bapt@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r283842 - head/usr.sbin/pw Message-ID: <20150601143951.J863@besplex.bde.org> In-Reply-To: <201505312207.t4VM73Vh015383@svn.freebsd.org> References: <201505312207.t4VM73Vh015383@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150601143951.J863>