bug#13827: faulty range check in bytevector accessor

bug#13827: faulty range check in bytevector accessor

[Ian Price]

Branch: master
Commit: 9b977c836bf147d386944c401113aba32776fa68
System: 32 bit x86 Fedora 16

(use-modules (rnrs bytevectors))
(define not-32-bit (expt 2 32))
(define bv (make-bytevector 4))
(bytevector-u32-set! bv 0 not-32-bit (endianness big))
(pk bv)

Running this gives me a core dump. It happens for a wide range of values
that don't fit in 32 bits.

After some talk on #guile, Mark and I believe it comes down to the range
check in INTEGER_ACCESSOR_PROLOGUE in bytevectors.c

-- 
Ian Price -- shift-reset.com

"Programming is like pinball. The reward for doing it well is
the opportunity to do it again" - from "The Wizardy Compiled"

bug#13827: faulty range check in bytevector accessor

[Mark H Weaver]

Ian Price <ianprice90@googlemail.com> writes:
> After some talk on #guile, Mark and I believe it comes down to the range
> check in INTEGER_ACCESSOR_PROLOGUE in bytevectors.c

Going a bit further: INTEGER_ACCESSOR_PROLOGUE uses 'scm_to_uint', which
I believe should fail for 2^32 on a 32-bit machine.  According to
numbers.h:430, 'scm_to_uint' should be an alias for 'scm_to_uint32',
which is defined in numbers.c:9277 and conv-uinteger.i.c:27.

It seems to me that it ought to be getting to conv-uinteger.i.c:50,
which calls 'mpz_fits_ulong_p'.  So maybe it's a bug in the version of
libgmp on Ian's machine, or perhaps I'm missing something.

I don't know whether it's possible to step through the code in
'conv-uinteger.i.c' using gdb.  If so, I'd like to see what happens.  If
not, I suspect the next step is to write some test programs in C and try
them on Ian's machine: first test 'scm_to_uint32', which should raise an
exception for 2^32.  If it doesn't then try testing 'mpz_fits_ulong_p'
directly and see if it's broken.

    Thanks,
      Mark

bug#13827: faulty range check in bytevector accessor

[Ludovic Courtès]

Ian Price <ianprice90@googlemail.com> skribis:

> Branch: master
> Commit: 9b977c836bf147d386944c401113aba32776fa68
> System: 32 bit x86 Fedora 16
>
> (use-modules (rnrs bytevectors))
> (define not-32-bit (expt 2 32))
> (define bv (make-bytevector 4))
> (bytevector-u32-set! bv 0 not-32-bit (endianness big))
> (pk bv)

FWIW, with 2.0.7+ on x86_64-linux-gnu, with GMP 5.1.0:

--8<---------------cut here---------------start------------->8---
scheme@(guile-user)> (use-modules (rnrs bytevectors))
scheme@(guile-user)> (define not-32-bit (expt 2 32))
scheme@(guile-user)> (define bv (make-bytevector 4))
scheme@(guile-user)> (bytevector-u32-set! bv 0 not-32-bit (endianness big))
<unnamed port>:4:0: In procedure #<procedure 1b1bf00 at <current input>:4:0 ()>:
<unnamed port>:4:0: In procedure bytevector-u32-set!: Value out of range: 4294967296

Entering a new prompt.  Type `,bt' for a backtrace or `,q' to continue.
--8<---------------cut here---------------end--------------->8---

As Mark pointed out, it could be due to the GMP version you’re using.

Could you check that?

TIA,
Ludo’.

bug#13827: faulty range check in bytevector accessor

[Mark H Weaver]

ludo@gnu.org (Ludovic Courtès) writes:

