Re: write past end of buffer in vasnprintf() implementation of %f

2018-10-02 Thread Bruno Haible 
> +  for (size = 0; size <= 8; size++)
> +{
> +  size_t length;
> +  char *result;
> +
> +  memcpy (buf, "DEADBEEF", 8);
> +  length = size;
> +  result = my_asnprintf (buf, , "%2.0f", 1.6314159265358979e+125);
> +  ASSERT (result != NULL);
> +  ASSERT (strcmp (result, 
> "16314159265358979021572935093952849305752959889973415177246818626842325068536614838678161083520756952076273094236944990208")
>  == 0);

This new test case is a little bit too strict: it fails on Cygwin. This
patch fixes it.


2018-10-02  Bruno Haible  

vasnprintf tests: Avoid test failure on Cygwin.
* tests/test-vasnprintf.c (test_function): Change the test added on
2018-09-23 to check only the 42 most significant digits.

diff --git a/tests/test-vasnprintf.c b/tests/test-vasnprintf.c
index 93d81d7..0cd17f9 100644
--- a/tests/test-vasnprintf.c
+++ b/tests/test-vasnprintf.c
@@ -70,7 +70,13 @@ test_function (char * (*my_asnprintf) (char *, size_t *, 
const char *, ...))
   length = size;
   result = my_asnprintf (buf, , "%2.0f", 1.6314159265358979e+125);
   ASSERT (result != NULL);
-  ASSERT (strcmp (result, 
"16314159265358979021572935093952849305752959889973415177246818626842325068536614838678161083520756952076273094236944990208")
 == 0);
+  /* The exact result and the result on glibc systems is
+ 
16314159265358979021572935093952849305752959889973415177246818626842325068536614838678161083520756952076273094236944990208
+ On Cygwin, the result is
+ 
1631415926535897902157293509395284930575296000
+   */
+  ASSERT (strlen (result) == 126);
+  ASSERT (memcmp (result, "163141592653589790215729350939528493057529", 
42) == 0);
   ASSERT (length == 126);
   if (size < 126 + 1)
 ASSERT (result != buf);

Re: write past end of buffer in vasnprintf() implementation of %f

2018-09-23 Thread Ben Pfaff 
On Sun, Sep 23, 2018 at 02:25:50PM +0200, Bruno Haible wrote:
> > The line in convert_to_decimal() cited above is the assignment here:
> > 
> >   /* Terminate the string.  */
> >   *d_ptr = '\0';
> > 
> > I guess that the space calculation passed to malloc() at the top of the
> > same function is not precise.  I don't know whether the right thing to
> > do is to just add one.
> 
> Indeed, the right thing is to add just 1.
> 
> > This bug was originally reported against GNU PSPP:
> > https://savannah.gnu.org/bugs/?func=detailitem_id=54686
> > 
> > For this report, I've simplified it to remove the PSPP dependency (and
> > to make sure it isn't somehow a PSPP bug).
> 
> I found a smaller test case: 1.6314159265358979e+125 instead of
> 1.24726002000241678234e+269, and added that to the test suite.
> For the record, the issue occurs for all numbers in the ranges
>   10^125 <= arg < 2^416
>   10^134 <= arg < 2^448
>   10^260 <= arg < 2^864
>   10^269 <= arg < 2^896
>   ...

Thank you very much for the fix!

Re: write past end of buffer in vasnprintf() implementation of %f

2018-09-23 Thread Bruno Haible 
Ben Pfaff wrote:
> CC='gcc -fsanitize=address -g -O0' ./gnulib-tool --test vasnprintf 
> vasnprintf-posix

A couple of notes about this report:

1) The -O0 in the above command is ineffective. Reason: CFLAGS is '-O2 -g'
   by default, thus when a file gets compiled by $CC $CFLAGS, the -O2 always
   overrides the -O0.

   You need to write
 CC='gcc -fsanitize=address' CFLAGS='-g -O0'
   if you really want the -O0 to have an effect.

2) CC='gcc -fsanitize=address' modifies the result of the
   "checking whether printf supports the 'n' directive" test.

   Namely, the test program crashes with error
   "*** %n in writable segment detected ***".

3) CC='gcc -fsanitize=address' modifies also the result of the
   "checking whether printf survives out-of-memory conditions" test.

   Namely, when a large malloc() fails, the AddressSanitizer, instead
   of letting malloc() return NULL with errno set to ENOMEM, simply
   terminates the program with exit code 1. Gnulib's test in printf.m4 cannot
   distinguish a buggy malloc() from a buggy printf(); it notices that
   the program exits and concludes that printf is buggy.

   This failure has the effect of defining NEED_PRINTF_ENOMEM,
   NEED_PRINTF_DOUBLE, NEED_PRINTF_LONG_DOUBLE in config.h. They are usually
   not defined on glibc systems.

Bruno

Re: write past end of buffer in vasnprintf() implementation of %f

