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

Message ID CALoOobOpSFwNOqD2RbsSQ95+16=xWN=fTpDJZqgPGJPSXCDmEA@mail.gmail.com
State New
Headers show

Commit Message

Paul Pluzhnikov Oct. 26, 2015, 3:49 a.m. UTC
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.

Comments

Siddhesh Poyarekar Oct. 26, 2015, 6:50 a.m. UTC | #1
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. UTC | #2
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. UTC | #3
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. UTC | #4
This patch is missing spaces before '(' in calls to __set_errno.
Paul Pluzhnikov Oct. 26, 2015, 3:59 p.m. UTC | #5
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. UTC | #6
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. UTC | #7
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. UTC | #8
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 mbox

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);