Fix BZ 19165 -- overflow in fread / fwrite

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.

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.

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

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.

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.

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.

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.