Fix BZ 19165 -- overflow in fread / fwrite
Commit Message
Ok, let's try one more time.
Previous attempt: https://sourceware.org/ml/libc-alpha/2015-10/msg00894.html
Tested on Linux/x86_64 with 6.0.0 (r222386).
Testing with (gcc-4.8 Ubuntu 4.8.4-2ubuntu1~14.04) 4:4.8.2-1ubuntu6
fails for apparently unrelated reasons:
build/math/test-double-vlen2-wrappers.o: In function `cos_vlen2':
/glibc-git/math/../sysdeps/x86_64/fpu/test-double-vlen2-wrappers.c:24:
undefined reference to `_ZGVbN2v_cos'
/glibc-git/build-system-gcc/math/test-double-vlen2-wrappers.o: In
function `sin_vlen2':
/glibc-git/math/../sysdeps/x86_64/fpu/test-double-vlen2-wrappers.c:25:
undefined reference to `_ZGVbN2v_sin'
... etc.
Thanks,
2015-12-07 Paul Pluzhnikov <ppluzhnikov@google.com>
[BZ #19165]
* misc/sys/cdefs.h (__umul_size_t_overflow): New.
(__umul_size_t_saturated): New.
* libio/iofread.c (_IO_fread): Protect against overflow.
* libio/iofread_u.c (__fread_unlocked): Likewise.
* b/libio/iofwrite.c (_IO_fwrite): Likewise.
* libio/iofwrite_u.c (fwrite_unlocked): Likewise.
Comments
On Mon, 7 Dec 2015, Paul Pluzhnikov wrote:
> Testing with (gcc-4.8 Ubuntu 4.8.4-2ubuntu1~14.04) 4:4.8.2-1ubuntu6
> fails for apparently unrelated reasons:
>
> build/math/test-double-vlen2-wrappers.o: In function `cos_vlen2':
> /glibc-git/math/../sysdeps/x86_64/fpu/test-double-vlen2-wrappers.c:24:
> undefined reference to `_ZGVbN2v_cos'
> /glibc-git/build-system-gcc/math/test-double-vlen2-wrappers.o: In
> function `sin_vlen2':
> /glibc-git/math/../sysdeps/x86_64/fpu/test-double-vlen2-wrappers.c:25:
> undefined reference to `_ZGVbN2v_sin'
> ... etc.
You should raise that with Andrew Senkevich, including details of whether
libmvec got built, what symbols it exports, the command line used for that
link, etc.
On 12/07/2015 09:21 AM, Paul Pluzhnikov wrote:
> +#include <stddef.h> /* for size_t. */
Won't this pollute the namespace? If not, please add a comment
explaining why not.
> +__always_inline
> +static int
> +__umul_size_t_overflow (size_t a, size_t b)
> +{
> +#if __GNUC_PREREQ (5, 0)
> + size_t result;
> +
> + /* _Static_assert, but without triggering conformance test failures. */
Can this problem, whatever it is, be fixed by altering _Static_assert in
cdefs.h, rather than avoiding use of _Static_assert?
> + __attribute__ ((__unused__))
> + char sizeof_size_t_eq_sizeof_ulong[
> + sizeof (size_t) == sizeof (unsigned long) ? 1 : -1];
> +
> + return __builtin_umull_overflow (a, b, &result);
This should use __builtin_mul_overflow instead, so that you don't need
to worry about checking whether size_t is the same width as unsigned long.
> +#else
> + const size_t half_size_t = (size_t) 1 << 4 * sizeof (size_t);
> + const size_t size_max = (size_t) -1;
The 4 is a mystery constant. Better would be to declare half_size_t =
size_max / 2 + 1.
Generally speaking let's not bother adding 'const' to locals that aren't
later assigned to, as these 'const's are more trouble than they're
worth. (Similarly for 'register' and locals whose addresses are not taken.)
All the locals need to be protected with leading "__", since I assume
this .h file can be included from user code.
Defining otherwise-unused static functions in headers will lead to
problems with picky compilers.
The above comments suggest that this patch is part of a larger
maintenance problem with glibc and integer overflow checking. To
simplify maintenance, I suggest instead that we leave cdefs.h alone, and
instead copy and include gnulib's intprops.h for internal use only
inside glibc, and use intprops.h's macro 'INT_MULTIPLY_OVERFLOW (a, b)'
instead of defining and using '__umul_size_t_overflow (a, b)'. This
would simplify maintenance in the long run, as it already incorporates
all the above comments. Please see:
http://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/intprops.h
On 12/07/2015 06:21 PM, Paul Pluzhnikov wrote:
> diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
> index 99e94cc..4474f09 100644
> --- a/misc/sys/cdefs.h
> +++ b/misc/sys/cdefs.h
I think it's better for now not to put this into an installed header
file. include/stdlib.h comes to my mind as a candidate location.
Then you can use _Static_assert and don't have to worry about including
<stddef.h>.
Florian
On Mon, Dec 7, 2015 at 10:52 AM, Paul Eggert <eggert@cs.ucla.edu> wrote:
> The above comments suggest that this patch is part of a larger maintenance
> problem with glibc and integer overflow checking. To simplify maintenance,
> I suggest instead that we leave cdefs.h alone, and instead copy and include
> gnulib's intprops.h for internal use only inside glibc, and use intprops.h's
> macro 'INT_MULTIPLY_OVERFLOW (a, b)' instead of defining and using
> '__umul_size_t_overflow (a, b)'. This would simplify maintenance in the
> long run, as it already incorporates all the above comments. Please see:
>
> http://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/intprops.h
Sigh. A "simple" overflow check in _IO_fread turned into a multi-month
ordeal ...
While I agree that what you are proposing is probably better, I am not
currently prepared to undertake that change. I'll un-assign BZ19165
from myself instead.
On 12/10/2015 08:43 PM, Paul Pluzhnikov wrote:
> On Mon, Dec 7, 2015 at 10:52 AM, Paul Eggert <eggert@cs.ucla.edu> wrote:
>
>> The above comments suggest that this patch is part of a larger maintenance
>> problem with glibc and integer overflow checking. To simplify maintenance,
>> I suggest instead that we leave cdefs.h alone, and instead copy and include
>> gnulib's intprops.h for internal use only inside glibc, and use intprops.h's
>> macro 'INT_MULTIPLY_OVERFLOW (a, b)' instead of defining and using
>> '__umul_size_t_overflow (a, b)'. This would simplify maintenance in the
>> long run, as it already incorporates all the above comments. Please see:
>>
>> http://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/intprops.h
>
> Sigh. A "simple" overflow check in _IO_fread turned into a multi-month
> ordeal ...
I don't think we want to import intprops.h because it is totally opaque.
> While I agree that what you are proposing is probably better, I am not
> currently prepared to undertake that change. I'll un-assign BZ19165
> from myself instead.
Do you mind if I try to move this forward?
Thanks,
Florian
On Thu, Dec 10, 2015 at 11:49 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 12/10/2015 08:43 PM, Paul Pluzhnikov wrote:
>> I'll un-assign BZ19165 from myself instead.
>
> Do you mind if I try to move this forward?
Not at all (I thought that's exactly what un-assigning myself means:
let someone else take a stab ;-)
Thanks!
On Thu, Dec 10, 2015 at 11:53:04AM -0800, Paul Pluzhnikov wrote:
> On Thu, Dec 10, 2015 at 11:49 AM, Florian Weimer <fweimer@redhat.com> wrote:
> > On 12/10/2015 08:43 PM, Paul Pluzhnikov wrote:
>
> >> I'll un-assign BZ19165 from myself instead.
> >
> > Do you mind if I try to move this forward?
>
> Not at all (I thought that's exactly what un-assigning myself means:
> let someone else take a stab ;-)
I think the problem may be even worse than we all expected. I've been
trying to fix the corresponding issue in musl, and it looks like the
_kernel_ is spuriously failing these reads with EFAULT by pre-checking
the validity of the potential destination address range rather than
only checking if there would actually be data to copy. I haven't yet
dug into the kernel sources to figure out why this is happening but
read(2), readv(2), pread(2), etc. are probably all affected and I'm
skeptical of whether it makes sense to try to work around this in
libc. We should probably seek clarificatin from the Austin Group on
whether those interfaces are intended to have well-defined behavior
when the nbytes argument is greater than the size of the buffer. For
fread it's WG14's domain and getting a good answer from them on
whether invalid size yields UB is probably going to be difficult...
Rich
On 02/11/2016 03:26 AM, Rich Felker wrote:
> I think the problem may be even worse than we all expected. I've been
> trying to fix the corresponding issue in musl, and it looks like the
> _kernel_ is spuriously failing these reads with EFAULT by pre-checking
> the validity of the potential destination address range rather than
> only checking if there would actually be data to copy.
Yes, system call behavior in this area is fairly regular: if a memory
region is passed, it is checked for validity as a whole, and not just
for the parts that are actually needed. By now, this is part of the
user space interface, and probably cannot change without breaking
backwards compatibility.
Florian
(sorry for the malformatted message earlier)
On Thu, Feb 11, 2016 at 7:22 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 02/11/2016 03:26 AM, Rich Felker wrote:
> > I think the problem may be even worse than we all expected. I've been
> > trying to fix the corresponding issue in musl, and it looks like the
> > _kernel_ is spuriously failing these reads with EFAULT by pre-checking
> > the validity of the potential destination address range rather than
> > only checking if there would actually be data to copy.
>
> Yes, system call behavior in this area is fairly regular: if a memory
> region is passed, it is checked for validity as a whole, and not just
> for the parts that are actually needed. By now, this is part of the
> user space interface, and probably cannot change without breaking
> backwards compatibility.
Backward compatibility doesn't seem like a strong argument to me --
changing this would only make programs that don't work now, start
working. It would be much more problematic the other way around.
However, it seems to me there's a serious implementation-level
obstacle: for at least some device I/O, the kernel may need to
finalize memory access checks before it knows how much data is
available. And it would be very confusing if the behavior depended on
what type of fd you were using.
zw
On Thu, Feb 11, 2016 at 08:50:58AM -0500, Zack Weinberg wrote:
> On Feb 11, 2016 7:22 AM, "Florian Weimer" <fweimer@redhat.com> wrote:
> >
> > On 02/11/2016 03:26 AM, Rich Felker wrote:
> > > I think the problem may be even worse than we all expected. I've been
> > > trying to fix the corresponding issue in musl, and it looks like the
> > > _kernel_ is spuriously failing these reads with EFAULT by pre-checking
> > > the validity of the potential destination address range rather than
> > > only checking if there would actually be data to copy.
> >
> > Yes, system call behavior in this area is fairly regular: if a memory
> > region is passed, it is checked for validity as a whole, and not just
> > for the parts that are actually needed. By now, this is part of the
> > user space interface, and probably cannot change without breaking
> > backwards compatibility.
>
> Also, the kernel might need to finalize access checks and wire down the
> pages for DMA before it even knows how much data is available.
That makes no sense except for O_DIRECT wackiness where I doubt anyone
cares about correctness/standards. Normal file reads should always be
DMA into the fs cache followed by memcpy to the caller's buffer.
Rich
On Thu, Feb 11, 2016 at 10:30 AM, Rich Felker <dalias@libc.org> wrote:
> On Thu, Feb 11, 2016 at 08:50:58AM -0500, Zack Weinberg wrote:
>>
>> [...] the kernel might need to finalize access checks and wire down the
>> pages for DMA before it even knows how much data is available.
>
> That makes no sense except for O_DIRECT wackiness where I doubt anyone
> cares about correctness/standards. Normal file reads should always be
> DMA into the fs cache followed by memcpy to the caller's buffer.
I was thinking of character devices and maybe also sockets. Those don't
go through the fs cache.
And I don't think anyone wants the behavior to vary depending on which
kind of fd you have passed to read().
zw
On 02/11/2016 07:58 AM, Zack Weinberg wrote:
> I don't think anyone wants the behavior to vary depending on which
> kind of fd you have passed to read().
As an application developer I'd rather have 'read', 'fread', etc. fail,
or even dump core, if I give them a buffer that is not entirely valid. I
hope, for example, that checkers like valgrind object to this sort of
thing, and would find it useful if glibc continues to report it as an
error. If POSIX really requires that 'read' and 'fread' must succeed in
this situation, then from my point of view it's a POSIX bug that needs
to get fixed, as the benefit of the extra checking is significant.
@@ -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 = __umul_size_t_saturated (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 = __umul_size_t_saturated (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 = __umul_size_t_saturated (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 = __umul_size_t_saturated (size, count);
_IO_size_t written = 0;
CHECK_FILE (fp, 0);
if (request == 0)
@@ -441,4 +441,37 @@
# endif
#endif
+#include <stddef.h> /* for size_t. */
+
+__always_inline
+static int
+__umul_size_t_overflow (size_t a, size_t b)
+{
+#if __GNUC_PREREQ (5, 0)
+ size_t result;
+
+ /* _Static_assert, but without triggering conformance test failures. */
+ __attribute__ ((__unused__))
+ char sizeof_size_t_eq_sizeof_ulong[
+ sizeof (size_t) == sizeof (unsigned long) ? 1 : -1];
+
+ return __builtin_umull_overflow (a, b, &result);
+#else
+ const size_t half_size_t = (size_t) 1 << 4 * sizeof (size_t);
+ const size_t size_max = (size_t) -1;
+
+ return __glibc_unlikely ((a | b) >= half_size_t) && b > 1 && a > size_max / b;
+#endif
+}
+
+__always_inline
+static size_t
+__umul_size_t_saturated (size_t a, size_t b)
+{
+ const size_t size_max = (size_t) -1;
+
+ if (__umul_size_t_overflow (a, b)) return size_max;
+ return a * b;
+}
+
#endif /* sys/cdefs.h */