2018-09-23 Thread Bruno Haible 
Hi Ben,

> When I apply the following patch to gnulib:
> 
> --
> diff --git a/tests/test-vasnprintf.c b/tests/test-vasnprintf.c
> index 19731bc93378..105ac24c94a3 100644
> --- a/tests/test-vasnprintf.c
> +++ b/tests/test-vasnprintf.c
> @@ -59,6 +59,11 @@ test_function (char * (*my_asnprintf) (char *, size_t *, 
> const char *, ...))
>if (result != buf)
>  free (result);
>  }
> +
> +  size_t length = 8;
> +  char *result = my_asnprintf (buf, , "%2.0f", 
> 0x1.e38417c792296p+893);
> +  if (result != buf)
> +free (result);
>  }
>  
>  static char *
> --
> 
> and then run:
> 
> CC='gcc -fsanitize=address -g -O0' ./gnulib-tool --test vasnprintf 
> vasnprintf-posix
> 
> I get the following failure from test-vasnprintf:
> 
> --
> ==17880==ERROR: AddressSanitizer: heap-buffer-overflow on address 0xf4b03ece 
> at pc 0xf770f8ed bp 0xffac9638 sp 0xffac962c
> WRITE of size 1 at 0xf4b03ece thread T0
> #0 0xf770f8ec in convert_to_decimal ../../gllib/vasnprintf.c:899
> #1 0xf770f8ec in scale10_round_decimal_decoded 
> ../../gllib/vasnprintf.c:1292
> #2 0xf771057c in scale10_round_decimal_double 
> ../../gllib/vasnprintf.c:1328
> #3 0xf771755c in vasnprintf ../../gllib/vasnprintf.c:4119

An excellent bug report! Thank you.

> The line in convert_to_decimal() cited above is the assignment here:
> 
>   /* Terminate the string.  */
>   *d_ptr = '\0';
> 
> I guess that the space calculation passed to malloc() at the top of the
> same function is not precise.  I don't know whether the right thing to
> do is to just add one.

Indeed, the right thing is to add just 1.

> This bug was originally reported against GNU PSPP:
> https://savannah.gnu.org/bugs/?func=detailitem_id=54686
> 
> For this report, I've simplified it to remove the PSPP dependency (and
> to make sure it isn't somehow a PSPP bug).

I found a smaller test case: 1.6314159265358979e+125 instead of
1.24726002000241678234e+269, and added that to the test suite.
For the record, the issue occurs for all numbers in the ranges
  10^125 <= arg < 2^416
  10^134 <= arg < 2^448
  10^260 <= arg < 2^864
  10^269 <= arg < 2^896
  ...


2018-09-23  Bruno Haible  

vasnprintf: Fix heap memory overrun bug.
Reported by Ben Pfaff  in
.
* lib/vasnprintf.c (convert_to_decimal): Allocate one more byte of
memory.
* tests/test-vasnprintf.c (test_function): Add another test.

diff --git a/lib/vasnprintf.c b/lib/vasnprintf.c
index 56ffbe3..30d021b 100644
--- a/lib/vasnprintf.c
+++ b/lib/vasnprintf.c
@@ -860,7 +860,9 @@ convert_to_decimal (mpn_t a, size_t extra_zeroes)
   size_t a_len = a.nlimbs;
   /* 0.03345 is slightly larger than log(2)/(9*log(10)).  */
   size_t c_len = 9 * ((size_t)(a_len * (GMP_LIMB_BITS * 0.03345f)) + 1);
-  char *c_ptr = (char *) malloc (xsum (c_len, extra_zeroes));
+  /* We need extra_zeroes bytes for zeroes, followed by c_len bytes for the
+ digits of a, followed by 1 byte for the terminating NUL.  */
+  char *c_ptr = (char *) malloc (xsum (xsum (extra_zeroes, c_len), 1));
   if (c_ptr != NULL)
 {
   char *d_ptr = c_ptr;
diff --git a/tests/test-vasnprintf.c b/tests/test-vasnprintf.c
index 19731bc..93d81d7 100644
--- a/tests/test-vasnprintf.c
+++ b/tests/test-vasnprintf.c
@@ -53,7 +53,26 @@ test_function (char * (*my_asnprintf) (char *, size_t *, 
const char *, ...))
   ASSERT (result != NULL);
   ASSERT (strcmp (result, "12345") == 0);
   ASSERT (length == 5);
-  if (size < 6)
+  if (size < 5 + 1)
+ASSERT (result != buf);
+  ASSERT (memcmp (buf + size, &"DEADBEEF"[size], 8 - size) == 0);
+  if (result != buf)
+free (result);
+}
+
+  /* Note: This test assumes IEEE 754 representation of 'double' floats.  */
+  for (size = 0; size <= 8; size++)
+{
+  size_t length;
+  char *result;
+
+  memcpy (buf, "DEADBEEF", 8);
+  length = size;
+  result = my_asnprintf (buf, , "%2.0f", 1.6314159265358979e+125);
+  ASSERT (result != NULL);
+  ASSERT (strcmp (result, 
"16314159265358979021572935093952849305752959889973415177246818626842325068536614838678161083520756952076273094236944990208")
 == 0);
+  ASSERT (length == 126);
+  if (size < 126 + 1)
 ASSERT (result != buf);
   ASSERT (memcmp (buf + size, &"DEADBEEF"[size], 8 - size) == 0);
   if (result != buf)

write past end of buffer in vasnprintf() implementation of %f

2018-09-22 Thread Ben Pfaff 
When I apply the following patch to gnulib:

--
diff --git a/tests/test-vasnprintf.c b/tests/test-vasnprintf.c
index 19731bc93378..105ac24c94a3 100644
--- a/tests/test-vasnprintf.c
+++ b/tests/test-vasnprintf.c
@@ -59,6 +59,11 @@ test_function (char * (*my_asnprintf) (char *, size_t *, 
const char *, ...))
   if (result != buf)
 free (result);
 }
