svn commit: r283842 - head/usr.sbin/pw

[Baptiste Daroussin]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-all/attachments/20150601/8b958bff/attachment.sig>