Fix BZ 19165 -- overflow in fread / fwrite
Commit Message
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
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.
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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
+}