+
+  size_t length = 8;
+  char *result = my_asnprintf (buf, , "%2.0f", 0x1.e38417c792296p+893);
+  if (result != buf)
+free (result);
 }
 
 static char *
--

and then run:

CC='gcc -fsanitize=address -g -O0' ./gnulib-tool --test vasnprintf 
vasnprintf-posix

I get the following failure from test-vasnprintf:

--
==17880==ERROR: AddressSanitizer: heap-buffer-overflow on address 0xf4b03ece at 
pc 0xf770f8ed bp 0xffac9638 sp 0xffac962c
WRITE of size 1 at 0xf4b03ece thread T0
#0 0xf770f8ec in convert_to_decimal ../../gllib/vasnprintf.c:899
#1 0xf770f8ec in scale10_round_decimal_decoded ../../gllib/vasnprintf.c:1292
#2 0xf771057c in scale10_round_decimal_double ../../gllib/vasnprintf.c:1328
#3 0xf771755c in vasnprintf ../../gllib/vasnprintf.c:4119
#4 0xf770c692 in my_asnprintf ../../gltests/test-vasnprintf.c:76
#5 0xf770ca39 in test_function ../../gltests/test-vasnprintf.c:64
#6 0xf770c384 in test_vasnprintf ../../gltests/test-vasnprintf.c:84
#7 0xf770c384 in main ../../gltests/test-vasnprintf.c:96
#8 0xf6f2be80 in __libc_start_main (/lib/i386-linux-gnu/libc.so.6+0x18e80)
#9 0xf770c4de  
(/home/blp/pspp/gnulib2/testdir21347/build/gltests/test-vasnprintf+0x14de)

0xf4b03ece is located 0 bytes to the right of 270-byte region 
[0xf4b03dc0,0xf4b03ece)
allocated by thread T0 here:
#0 0xf71cc334 in malloc (/usr/lib/i386-linux-gnu/libasan.so.4+0xe0334)
#1 0xf770f223 in convert_to_decimal ../../gllib/vasnprintf.c:863
#2 0xf770f223 in scale10_round_decimal_decoded ../../gllib/vasnprintf.c:1292

SUMMARY: AddressSanitizer: heap-buffer-overflow ../../gllib/vasnprintf.c:899 in 
convert_to_decimal
Shadow bytes around the buggy address:
  0x3e960780: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3e960790: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3e9607a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3e9607b0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x3e9607c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x3e9607d0: 00 00 00 00 00 00 00 00 00[06]fa fa fa fa fa fa
  0x3e9607e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3e9607f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3e960800: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3e960810: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3e960820: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:   00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:   fa
  Freed heap region:   fd
  Stack left redzone:  f1
  Stack mid redzone:   f2
  Stack right redzone: f3
  Stack after return:  f5
  Stack use after scope:   f8
  Global redzone:  f9
  Global init order:   f6
  Poisoned by user:f7
  Container overflow:  fc
  Array cookie:ac
  Intra object redzone:bb
  ASan internal:   fe
  Left alloca redzone: ca
  Right alloca redzone:cb
==17880==ABORTING
FAIL test-vasnprintf (exit status: 1)
--

The line in convert_to_decimal() cited above is the assignment here:

  /* Terminate the string.  */
  *d_ptr = '\0';

I guess that the space calculation passed to malloc() at the top of the
same function is not precise.  I don't know whether the right thing to
do is to just add one.

This bug was originally reported against GNU PSPP:
https://savannah.gnu.org/bugs/?func=detailitem_id=54686

For this report, I've simplified it to remove the PSPP dependency (and
to make sure it isn't somehow a PSPP bug).

I'd appreciate it if someone more familiar with this part of the
vasnprintf() code, which seems rather opaque to a newcomer, would
investigate this.  Bruno, you're the main committer to this file, so I'm
guessing that's you, hence the CC.

Thanks a lot!