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

[Paul Pluzhnikov]

On Mon, Oct 26, 2015 at 1:06 PM, Rich Felker <dalias@libc.org> wrote:

> This highly pessimizes short reads/writes, e.g. fwrite(&c,1,1,f), by
> introducing a div operation. The obvious intent of the original code
> was to avoid this.

Thanks for comments, everyone.

Revised patch addresses all of them (I believe). Not sure whether the
name or location of the saturating multiplication is acceptable
though. Suggestions welcome.

2015-10-26  Paul Pluzhnikov  <ppluzhnikov@google.com>

[BZ #19165]
        * libio/libioP.h (_IO_saturating_umull): New.
        * libio/iofread.c (_IO_fread): Use it.
        * ibio/iofread_u.c (__fread_unlocked): Likewise.
        * libio/iofwrite.c (_IO_fwrite): Error on overflow.
        * libio/iofwrite_u.c (fwrite_unlocked): Likewise.

-- 
Paul Pluzhnikov
-------------- next part --------------
diff --git a/libio/iofread.c b/libio/iofread.c
index eb69b05..d2a42bc 100644
--- a/libio/iofread.c
+++ b/libio/iofread.c
@@ -29,7 +29,7 @@
 _IO_size_t
 _IO_fread (void *buf, _IO_size_t size, _IO_size_t count, _IO_FILE *fp)
 {
-  _IO_size_t bytes_requested = size * count;
+  _IO_size_t bytes_requested = _IO_saturating_umull (size, count);
   _IO_size_t bytes_read;
   CHECK_FILE (fp, 0);
   if (bytes_requested == 0)
diff --git a/libio/iofread_u.c b/libio/iofread_u.c
index 997b714..8c3a821 100644
--- a/libio/iofread_u.c
+++ b/libio/iofread_u.c
@@ -32,7 +32,7 @@
 _IO_size_t
 __fread_unlocked (void *buf, _IO_size_t size, _IO_size_t count, _IO_FILE *fp)
 {
-  _IO_size_t bytes_requested = size * count;
+  _IO_size_t bytes_requested = _IO_saturating_umull (size, count);
   _IO_size_t bytes_read;
   CHECK_FILE (fp, 0);
   if (bytes_requested == 0)
diff --git a/libio/iofwrite.c b/libio/iofwrite.c
index 48ad4bc..9bce404 100644
--- a/libio/iofwrite.c
+++ b/libio/iofwrite.c
@@ -29,7 +29,7 @@
 _IO_size_t
 _IO_fwrite (const void *buf, _IO_size_t size, _IO_size_t count, _IO_FILE *fp)
 {
-  _IO_size_t request = size * count;
+  _IO_size_t request = _IO_saturating_umull (size, count);
   _IO_size_t written = 0;
   CHECK_FILE (fp, 0);
   if (request == 0)
diff --git a/libio/iofwrite_u.c b/libio/iofwrite_u.c
index 2b1c47a..081a7b1 100644
--- a/libio/iofwrite_u.c
+++ b/libio/iofwrite_u.c
@@ -33,7 +33,7 @@ _IO_size_t
 fwrite_unlocked (const void *buf, _IO_size_t size, _IO_size_t count,
 		 _IO_FILE *fp)
 {
-  _IO_size_t request = size * count;
+  _IO_size_t request = _IO_saturating_umull (size, count);
   _IO_size_t written = 0;
   CHECK_FILE (fp, 0);
   if (request == 0)
diff --git a/libio/libioP.h b/libio/libioP.h
index b1ca774..40dd9b8 100644
--- a/libio/libioP.h
+++ b/libio/libioP.h
@@ -890,3 +890,24 @@ _IO_acquire_lock_clear_flags2_fct (_IO_FILE **p)
                                           | _IO_FLAGS2_SCANF_STD);	      \
   } while (0)
 #endif
+
+/* Returns a*b if the result doesn't overflow, else SIZE_MAX.  */
+static inline size_t
+__attribute__ ((__always_inline__))
+_IO_saturating_umull (size_t a, size_t b)
+{
+#if __GNUC_PREREQ(5, 0)
+  size_t result;
+
+  if (__builtin_umull_overflow (a, b, &result)) {
+    return SIZE_MAX;
+  }
+  return result;
+#else
+  const size_t mul_no_overflow = (size_t) 1 << 4 * sizeof (size_t);
+  if ((a >= mul_no_overflow || b >= mul_no_overflow)
+      && b > 1 && a > SIZE_MAX / b)
+    return SIZE_MAX;
+  return a * b;
+#endif
+}