Fix BZ 19165 -- overflow in fread / fwrite

  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.
  

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
  

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
  

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.
  

This patch is missing spaces before '(' in calls to __set_errno.
  

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,
  

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
  

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.
  

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