svn commit: r278634 - head/lib/libc/gen

[Bruce Evans]

On Fri, 13 Feb 2015, Andrey Chernov wrote:

> On 13.02.2015 10:18, Bruce Evans wrote:
>>     if (arg > RLIM_INFINITY)
>>         err(...);
>
> Checking for RLIM_INFINITY is wrong here, since it is ulong long max,

No, it is correct.  rlim_t is neither unsigned not specificially
ulong long.  It is int64_t, which happens to be long long on some
arches.

I already explained that this doesn't conform to POSIX.  More details:
- POSIX reqires rlim_t to be an unsigned type.  This would be uint64_t
   in FreeBSD if FreeBSD conformed.
- POSIX doesn't seem to specify the type of RLIM_INFINITY.  It may be
   much smaller than the max for the type, so its natural type might be
   smaller.  FreeBSD uses the max for the type, then casts the value
   to the type.  The cast does little or nothing except to break use
   of RLIM_INFINITY in cpp arithmetic.  This is exactly what is not
   wanted here -- fixing the problem miight need #if expressions to
   comparing RLIM_INFINITY with LONG_MAX.
- FreeBSD also doesn't implement POSIX's RLIM_SAVED_MAX and
   RLIM_SAVED_CUR.  These are for handling unrepresentable values,
   and might be the best way to handle the unrepresentable values
   that can be asked for by ulimit(3) even more easily than by
   setrlimit(2).  Note that POSIX allows almost any handling for
   unrepresentable values in ulimit(3).  The values can be mapped
   to any garbage and returned.  The attempted fixes are mainly to
   return the same garbage that is mapped to.

> considering
> arg = va_arg(ap, long);
> and ulimit(3) stating that arg is always plain long.

This is a broken as designed API, since long is logically different
from rlim_t, and ulimit()'s ulimits are 512-blocks while rlim_t's
units are bytes, so a scale factor is needed and there would be
representability problems even if the types were the same.  This
gives unrepresentable values in 3 ways:
- on amd64, the types are the same.  LONG_MAX in 512-blocks is 512
   too large to represent in bytes.  But RLIM_INFINITY in bytes is
   easy to represent in 512-blocks.
- on amd64, the garbage arg LONG_MIN in 512-blocks is also 512 times
   too large (in absolute value) to represent in bytes.  So large
   negative values need special handling to preserve their garbageness
   until this can be classified.  (Early versions of the fixed tried
   to push this to setrlimit(), since ulimit()'s bad API enourages
   converting garbage to garbage like setrlimit() will do instead of
   returning an error.  I also want the handling of large negative
   garbage args to be not too different from the handling of small
   negative garbage args.)
- on i386, rlim_t is larger.  RLIM_INFINITY in bytes 2**23 times
   too large to represent in 512-blocks.  (This case was already handled
   correctly.)

> Proper check will be
>
> if (arg < 0) {
>    errno = EINVAL;
>    return (-1);
> }

No, this takes more code, and gives error handling that is not supported
by ulimit(3).  EINVAL for ulimit(3) is only specified or documented to
mean that the command (the first arg) was invalid).

> if (arg > LONG_MAX / 512)
>    arg = LONG_MAX / 512;

This is wrong, since arg > LONG_MAX is not an error when rlim_t is
much larger than long (as on amd64).

Breaking this would break getting and setting the default limit of
RLIM_INFINITY.  This is already broken on i386 and slightly broken
on amd64:
- "get" using ulimit(3) cannot return RLIM_INFINITY since RLIM_INFINITY
   is not a multiple of 512.  Rounding down is specified to occur, and
   does occur.  Then:
   - on amd64, RLIM_INFINITY / 512 is returned
   - on i386, RLIM_INFINITY / 512 is unrepresentable as a long.  The return
     value is explicitly unspecified.  It can be any undocumented garbage.
     FreeBSD returns the undocumented clamped value LONG_MAX.
- "set" of the value returned by "get" cannot restore the original value:
   - on amd64, it sets the value to RLIM_INFINITY / 512 * 512 =
     RLIM_INFINITY rounded down to a multiple of 512
   - on i386, it sets the value to LONG_MAX * 512.  This is about 2**23
     times smaller than the original value.

> That all. In pure theoretical case RLIM_INFINITY is less than LONG_MAX,
> it is job of underlying setrlimit(2) to return error.

No, this cannot be handled by setrlimit(2), since the LONG_MAX cannot
even be passed to setrlimit() then (if RLIM_INFINITY is the max for
its type and not artificially low).

This case occurs in practice (except ulimit(3) should never be used,
and its only known "users" are Coverity and programmers finding and
fixing bugs in it.  My (arg > RLIM_INFINITY) check is simplified to
show a general problem with range checking.  For ulimit(), we
actually need to check (arg * 512 > RLIM_INFINITY), except we write
this as arg > RLIM_INFINITY since the multiplication might overflow.
The multiplication does overflow on amd64 when arg == LONG_MAX.  The
overflow case is precisely when we cannot pass the scaled arg to
setrlimit(2).

Bruce