> Ian Price <ianprice90@googlemail.com> skribis:
>
>> Branch: master
>> Commit: 9b977c836bf147d386944c401113aba32776fa68
>> System: 32 bit x86 Fedora 16
>>
>> (use-modules (rnrs bytevectors))
>> (define not-32-bit (expt 2 32))
>> (define bv (make-bytevector 4))
>> (bytevector-u32-set! bv 0 not-32-bit (endianness big))
>> (pk bv)
>
> FWIW, with 2.0.7+ on x86_64-linux-gnu, with GMP 5.1.0:
>
> scheme@(guile-user)> (use-modules (rnrs bytevectors))
> scheme@(guile-user)> (define not-32-bit (expt 2 32))
> scheme@(guile-user)> (define bv (make-bytevector 4))
> scheme@(guile-user)> (bytevector-u32-set! bv 0 not-32-bit (endianness big))
> <unnamed port>:4:0: In procedure #<procedure 1b1bf00 at <current input>:4:0 ()>:
> <unnamed port>:4:0: In procedure bytevector-u32-set!: Value out of range: 4294967296

That's because you're on a 64-bit system, where 2^32 is no difficulty.

     Mark

bug#13827: faulty range check in bytevector accessor

[Ian Price]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=iso-2022-jp-2, Size: 550 bytes --]

