[Bug 198377] libc: Invalid size check in load_msgcat()

[Bruce Evans]

On Fri, 6 Mar 2015 bugzilla-noreply at freebsd.org wrote:

> According to coverity 1193663, the following check always yields a false
> result:
>
>
> 405    if (st.st_size > SIZE_T_MAX) {
> 406        _close(fd);
> 407        SAVEFAIL(name, lang, EFBIG);
> 408        NLRETERR(EFBIG);
> 409    }
> _____
>
> result_independent_of_operands: st.st_size > 18446744073709551615ULL is always
> false regardless of the values of its operands. This occurs as the logical
> operand of if.

Bug in coverity.  This test is only always false on arches where SIZE_T_MAX
exceeds OFF_MAX.  This happens to occur on amd64.

Coverity also prints SIZE_T_MAX with a wrong type and a cryptic format.
Coverity uses the long long abomination, but SIZE_T_MAX doesn't even have
type unsigned long long on amd64; it has type unsigned long.  SIZE_T_MAX
is defined in hex so that it is readable, but Coverity prints it (or
the limit of the type) in decimal.

BTW, SIZE_T_MAX shouldn't exist, and there are related style bugs in the
definition of SIZE_MAX.  SIZE_T_MAX is apparently the 4.4BSD spelling of
SIZE_MAX.  C90 neglected to define SIZE_MAX, but this was fixed in C99,
so there should be only SIZE_MAX now, except possibly under a
compatibility ifdef.  FreeBSD uses logically inconsistent types for
the definitions of various SIZE_MAX's: on amd64:
- SSIZE_MAX = __SSIZE_MAX is LONG_MAX
- SIZE_T_MAX = __SIZE_T_MAX is ULONG_MAX
_ SIZE_MAX = __SIZE_MAX is UINT64_MAX
__SIZE_MAX is defined with a different style than most old limits
including __SIZE_T_MAX.  It uses a fixed-width type.  This is not
needed size the width of u_long is known.  The old limits had to
used basic types since fixed width types were not available before
C99.  The types of these limits end up being correct since care is
taken elsewhere.

BTW, the definition of OFF_MAX is broken in sys/limits.h.  It is under a
POSIX-2001 ifdef, but no version of POSIX except Strawman draft 6 has it
according to google.

> We can workaround this by excluding also SIZE_T_MAX but we should also exclude
> negative values since that would indicate an overflow.

No, negative values indicate a kernel bug.  st_size cannot be negative.

The warning might actually be about potential sign extension bugs on
64-bit arches only.  On 32-bit arches, SIZE_T_MAX promotes to (signed)
int64_t for comparison with st_size, so there is no problem (unless
there is a kernel bug -- negative values would not be detected
accidentally).  On 64-bit arches, st_size promotes to u_long for
comparison with SIZE_T_MAX.  Again there is no problem unless there
is a kernel bug, but Coverity can reasonably warn about the promotion
being dangerous.

Error checking is hard enough to do without requiring ifdefs or other
complications to avoid doing it when it happens to be null.  Such ifdefs
are hard to write.  Here one can be written using the nonstandard OFF_MAX.
Without OFF_MAX, you would need a much larger ifdef to first define
OFF_MAX.  Promotion of limits in cpp expressions can be tricky, since
the promotion is to intmax_t or uintmax_t, but C expressions normally
promote to smaller types.

load_msgcat() may also be broken in using st_size or mmap() at all.
st_size is only valid for regular files, but load_msgcat() uses it
unconditionally.  But load_msgcat() also uses mmap() and fails if
it doesn't work.  So it more or less needs the file to be regular.
For non-regular files, that can be mmapped, the usual behaviour is
for st_size to be 0; the test is then passed but mmap() is asked
to map 0 bytes; even if that succeeds the map is not useful.

Bruce