This is the mail archive of the mailing list for the glibc project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

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.

+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.

+  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:

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]