Patchwork Fix BZ 19165 -- overflow in fread / fwrite

login
register
mail settings
Submitter Paul Pluzhnikov
Date Oct. 26, 2015, 3:49 a.m.
Message ID <CALoOobOpSFwNOqD2RbsSQ95+16=xWN=fTpDJZqgPGJPSXCDmEA@mail.gmail.com>
Download mbox | patch
Permalink /patch/9374/
State Changes Requested
Headers show

Comments

Paul Pluzhnikov - Oct. 26, 2015, 3:49 a.m.
Greetings,

Attached patch fixes BZ 19165 by failing fwrite when the byte count is
impossibly large, and by returning actual count from fread, instead of
approximation of it. Tested on Linux/x86_64, no new failures.


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

        [BZ #19165]
        * libio/iofread.c (_IO_fread): Return correct count.
        * ibio/iofread_u.c (__fread_unlocked): Likewise.
        * libio/iofwrite.c (_IO_fwrite): Error on overflow.
        * libio/iofwrite_u.c (fwrite_unlocked): Likewise.
Siddhesh Poyarekar - Oct. 26, 2015, 6:50 a.m.
On Monday 26 October 2015 09:19 AM, Paul Pluzhnikov wrote:
> Attached patch fixes BZ 19165 by failing fwrite when the byte count is
> impossibly large, and by returning actual count from fread, instead of
> approximation of it. Tested on Linux/x86_64, no new failures.
> 
> 
> 2015-10-25  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
>         [BZ #19165]
>         * libio/iofread.c (_IO_fread): Return correct count.
>         * ibio/iofread_u.c (__fread_unlocked): Likewise.
>         * libio/iofwrite.c (_IO_fwrite): Error on overflow.
>         * libio/iofwrite_u.c (fwrite_unlocked): Likewise.

Looks OK to me.

Siddhesh
Florian Weimer - Oct. 26, 2015, 7:59 a.m.
On 10/26/2015 04:49 AM, Paul Pluzhnikov wrote:

> diff --git a/libio/iofread.c b/libio/iofread.c
> index eb69b05..a8ea391 100644
> --- a/libio/iofread.c
> +++ b/libio/iofread.c
> @@ -37,7 +37,7 @@ _IO_fread (void *buf, _IO_size_t size, _IO_size_t count, _IO_FILE *fp)
>    _IO_acquire_lock (fp);
>    bytes_read = _IO_sgetn (fp, (char *) buf, bytes_requested);
>    _IO_release_lock (fp);
> -  return bytes_requested == bytes_read ? count : bytes_read / size;
> +  return bytes_read / size;
>  }
>  libc_hidden_def (_IO_fread)

I think this needs a comment why it is acceptable not to check for
overflow here.

> +  if (count > SIZE_MAX / size)
> +    {
> +      __set_errno(EOVERFLOW);
> +      return 0;
> +    }

Can you avoid the division?  Maybe it makes sense to add a separate
abstraction for this (a saturated multiplication function).  It could
use the built-in function with GCC 5.

Florian
Andreas Schwab - Oct. 26, 2015, 9:09 a.m.
Paul Pluzhnikov <ppluzhnikov@google.com> writes:

> diff --git a/libio/iofread.c b/libio/iofread.c
> index eb69b05..a8ea391 100644
> --- a/libio/iofread.c
> +++ b/libio/iofread.c
> @@ -37,7 +37,7 @@ _IO_fread (void *buf, _IO_size_t size, _IO_size_t count, _IO_FILE *fp)
>    _IO_acquire_lock (fp);
>    bytes_read = _IO_sgetn (fp, (char *) buf, bytes_requested);
>    _IO_release_lock (fp);
> -  return bytes_requested == bytes_read ? count : bytes_read / size;
> +  return bytes_read / size;
>  }
>  libc_hidden_def (_IO_fread)
>  

Please add a comment why we ignore overflow here.

Andreas.
Joseph Myers - Oct. 26, 2015, 1:33 p.m.
This patch is missing spaces before '(' in calls to __set_errno.
Paul Pluzhnikov - Oct. 26, 2015, 3:59 p.m.
On Mon, Oct 26, 2015 at 12:59 AM, Florian Weimer <fweimer@redhat.com> wrote:

> > +  if (count > SIZE_MAX / size)
> > +    {
> > +      __set_errno(EOVERFLOW);
> > +      return 0;
> > +    }
>
> Can you avoid the division?  Maybe it makes sense to add a separate
> abstraction for this (a saturated multiplication function).

This https://sourceware.org/bugzilla/show_bug.cgi?id=19165#c4 is how
OpenBSD avoids the division in common case.

Do we want something like:

inline int
mul_would_overflow (size_t a, size_t b)
{
  // sqrt (SIZE_MAX + 1)
  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 1;

  return 0;
}

> It could use the built-in function with GCC 5.

What's the builtin?

Thanks,
Florian Weimer - Oct. 26, 2015, 4:05 p.m.
On 10/26/2015 04:59 PM, Paul Pluzhnikov wrote:

> inline int
> mul_would_overflow (size_t a, size_t b)
> {
>   // sqrt (SIZE_MAX + 1)
>   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 1;
> 
>   return 0;
> }

