Re: major formatted output function bug with %c and the value 0

[Vincent Lefevre]

On 2024-03-25 18:13:22 +0100, marco.bodr...@tutanota.com wrote:
> Vincent, you did not confirm that this code worked for you, but I
> pushed it anyway.
> https://gmplib.org/repo/gmp/rev/4ac76064639e

Thanks. I'll look at it when I have some time.

-- 
Vincent Lefèvre  - Web: 
100% accessible validated (X)HTML - Blog: 
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: major formatted output function bug with %c and the value 0

[marco . bodrato]

Ciao,

18 feb 2024, 12:29 da marco.bodr...@tutanota.com:

> 15 dic 2023, 13:26 da vinc...@vinc17.net:
>
>> Note that there are similar issues in printf/repl-vsnprintf.c, and I
>>
>
> I finally had the time to examine the code and test it. I attach a proposed 
> patch.
>
Vincent, you did not confirm that this code worked for you, but I pushed it 
anyway.
https://gmplib.org/repo/gmp/rev/4ac76064639e

Thanks.
m
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: major formatted output function bug with %c and the value 0

[marco . bodrato]

Ciao Vincent,

15 dic 2023, 13:26 da vinc...@vinc17.net:

> Note that there are similar issues in printf/repl-vsnprintf.c, and I
>

I finally had the time to examine the code and test it. I attach a proposed 
patch.

I changed the 3 files: printf/doprntf.c, printf/repl-vsnprintf.c, 
printf/sprintffuns.c, to use the returned value of the called functions, 
instead of calling strlen on the string.

I also slightly changed the tests, to check at least one case with "%c", 0.
For this to work I also had to change a
-  (*__gmp_free_func) (got, strlen(got)+1);
+  (*__gmp_free_func) (got, got_len+1);
When the string comes from a generic print, it is safer to relay on the return 
value than on the length computed by strlen()...


> think that this is the cause of the MPFR failure with MS Visual C++
> (vsnprintf is used by gmp_vasprintf).
>
If you can also test on that side, let me know. Thanks!

Gis,
m

diff -r 1c566525476a printf/doprntf.c
--- a/printf/doprntf.c	Wed Dec 27 19:35:36 2023 +0100
+++ b/printf/doprntf.c	Sun Feb 18 12:17:18 2024 +0100
@@ -267,8 +267,7 @@
 	 mean truncation */
   ASSERT (explen >= 0 && explen < sizeof(exponent)-1);
 #else
-  sprintf (exponent, p->expfmt, expsign, expval);
-  explen = strlen (exponent);
+  explen = sprintf (exponent, p->expfmt, expsign, expval);
   ASSERT (explen < sizeof(exponent));
 #endif
   TRACE (printf ("  expfmt %s gives %s\n", p->expfmt, exponent));
