Fix BZ 19165 -- overflow in fread / fwrite
diff mbox

Message ID CALoOobPxCPN_Lwvc98CevgCJMwHa_9cURZsALsLeG+SPDSF+Xw@mail.gmail.com
State New
Headers show

Commit Message

Paul Pluzhnikov Oct. 27, 2015, 2:04 a.m. UTC
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.

Comments

Paul Pluzhnikov Oct. 27, 2015, 2:06 a.m. UTC | #1
On Mon, Oct 26, 2015 at 7:04 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:

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

Oops. Let's try that again:

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): Likewise.
        * libio/iofwrite_u.c (fwrite_unlocked): Likewise.
Mike Frysinger Oct. 27, 2015, 4:26 a.m. UTC | #2
On 26 Oct 2015 19:04, Paul Pluzhnikov wrote:
> --- a/libio/libioP.h
> +++ b/libio/libioP.h
> +
> +/* Returns a*b if the result doesn't overflow, else SIZE_MAX.  */
> +static inline size_t
> +__attribute__ ((__always_inline__))

__always_inline

> +_IO_saturating_umull (size_t a, size_t b)
> +{
> +#if __GNUC_PREREQ(5, 0)

needs space before the (

> +  size_t result;
> +
> +  if (__builtin_umull_overflow (a, b, &result)) {
> +    return SIZE_MAX;
> +  }

braces are wrong -- just delete them

> +  return result;

seems like it'd be better:
  return __builtin_umull_overflow (a, b, &result) ? SIZE_MAX : 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)

should we add a __umull_overflow define to misc/sys/cdefs.h ?
then we don't have to duplicate this logic everywhere.
-mike
Paul Pluzhnikov Oct. 27, 2015, 5:11 a.m. UTC | #3
On Mon, Oct 26, 2015 at 9:26 PM, Mike Frysinger <vapier@gentoo.org> wrote:

> seems like it'd be better:
>   return __builtin_umull_overflow (a, b, &result) ? SIZE_MAX : result;

Thanks. I've fixed this in the patch that I will send next.

>
>> +#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)
>
> should we add a __umull_overflow define to misc/sys/cdefs.h ?
> then we don't have to duplicate this logic everywhere.

In malloc/malloc.c the following trick is played to save another compare/branch:

#define HALF_INTERNAL_SIZE_T \
  (((INTERNAL_SIZE_T) 1) << (8 * sizeof (INTERNAL_SIZE_T) / 2))
  if (__builtin_expect ((n | elem_size) >= HALF_INTERNAL_SIZE_T, 0))

To be clear, you are proposing adding to misc/sys/cdefs.h something like:

static inline bool
__attribute__ ((__always_inline))
__umull_overflow (size_t a, size_t b)
{
#if __GNUC_PREREQ(5, 0)
  size_t result;

  return __builtin_umull_overflow (a, b, &result);
#else
  const size_t half_size_t = (size_t) 1 << 4 * sizeof (size_t);
  return __glibc_unlikely ((a|b) >= half_size_t) && b > 1 && a > SIZE_MAX / b;
#endif
}

or an equivalent macro?
(What would be the advantage of macro over inline function?)

Thanks.
Joseph Myers Oct. 27, 2015, 10:51 a.m. UTC | #4
On Mon, 26 Oct 2015, Paul Pluzhnikov wrote:

> static inline bool
> __attribute__ ((__always_inline))
> __umull_overflow (size_t a, size_t b)

I don't approve of function naming that quietly embeds the assumption that 
size_t and unsigned long have the same set of values.

If you want a multiplication function for size_t, whether saturating or 
setting an explicit overflow flag, then the name should make clear that 
it's for size_t.  And if you use __builtin_umull_overflow in the 
implementation, you should also have a static assertion that SIZE_MAX == 
ULONG_MAX.
Paul Pluzhnikov Oct. 27, 2015, 1:59 p.m. UTC | #5
On Tue, Oct 27, 2015 at 3:51 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Mon, 26 Oct 2015, Paul Pluzhnikov wrote:
>
>> static inline bool
>> __attribute__ ((__always_inline))
>> __umull_overflow (size_t a, size_t b)
>
> I don't approve of function naming that quietly embeds the assumption that
> size_t and unsigned long have the same set of values.
>
> If you want a multiplication function for size_t, whether saturating or
> setting an explicit overflow flag, then the name should make clear that
> it's for size_t.

__umul_size_t_overflow ?
__glibc_mul_size_t_overflow ?

Other suggestions?

Thanks.
Alexander Cherepanov Oct. 27, 2015, 6:21 p.m. UTC | #6
On 2015-10-27 05:06, Paul Pluzhnikov wrote:
> On Mon, Oct 26, 2015 at 7:04 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>
>> [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.
>
> Oops. Let's try that again:
>
> 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 here?

>          * libio/iofwrite.c (_IO_fwrite): Likewise.
>          * libio/iofwrite_u.c (fwrite_unlocked): Likewise.

Patch
diff mbox

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