I think saturating multiplication would be the more useful abstraction:
return the product if it is exact, or (size_t)-1 if it overflows.

>> It could use the built-in function with GCC 5.

The variants available are documented here:

<https://gcc.gnu.org/onlinedocs/gcc-5.2.0/gcc/Integer-Overflow-Builtins.html>

Florian
Alexander Cherepanov - Oct. 26, 2015, 5:50 p.m.
On 2015-10-26 06:49, Paul Pluzhnikov wrote:
> Attached patch fixes BZ 19165 by failing fwrite when the byte count is
> impossibly large,

I think this goes against the standard. In such cases fwrite should go 
until an error. A saturated multiplication, as proposed by Florian, is 
probably a good idea.

> and by returning actual count from fread, instead of
> approximation of it.

Thanks, this should eliminate the main concern (please note though that, 
after wrapping, bytes_requested and hence bytes_read may be not evenly 
divisible by size). But I think this is not enough to be standard-compliant.
Rich Felker - Oct. 26, 2015, 8:06 p.m.
On Sun, Oct 25, 2015 at 08:49:30PM -0700, Paul Pluzhnikov wrote:
> Greetings,
> 
> Attached patch fixes BZ 19165 by failing fwrite when the byte count is
> impossibly large, and by returning actual count from fread, instead of
> approximation of it. Tested on Linux/x86_64, no new failures.
> 
> 
> 2015-10-25  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
>         [BZ #19165]
>         * libio/iofread.c (_IO_fread): Return correct count.
>         * ibio/iofread_u.c (__fread_unlocked): Likewise.
>         * libio/iofwrite.c (_IO_fwrite): Error on overflow.
>         * libio/iofwrite_u.c (fwrite_unlocked): Likewise.
> 
> -- 
> Paul Pluzhnikov

> diff --git a/libio/iofread.c b/libio/iofread.c
> index eb69b05..a8ea391 100644
> --- a/libio/iofread.c
> +++ b/libio/iofread.c
> @@ -37,7 +37,7 @@ _IO_fread (void *buf, _IO_size_t size, _IO_size_t count, _IO_FILE *fp)
>    _IO_acquire_lock (fp);
>    bytes_read = _IO_sgetn (fp, (char *) buf, bytes_requested);
>    _IO_release_lock (fp);
> -  return bytes_requested == bytes_read ? count : bytes_read / size;
> +  return bytes_read / size;

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.

Rich

Patch

diff --git a/libio/iofread.c b/libio/iofread.c
index eb69b05..a8ea391 100644
--- a/libio/iofread.c
+++ b/libio/iofread.c
@@ -37,7 +37,7 @@  _IO_fread (void *buf, _IO_size_t size, _IO_size_t count, _IO_FILE *fp)
   _IO_acquire_lock (fp);
   bytes_read = _IO_sgetn (fp, (char *) buf, bytes_requested);
   _IO_release_lock (fp);
-  return bytes_requested == bytes_read ? count : bytes_read / size;
+  return bytes_read / size;
 }
 libc_hidden_def (_IO_fread)
 
diff --git a/libio/iofread_u.c b/libio/iofread_u.c
index 997b714..28651bf 100644
--- a/libio/iofread_u.c
+++ b/libio/iofread_u.c
@@ -38,7 +38,7 @@  __fread_unlocked (void *buf, _IO_size_t size, _IO_size_t count, _IO_FILE *fp)
   if (bytes_requested == 0)
     return 0;
   bytes_read = _IO_sgetn (fp, (char *) buf, bytes_requested);
-  return bytes_requested == bytes_read ? count : bytes_read / size;
+  return bytes_read / size;
 }
 libc_hidden_def (__fread_unlocked)
 weak_alias (__fread_unlocked, fread_unlocked)
diff --git a/libio/iofwrite.c b/libio/iofwrite.c
index 48ad4bc..4f6c29c 100644
--- a/libio/iofwrite.c
+++ b/libio/iofwrite.c
@@ -34,6 +34,11 @@  _IO_fwrite (const void *buf, _IO_size_t size, _IO_size_t count, _IO_FILE *fp)
   CHECK_FILE (fp, 0);
   if (request == 0)
     return 0;
+  if (count > SIZE_MAX / size)
+    {
+      __set_errno(EOVERFLOW);
+      return 0;
+    }
   _IO_acquire_lock (fp);
   if (_IO_vtable_offset (fp) != 0 || _IO_fwide (fp, -1) == -1)
     written = _IO_sputn (fp, (const char *) buf, request);
diff --git a/libio/iofwrite_u.c b/libio/iofwrite_u.c
index 2b1c47a..f818aec 100644
--- a/libio/iofwrite_u.c
+++ b/libio/iofwrite_u.c
@@ -38,6 +38,11 @@  fwrite_unlocked (const void *buf, _IO_size_t size, _IO_size_t count,
   CHECK_FILE (fp, 0);
   if (request == 0)
     return 0;
+  if (count > SIZE_MAX / size)
+    {
+      __set_errno(EOVERFLOW);
+      return 0;
+    }
   if (_IO_fwide (fp, -1) == -1)
     {
       written = _IO_sputn (fp, (const char *) buf, request);