ludo@gnu.org (Ludovic Court^[$(D+2^[(Bs) writes:

> As Mark pointed out, it could be due to the GMP version you^[$B!G^[(Bre using.
>
> Could you check that?

Well, I wouldn't rule it out until I've had a proper look. I believe I
am running 4.3.2, but do you have a specific test in mind?

I'm going away for the weekend, though, so I won't really be able to
debug this properly till Monday.

-- 
Ian Price -- shift-reset.com

"Programming is like pinball. The reward for doing it well is
the opportunity to do it again" - from "The Wizardy Compiled"

bug#13827: faulty range check in bytevector accessor

[Andy Wingo]

On Wed 27 Feb 2013 03:02, Ian Price <ianprice90@googlemail.com> writes:

> Branch: master
> Commit: 9b977c836bf147d386944c401113aba32776fa68
> System: 32 bit x86 Fedora 16
>
> (use-modules (rnrs bytevectors))
> (define not-32-bit (expt 2 32))
> (define bv (make-bytevector 4))
> (bytevector-u32-set! bv 0 not-32-bit (endianness big))
> (pk bv)
>
> Running this gives me a core dump. It happens for a wide range of values
> that don't fit in 32 bits.
>
> After some talk on #guile, Mark and I believe it comes down to the range
> check in INTEGER_ACCESSOR_PROLOGUE in bytevectors.c

Something like this look right to you?

--- a/libguile/bytevectors.c
+++ b/libguile/bytevectors.c
@@ -1,4 +1,4 @@
-/* Copyright (C) 2009, 2010, 2011, 2012 Free Software Foundation, Inc.
+/* Copyright (C) 2009, 2010, 2011, 2012, 2013 Free Software Foundation, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public License
@@ -82,12 +82,12 @@
   _sign char *c_bv;						\
 								\
   SCM_VALIDATE_BYTEVECTOR (1, bv);				\
-  c_index = scm_to_uint (index);				\
+  c_index = scm_to_size_t (index);				\
 								\
   c_len = SCM_BYTEVECTOR_LENGTH (bv);				\
   c_bv = (_sign char *) SCM_BYTEVECTOR_CONTENTS (bv);		\
 								\
-  if (SCM_UNLIKELY (c_index + ((_len) >> 3UL) - 1 >= c_len))	\
+  if (SCM_UNLIKELY (c_index >= c_len))                          \
     scm_out_of_range (FUNC_NAME, index);
 
 /* Template for fixed-size integer access (only 8, 16 or 32-bit).  */



-- 
http://wingolog.org/

bug#13827: faulty range check in bytevector accessor

[Andy Wingo]

On Wed 13 Mar 2013 13:55, Andy Wingo <wingo@pobox.com> writes:

> -  if (SCM_UNLIKELY (c_index + ((_len) >> 3UL) - 1 >= c_len))	\
> +  if (SCM_UNLIKELY (c_index >= c_len))                          \
>      scm_out_of_range (FUNC_NAME, index);

I see the intention was to take into account the size of the access
(e.g. 32 bits).  Confusing with len, _len, and c_len...

-- 
http://wingolog.org/

bug#13827: faulty range check in bytevector accessor

[Ben Rocer]

[resubmitting to bug-guile@gnu.org as debbugs seems to have eaten my
 first mail]

When I tried to reproduce this bug on a 32-bit x86 system, I got an
abort in the function bytevector_large_set(); I think this is also
where the bug is.

Specifically, there are two bugs in these two consecutive lines in
bytevector_large_set():

value_size = (mpz_sizeinbase (c_mpz, 2) + (8 * c_size)) / (8 * c_size);
if (SCM_UNLIKELY (value_size > c_size))

In the first line, there is an off-by-one error in the calculation of
value_size; it gives the wrong answer if mpz_sizeinbase() is a
multiple of (8 * c_size) (see
https://gmplib.org/manual/Integer-Import-and-Export.html).

Secondly, this calculation gives the number of (c_size-byte) *words*
required to hold c_mpz, not the number of bytes. So the check in the
next line should be (c_size * value_size > c_size), or equivalently
(value_size > 1).

Since bytevector-u64-set! also calls bytevector_large_set, it
may be possible to reproduce this bug on 64 bit systems too; e.g
(bytevector-u64-set! (make-bytevector 8) 0 (expt 2 64) (endianness big))
[untested]


--- a/libguile/bytevectors.c
+++ b/libguile/bytevectors.c
@@ -867,10 +867,10 @@ bytevector_large_set (char *c_bv, size_t c_size, int signed_p,
     memset (c_bv, 0, c_size);
   else
     {
-      size_t word_count, value_size;
+      size_t word_count, value_words;
 
-      value_size = (mpz_sizeinbase (c_mpz, 2) + (8 * c_size)) / (8 * c_size);
-      if (SCM_UNLIKELY (value_size > c_size))
+      value_words = (mpz_sizeinbase (c_mpz, 2) + (8 * c_size) - 1) / (8 * c_size);
+      if (SCM_UNLIKELY (value_words > 1))
 	{
 	  err = -2;
 	  goto finish;

bug#13827: faulty range check in bytevector accessor

[Andy Wingo]

Hi!

Thank you very much for the bug report and fix!  Applied to master, will
be part of 2.1.4.

Cheers,

Andy

On Mon 28 Jul 2014 16:35, "Ben Rocer" <fleabyte@mail.com> writes:

> [resubmitting to bug-guile@gnu.org as debbugs seems to have eaten my
>  first mail]
>
> When I tried to reproduce this bug on a 32-bit x86 system, I got an
> abort in the function bytevector_large_set(); I think this is also
> where the bug is.
>
> Specifically, there are two bugs in these two consecutive lines in
> bytevector_large_set():
>
> value_size = (mpz_sizeinbase (c_mpz, 2) + (8 * c_size)) / (8 * c_size);
> if (SCM_UNLIKELY (value_size > c_size))
>
> In the first line, there is an off-by-one error in the calculation of
> value_size; it gives the wrong answer if mpz_sizeinbase() is a
> multiple of (8 * c_size) (see
> https://gmplib.org/manual/Integer-Import-and-Export.html).
>
> Secondly, this calculation gives the number of (c_size-byte) *words*
> required to hold c_mpz, not the number of bytes. So the check in the
> next line should be (c_size * value_size > c_size), or equivalently
> (value_size > 1).
>
> Since bytevector-u64-set! also calls bytevector_large_set, it
> may be possible to reproduce this bug on 64 bit systems too; e.g
> (bytevector-u64-set! (make-bytevector 8) 0 (expt 2 64) (endianness big))
> [untested]
>
>
> --- a/libguile/bytevectors.c
> +++ b/libguile/bytevectors.c
> @@ -867,10 +867,10 @@ bytevector_large_set (char *c_bv, size_t c_size, int signed_p,
>      memset (c_bv, 0, c_size);
>    else
>      {
> -      size_t word_count, value_size;
> +      size_t word_count, value_words;
>  
> -      value_size = (mpz_sizeinbase (c_mpz, 2) + (8 * c_size)) / (8 * c_size);
> -      if (SCM_UNLIKELY (value_size > c_size))
> +      value_words = (mpz_sizeinbase (c_mpz, 2) + (8 * c_size) - 1) / (8 * c_size);
> +      if (SCM_UNLIKELY (value_words > 1))
>  	{
>  	  err = -2;
>  	  goto finish;