Re: [patch] Fix BZ 19165 -- overflow in fread / fwrite

[Paul Eggert <eggert at cs dot ucla dot edu>]

On 12/07/2015 09:21 AM, Paul Pluzhnikov wrote:

> +#include <stddef.h>  /* for size_t.  */

Won't this pollute the namespace?  If not, please add a comment 
explaining why not.

> +__always_inline
> +static int
> +__umul_size_t_overflow (size_t a, size_t b)
> +{
> +#if __GNUC_PREREQ (5, 0)
> +  size_t result;
> +
> +  /* _Static_assert, but without triggering conformance test failures.  */

Can this problem, whatever it is, be fixed by altering _Static_assert in 
cdefs.h, rather than avoiding use of _Static_assert?

> +  __attribute__ ((__unused__))
> +  char sizeof_size_t_eq_sizeof_ulong[
> +    sizeof (size_t) == sizeof (unsigned long) ? 1 : -1];
> +
> +  return __builtin_umull_overflow (a, b, &result);

This should use __builtin_mul_overflow instead, so that you don't need 
to worry about checking whether size_t is the same width as unsigned long.

> +#else
> +  const size_t half_size_t = (size_t) 1 << 4 * sizeof (size_t);
> +  const size_t size_max = (size_t) -1;

The 4 is a mystery constant.  Better would be to declare half_size_t = 
size_max / 2 + 1.

Generally speaking let's not bother adding 'const' to locals that aren't 
later assigned to, as these 'const's are more trouble than they're 
worth.  (Similarly for 'register' and locals whose addresses are not taken.)

All the locals need to be protected with leading "__", since I assume 
this .h file can be included from user code.

Defining otherwise-unused static functions in headers will lead to 
problems with picky compilers.


The above comments suggest that this patch is part of a larger 
maintenance problem with glibc and integer overflow checking.  To 
simplify maintenance, I suggest instead that we leave cdefs.h alone, and 
instead copy and include gnulib's intprops.h for internal use only 
inside glibc, and use intprops.h's macro 'INT_MULTIPLY_OVERFLOW (a, b)' 
instead of defining and using '__umul_size_t_overflow (a, b)'.  This 
would simplify maintenance in the long run, as it already incorporates 
all the above comments.  Please see:

http://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/intprops.h