On Mon, Jun 01, 2015 at 03:36:58PM +1000, Bruce Evans wrote: > 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.)
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 warning.
I consider using %u here is less buggy than the previous casts.
>
> 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.
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.
>
> > @@ -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.
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
pgpPTyTuRBEy0.pgp
Description: PGP signature