diff -r 1c566525476a printf/repl-vsnprintf.c
--- a/printf/repl-vsnprintf.c	Wed Dec 27 19:35:36 2023 +0100
+++ b/printf/repl-vsnprintf.c	Sun Feb 18 12:17:18 2024 +0100
@@ -364,16 +364,14 @@
 
   if (total_width <= buf_size)
 {
-  vsprintf (buf, orig_fmt, orig_ap);
-  len = strlen (buf);
+  len = vsprintf (buf, orig_fmt, orig_ap);
 }
   else
 {
   char  *s;
 
   s = __GMP_ALLOCATE_FUNC_TYPE (total_width, char);
-  vsprintf (s, orig_fmt, orig_ap);
-  len = strlen (s);
+  len = vsprintf (s, orig_fmt, orig_ap);
   if (buf_size != 0)
 	{
 	  size_t  copylen = MIN (len, buf_size-1);
diff -r 1c566525476a printf/sprintffuns.c
--- a/printf/sprintffuns.c	Wed Dec 27 19:35:36 2023 +0100
+++ b/printf/sprintffuns.c	Sun Feb 18 12:17:18 2024 +0100
@@ -53,9 +53,9 @@
 {
   char  *buf = *bufp;
   int   ret;
-  vsprintf (buf, fmt, ap);
-  ret = strlen (buf);
-  *bufp = buf + ret;
+  ret = vsprintf (buf, fmt, ap);
+  if (ret > 0)
+*bufp = buf + ret;
   return ret;
 }
 
diff -r 1c566525476a tests/misc/t-printf.c
--- a/tests/misc/t-printf.c	Wed Dec 27 19:35:36 2023 +0100
+++ b/tests/misc/t-printf.c	Sun Feb 18 12:17:18 2024 +0100
@@ -128,12 +128,11 @@
 }
 
 void
-check_vsprintf (const char *want, const char *fmt, va_list ap)
+check_vsprintf (const char *want, int want_len, const char *fmt, va_list ap)
 {
   char  got[MAX_OUTPUT];
-  int   got_len, want_len;
+  int   got_len;
 
-  want_len = strlen (want);
   got_len = gmp_vsprintf (got, fmt, ap);
 
   if (got_len != want_len || strcmp (got, want) != 0)
@@ -149,14 +148,12 @@
 }
 
 void
-check_vfprintf (const char *want, const char *fmt, va_list ap)
+check_vfprintf (const char *want, int want_len, const char *fmt, va_list ap)
 {
   char  got[MAX_OUTPUT];
-  int   got_len, want_len, fread_len;
+  int   got_len, fread_len;
   long  ftell_len;
 
-  want_len = strlen (want);
-
   rewind (check_vfprintf_fp);
   got_len = gmp_vfprintf (check_vfprintf_fp, fmt, ap);
   ASSERT_ALWAYS (got_len != -1);
@@ -187,14 +184,12 @@
 }
 
 void
-check_vsnprintf (const char *want, const char *fmt, va_list ap)
+check_vsnprintf (const char *want, int want_len, const char *fmt, va_list ap)
 {
   chargot[MAX_OUTPUT+1];
-  int ret, got_len, want_len;
+  int ret, got_len;
   size_t  bufsize;
 
-  want_len = strlen (want);
-
   bufsize = -1;
   for (;;)
 {
@@ -243,12 +238,11 @@
 }
 
 void
-check_vasprintf (const char *want, const char *fmt, va_list ap)
+check_vasprintf (const char *want, int want_len, const char *fmt, va_list ap)
 {
   char  *got;
-  int   got_len, want_len;
+  int   got_len;
 
-  want_len = strlen (want);
   got_len = gmp_vasprintf (, fmt, ap);
 
   if (got_len != want_len || strcmp (got, want) != 0)
@@ -261,19 +255,17 @@
   printf ("  want_len %d\n", want_len);
   abort ();
 }
-  (*__gmp_free_func) (got, strlen(got)+1);
+  (*__gmp_free_func) (got, got_len+1);
 }
 
 void
-check_obstack_vprintf (const char *want, const char *fmt, va_list ap)
+check_obstack_vprintf (const char *want, int want_len, const char *fmt, va_list ap)
 {
 #if HAVE_OBSTACK_VPRINTF
   struct obstack  ob;
-  int   got_len, want_len, ob_len;
+  int   got_len, ob_len;
   char  *got;
 
-  want_len = strlen (want);
-
   obstack_init ();
   got_len = gmp_obstack_vprintf (, fmt, ap);
   got = (char *) obstack_base ();
@@ -300,15 +292,30 @@
 void
 check_one (const char *want, const char *fmt, ...)
 {
+  int want_len = strlen (want);
   va_list ap;
   va_start (ap, fmt);
 
   /* simplest first */
-  check_vsprintf (want, fmt, ap);
-  

Re: major formatted output function bug with %c and the value 0

[Vincent Lefevre]

Hi,

On 2023-12-13 20:03:13 +0100, marco.bodr...@tutanota.com wrote:
> It was changed in 2001, probably a workaround, because the comment was
> "Don't use sprintf return value (it's a pointer on SunOS 4)"
> https://gmplib.org/repo/gmp/rev/0889877bb94a

Note that there are similar issues in printf/repl-vsnprintf.c, and I
think that this is the cause of the MPFR failure with MS Visual C++
(vsnprintf is used by gmp_vasprintf).

This file was added on 26 Oct 2001, thus 2 months after 0889877bb94a.

-- 
Vincent Lefèvre  - Web: 
100% accessible validated (X)HTML - Blog: 
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: major formatted output function bug with %c and the value 0

[marco . bodrato]

Ciao,

13 dic 2023, 15:53 da vinc...@vinc17.net:

> On 2023-12-03 20:19:10 +0100, Vincent Lefevre wrote:
>
>> With GMP 6.3.0, the formatted output functions do not handle %c
>> with the value 0 correctly. For gmp_sprintf, the return value is
>> incorrect.
>>
> In printf/sprintffuns.c, function gmp_sprintf_format(), I suppose that
>
>  vsprintf (buf, fmt, ap);
>  ret = strlen (buf);
>
> should actually be something like
>
>  ret = vsprintf (buf, fmt, ap);
>  if (ret < 0)
>  ret = 0;
>
> to avoid issues due to non-terminating null characters (not tested).
>
It was changed in 2001, probably a workaround, because the comment was
"Don't use sprintf return   value (it's a pointer on SunOS 4)"
https://gmplib.org/repo/gmp/rev/0889877bb94a

Maybe we should simply "revert" that change, and use the return value both from 
sprintf (in printf/doprntf.c) and from vsprintf (in printf/sprintffuns.c)?

Or, if we care not to modify the pointer bufp, we can use something like the 
following:
diff -r f6073853d16a printf/sprintffuns.c
--- a/printf/sprintffuns.c  Mon Oct 16 08:16:06 2023 +0200
+++ b/printf/sprintffuns.c  Wed Dec 13 19:53:50 2023 +0100
@@ -53,9 +53,9 @@
{
   char  *buf = *bufp;
   int   ret;
-  vsprintf (buf, fmt, ap);
-  ret = strlen (buf);
-  *bufp = buf + ret;
+  ret = vsprintf (buf, fmt, ap);
+  if (ret > 0)
+    *bufp = buf + ret;
   return ret;
}
 
It passes the test suite, but I didn't really think about what it does.

Ĝis,
mb
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: major formatted output function bug with %c and the value 0

[Vincent Lefevre]

On 2023-12-03 20:19:10 +0100, Vincent Lefevre wrote:
> With GMP 6.3.0, the formatted output functions do not handle %c
> with the value 0 correctly. For gmp_sprintf, the return value is
> incorrect.

In printf/sprintffuns.c, function gmp_sprintf_format(), I suppose that

  vsprintf (buf, fmt, ap);
  ret = strlen (buf);

should actually be something like

  ret = vsprintf (buf, fmt, ap);
  if (ret < 0)
ret = 0;

to avoid issues due to non-terminating null characters (not tested).

But I'm wondering how an error is possibly detected. The comment is:

/* If vsprintf returns -1 then pass it upwards.  It doesn't matter that
   "*bufp" is ruined in this case, since gmp_doprint will bail out
   immediately anyway.  */

> For gmp_asprintf and gmp_vasprintf, this is either a buffer overflow
> (according to the GMP manual: "The block will be the size of the
> string and null-terminator.") or, in case this is an error in the
> GMP manual, possible memory corruption when freeing the allocated
> memory, if the custom memory allocation function cares about the
> size parameter.

This should be checked and the GMP manual should be fixed.

> 
> Testcase for gmp_sprintf:
> 
> 
> #include 
> #include 
> 
> static void test (int flag)
> {
>   char s[3] = { 1, 1, 1 };
>   int r;
> 
>   r = (flag ? sprintf : gmp_sprintf) (s, "%c", 0);
>   printf ("%4s: r = %d, s = { %d %d %d }\n",
>   flag ? "libc" : "gmp", r, s[0], s[1], s[2]);
> }
> 
> int main (void)
> {
>   test (0);
>   test (1);
>   return 0;
> }
> 
> 
> which currently gives:
> 
>  gmp: r = 0, s = { 0 0 1 }
> libc: r = 1, s = { 0 0 1 }
> 
> MPFR has various issues concerning %c with the value 0, but an
> attempt to fix them fails due to
> 
>   length = gmp_vasprintf (...);
> [...]
>   mpfr_free_str (s);
> 
> which is similar to GMP's tests/misc/t-printf.c file, which contains
> 
>   got_len = gmp_vasprintf (, fmt, ap);
> [...]
>   (*__gmp_free_func) (got, strlen(got)+1);
> 
> But replacing
> 
>   mpfr_free_str (s);
> 
> by
> 
>   mpfr_free_func (s, length + 1);
> 
> i.e. using the return value length instead of strlen(s), also fails.
> I suppose that this is related to the incorrect return value.

Well, the failure was due to another mpfr_free_func, no longer this
one (I've now completely fixed MPFR, I think).

-- 
Vincent Lefèvre  - Web: 
100% accessible validated (X)HTML - Blog: 
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

major formatted output function bug with %c and the value 0

[Vincent Lefevre]

With GMP 6.3.0, the formatted output functions do not handle %c
with the value 0 correctly. For gmp_sprintf, the return value is
incorrect. For gmp_asprintf and gmp_vasprintf, this is either a
buffer overflow (according to the GMP manual: "The block will be
the size of the string and null-terminator.") or, in case this
is an error in the GMP manual, possible memory corruption when
freeing the allocated memory, if the custom memory allocation
function cares about the size parameter.

Testcase for gmp_sprintf:


#include 
#include 

static void test (int flag)
{
  char s[3] = { 1, 1, 1 };
  int r;

  r = (flag ? sprintf : gmp_sprintf) (s, "%c", 0);
  printf ("%4s: r = %d, s = { %d %d %d }\n",
  flag ? "libc" : "gmp", r, s[0], s[1], s[2]);
}

int main (void)
{
  test (0);
  test (1);
  return 0;
}


which currently gives:

 gmp: r = 0, s = { 0 0 1 }
libc: r = 1, s = { 0 0 1 }

MPFR has various issues concerning %c with the value 0, but an
attempt to fix them fails due to

  length = gmp_vasprintf (...);
[...]
  mpfr_free_str (s);

which is similar to GMP's tests/misc/t-printf.c file, which contains

  got_len = gmp_vasprintf (, fmt, ap);
[...]
  (*__gmp_free_func) (got, strlen(got)+1);

But replacing

  mpfr_free_str (s);

by

  mpfr_free_func (s, length + 1);

i.e. using the return value length instead of strlen(s), also fails.
I suppose that this is related to the incorrect return value.

-- 
Vincent Lefèvre  - Web: 
100% accessible validated (X)HTML - Blog: 
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs