[PATCH] newlib: Improve memory allocation for floating-point string

Markus Eisenmann <[email protected]>
Newsgroups gmane.comp.lib.newlib
Message-ID <DB9PR06MB903275E78EBE394F69A6F5BDD2302@DB9PR06MB9032.eurprd06.prod.outlook.com> 
Hi!

IMHO, the commit "newlib: Fix memory leak regarding gdtoa-based _ldtoa_r()" [02/08/2023] has solved
a memory leak issue, but I think there's still scenarios that may lead to memory corruption or exception.
E.g., calling freedtoa() after gdto() frees the memory-block but does not invalidate the copy in the mp-result
member in the reent-structure. Call _dtoa_r() or _ldtoa_r() afterwards will "free" this block twice and will
corrupt the chain of free blocks. In addition, I think that the previous implementation in gdto() - like used
in phyton - that the value _Bigint::_k is stored in front of the memory instead in within the _reent-structure
is more robust and is more flexible.

Therefore, I suggest the attached patch to fix a potential memory leak by calling _ldtoa_r() - using gdtoa() -
after freeing a (another) result string by freedtoa() previously. Additionally, I've refactored (Ie., simplified)
the implementation of allocation/freeing the dtoa* string result-buffer over several source files.

Please, review and test my code-changes and may (hopefully) merge this improvements into Newlib.


Best regards from Salzburg,
Markus

P.S.: Don't hesitate to send me a mail if there are questions to clarify or if there's still a bug within the patch.
Newlib-Improve-memory-allocation-for-mpresult.patch (application/octet-stream, 7.2 KB) 
commit f741dae2f2724a66241c573d0bf376144d9bf6b5
Author: Markus Eisenmann <[email protected]>
Date:   Thu Dec 5 12:14:20 2024 +0100

    newlib: Improve memory allocation for floating-point string buffer in dtoa-like functions.

diff --git a/newlib/libc/machine/powerpc/simdldtoa.c b/newlib/libc/machine/powerpc/simdldtoa.c
index 9a90e64e2..409f31217 100644
--- a/newlib/libc/machine/powerpc/simdldtoa.c
+++ b/newlib/libc/machine/powerpc/simdldtoa.c
@@ -2718,10 +2718,7 @@ rnd.rndprc = NBITS;
 /* reentrancy addition to use mprec storage pool */
 if (_REENT_MP_RESULT(ptr))
   {
-    _REENT_MP_RESULT(ptr)->_k = _REENT_MP_RESULT_K(ptr);
-    _REENT_MP_RESULT(ptr)->_maxwds = 1 << _REENT_MP_RESULT_K(ptr);
-    Bfree (ptr, _REENT_MP_RESULT(ptr));
-    _REENT_MP_RESULT(ptr) = 0;
+    _mprslt_free_blck (ptr, _REENT_MP_RESULT(ptr)); /* release and free */
   }
 
 #if SIMD_LDBL_MANT_DIG == 24
@@ -2750,11 +2747,8 @@ if( mode == 0 )
 /* reentrancy addition to use mprec storage pool */
 /* we want to have enough space to hold the formatted result */
 i = ndigits + (mode == 3 ? (MAX_EXP_DIGITS + 1) : 1);
-j = sizeof (__ULong);
-for (_REENT_MP_RESULT_K(ptr) = 0; sizeof (_Bigint) - sizeof (__ULong) + j <= (unsigned)i; j <<= 1)
-  _REENT_MP_RESULT_K(ptr)++;
-_REENT_MP_RESULT(ptr) = Balloc (ptr, _REENT_MP_RESULT_K(ptr));
-outstr = (char *)_REENT_MP_RESULT(ptr);
+(void) j; /*unused*/
+outstr = _mprslt_alloc_str (ptr, i);
 
 /* This sanity limit must agree with the corresponding one in etoasc, to
    keep straight the returned value of outexpon.  */
diff --git a/newlib/libc/stdlib/dtoa.c b/newlib/libc/stdlib/dtoa.c
index 198fa663a..4a2b3dc5d 100644
--- a/newlib/libc/stdlib/dtoa.c
+++ b/newlib/libc/stdlib/dtoa.c
@@ -239,10 +239,7 @@ _dtoa_r (struct _reent *ptr,
   _REENT_CHECK_MP(ptr);
   if (_REENT_MP_RESULT(ptr))
     {
-      _REENT_MP_RESULT(ptr)->_k = _REENT_MP_RESULT_K(ptr);
-      _REENT_MP_RESULT(ptr)->_maxwds = 1 << _REENT_MP_RESULT_K(ptr);
-      Bfree (ptr, _REENT_MP_RESULT(ptr));
-      _REENT_MP_RESULT(ptr) = 0;
+      _mprslt_free_blck (ptr, _REENT_MP_RESULT(ptr)); /* release and free */
     }
 
   if (word0 (d) & Sign_bit)
@@ -424,12 +421,7 @@ _dtoa_r (struct _reent *ptr,
       if (i <= 0)
 	i = 1;
     }
-  j = sizeof (__ULong);
-  for (_REENT_MP_RESULT_K(ptr) = 0; sizeof (_Bigint) - sizeof (__ULong) + j <= i;
-       j <<= 1)
-    _REENT_MP_RESULT_K(ptr)++;
-  _REENT_MP_RESULT(ptr) = eBalloc (ptr, _REENT_MP_RESULT_K(ptr));
-  s = s0 = (char *) _REENT_MP_RESULT(ptr);
+  s = s0 = _mprslt_alloc_str (ptr, i);
 
   if (ilim >= 0 && ilim <= Quick_max && try_quick)
     {
diff --git a/newlib/libc/stdlib/gdtoa-dmisc.c b/newlib/libc/stdlib/gdtoa-dmisc.c
index f330f8ae7..b0adb695c 100644
--- a/newlib/libc/stdlib/gdtoa-dmisc.c
+++ b/newlib/libc/stdlib/gdtoa-dmisc.c
@@ -46,23 +46,8 @@ rv_alloc(ptr, i) struct _reent *ptr, int i;
 rv_alloc(struct _reent *ptr, int i)
 #endif
 {
-	int j;
-	char *r;
-
-	/* Allocate buffer in a compatible way with legacy _ldtoa_r(). */
-	j = sizeof(ULong);
-	for (_REENT_MP_RESULT_K (ptr) = 0;
-	     sizeof (Bigint) - sizeof (ULong) + j <= i; j <<= 1)
-		_REENT_MP_RESULT_K (ptr)++;
-	_REENT_MP_RESULT (ptr) = eBalloc (ptr, _REENT_MP_RESULT_K (ptr));
-	r = (char *) _REENT_MP_RESULT (ptr);
-
-	if (r == NULL)
-		return (
-#ifndef MULTIPLE_THREADS
-		dtoa_result =
-#endif
-			NULL);
+	char *r = _mprslt_alloc_str(ptr, i);
+
 	return
 #ifndef MULTIPLE_THREADS
 	dtoa_result =
@@ -102,9 +87,11 @@ freedtoa(ptr, s) struct _reent *ptr, char *s;
 freedtoa(struct _reent *ptr, char *s)
 #endif
 {
-	/* Free buffer allocated in a compatible way with legacy _ldtoa_r(). */
-	Bigint *b = (Bigint *)s;
-	b->_maxwds = 1 << (b->_k = _REENT_MP_RESULT_K (ptr));
+	Bigint *const b = (Bigint *)((int *)s - 1);
+
+	if (b == _REENT_MP_RESULT(ptr))
+		_REENT_MP_RESULT(ptr) = NULL;
+	b->_maxwds = 1 << (b->_k = *(int*)b);
 	Bfree(ptr, b);
 #ifndef MULTIPLE_THREADS
 	if (s == dtoa_result)
diff --git a/newlib/libc/stdlib/gdtoa-ldtoa.c b/newlib/libc/stdlib/gdtoa-ldtoa.c
index d5db270a6..1e8aa5906 100644
--- a/newlib/libc/stdlib/gdtoa-ldtoa.c
+++ b/newlib/libc/stdlib/gdtoa-ldtoa.c
@@ -72,8 +72,9 @@ _ldtoa_r(struct _reent *ptr,
 
 	/* reentrancy addition to use mprec storage pool */
 	if (_REENT_MP_RESULT (ptr)) {
-		freedtoa (ptr, (char *) _REENT_MP_RESULT (ptr));
+		char *s = _mprsv_ptr_from_bigint(_REENT_MP_RESULT(ptr));
 		_REENT_MP_RESULT (ptr) = 0;
+		freedtoa (ptr, s);
 	}
 
 	/*
diff --git a/newlib/libc/stdlib/ldtoa.c b/newlib/libc/stdlib/ldtoa.c
index 36613faad..84c94a5b7 100644
--- a/newlib/libc/stdlib/ldtoa.c
+++ b/newlib/libc/stdlib/ldtoa.c
@@ -2812,10 +2812,7 @@ _ldtoa_r (struct _reent *ptr, long double d, int mode, int ndigits,
 /* reentrancy addition to use mprec storage pool */
   if (_REENT_MP_RESULT (ptr))
     {
-      _REENT_MP_RESULT (ptr)->_k = _REENT_MP_RESULT_K (ptr);
-      _REENT_MP_RESULT (ptr)->_maxwds = 1 << _REENT_MP_RESULT_K (ptr);
-      Bfree (ptr, _REENT_MP_RESULT (ptr));
-      _REENT_MP_RESULT (ptr) = 0;
+      _mprslt_free_blck (ptr, _REENT_MP_RESULT(ptr)); /* release and free */
     }
 
 #if LDBL_MANT_DIG == 24
@@ -2947,14 +2944,10 @@ stripspaces:
   else				/* account for sign + max precision digs + E + exp sign + exponent */
     i = orig_ndigits + MAX_EXP_DIGITS + 4;
 
-  j = sizeof (__ULong);
-  for (_REENT_MP_RESULT_K (ptr) = 0;
-       sizeof (_Bigint) - sizeof (__ULong) + j <= i; j <<= 1)
-    _REENT_MP_RESULT_K (ptr)++;
-  _REENT_MP_RESULT (ptr) = eBalloc (ptr, _REENT_MP_RESULT_K (ptr));
+  (void) j; /*unused*/
 
 /* Copy from internal temporary buffer to permanent buffer.  */
-  outstr = (char *) _REENT_MP_RESULT (ptr);
+  outstr = _mprslt_alloc_str (ptr, i);
   strcpy (outstr, outbuf);
 
   if (rve)
diff --git a/newlib/libc/stdlib/mprec.c b/newlib/libc/stdlib/mprec.c
index 1f534b068..e76e2381a 100644
--- a/newlib/libc/stdlib/mprec.c
+++ b/newlib/libc/stdlib/mprec.c
@@ -137,6 +137,17 @@ Balloc (struct _reent *ptr, int k)
   return rv;
 }
 
+_Bigint *
+BAllocMemBlock (struct _reent *ptr, int n)
+{
+  int j, k;
+
+  for (j = sizeof(__ULong), k = 0;
+	sizeof(_Bigint) - sizeof(__ULong) + j < n; j <<= 1)
+    ++k;
+  return eBalloc (ptr, k);
+}
+
 void
 Bfree (struct _reent *ptr, _Bigint * v)
 {
diff --git a/newlib/libc/stdlib/mprec.h b/newlib/libc/stdlib/mprec.h
index 83de932c2..4d1920cff 100644
--- a/newlib/libc/stdlib/mprec.h
+++ b/newlib/libc/stdlib/mprec.h
@@ -406,5 +406,24 @@ extern const double tens[];
 extern const unsigned char __hexdig[];
 #endif /* !defined(PREFER_SIZE_OVER_SPEED) && !defined(__OPTIMIZE_SIZE__) && !defined(_SMALL_HEXDIG) */
 
-
 double _mprec_log10 (int);
+
+#define BAllocMemBlock _balloc_mprslt_block /* Allocation of a 'raw' block of at least n bytes */
+_Bigint * BAllocMemBlock (struct _reent *ptr, int n);
+
+#define _mprsv_ptr_from_bigint(_p)	((char*)((int*)(_p) + 1))
+
+#define _mprslt_alloc_str(ptr, __len) ({ \
+	_Bigint * __blk = BAllocRawBlock(__len + 1 + sizeof(int)); \
+	if (!__blk) __unreachable(); /*checked!*/ \
+	_REENT_MP_RESULT_K(ptr) = *(int*)__blk = __blk->_k; \
+	_REENT_MP_RESULT(ptr) = __blk; \
+	_mprsv_ptr_from_bigint(__blk); \
+	})
+
+#define _mprslt_free_blck(ptr, __ref) ({ \
+	_Bigint * __blk = /*(_Bigint*)*/(__ref); \
+	(__ref) = 0; \
+	__blk->_maxwds = 1 << (__blk->_k = *(int*)__blk); \
+	Bfree (ptr, __blk); \
+	})
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.