Message ID | CALoOobOpSFwNOqD2RbsSQ95+16=xWN=fTpDJZqgPGJPSXCDmEA@mail.gmail.com |
---|---|
State | New |
Headers | show |
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
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);