[PATCH] newlib: Fix memory leak regarding gdtoa-based _ldtoa_r().

[PATCH] newlib: Fix memory leak regarding gdtoa-based _ldtoa_r().

[Takashi Yano]

After the commit a4705d387f78, printf() for floating-point values
causes a memory leak. The legacy _ldtoa_r() assumed the char pointer
returned will be free'ed by Bfree(). However, gdtoa-based _ldtoa_r()
returns the pointer returned by gdtoa() which should be free'ed by
freedtoa(). Due to this issue, the caller of _ldtoa_r() fails to free
the allocated char buffer. This is the cause of the said memory leak.
https://cygwin.com/pipermail/cygwin/2023-July/254054.html

With this patch, a new buffer is allocated using Balloc() and the
buffer returned by gdtoa() is copied into it. Then, free the original
buffer using freedtoa().

Fixes: a4705d387f78 ("ldtoa: Import gdtoa from OpenBSD.")
Reported-by: natan_b <natan_b@libero.it>
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
---
 newlib/libc/stdlib/gdtoa-ldtoa.c | 21 +++++++++++++++++++--
 winsup/cygwin/release/3.4.8      |  3 +++
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/newlib/libc/stdlib/gdtoa-ldtoa.c b/newlib/libc/stdlib/gdtoa-ldtoa.c
index 14b99042c..7a484613f 100644
--- a/newlib/libc/stdlib/gdtoa-ldtoa.c
+++ b/newlib/libc/stdlib/gdtoa-ldtoa.c
@@ -63,7 +63,8 @@ _ldtoa_r(struct _reent *ptr,
 #endif
 	};
 	int be, kind;
-	char *ret;
+	int i, j;
+	char *ret, *outbuf;
 	struct ieee_ext *p = (struct ieee_ext *)&ld;
 	uint32_t bits[(LDBL_MANT_DIG + 31) / 32];
 	void *vbits = bits;
@@ -113,7 +114,23 @@ _ldtoa_r(struct _reent *ptr,
 		abort();
 	}
 
-	ret = gdtoa(ptr, &fpi, be, vbits, &kind, mode, ndigits, decpt, rve);
+	outbuf = gdtoa(ptr, &fpi, be, vbits, &kind, mode, ndigits, decpt, rve);
+
+	i = strlen (outbuf);
+	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));
+
+	/* Copy from gdtoa-type buffer (which is allocated by rv_alloc())
+	   to the buffer used by ldtoa (which is allocated by Balloc()). */
+	ret = (char *) _REENT_MP_RESULT (ptr);
+	strcpy (ret, outbuf);
+	if (rve)
+		*rve = ret + (*rve - outbuf);
+	freedtoa (ptr, outbuf);
+
 	if (*decpt == -32768)
 		*decpt = INT_MAX;
 	return ret;
diff --git a/winsup/cygwin/release/3.4.8 b/winsup/cygwin/release/3.4.8
index d37272eef..448831c65 100644
--- a/winsup/cygwin/release/3.4.8
+++ b/winsup/cygwin/release/3.4.8
@@ -14,3 +14,6 @@ Bug Fixes
 - Rename internal macros _NL_CTYPE_OUTDIGITSx_MB/WC to GLibc compatible
   _NL_CTYPE_OUTDIGITx_MB/WC.
   Addresses: https://cygwin.com/pipermail/cygwin-developers/2023-July/012637.html
+
+- Fix memory leak in printf() regarding gdtoa-based _ldtoa_r().
+  Addresses: https://cygwin.com/pipermail/cygwin/2023-July/254054.html
-- 
2.39.0

Re: [PATCH] newlib: Fix memory leak regarding gdtoa-based _ldtoa_r().

[Corinna Vinschen]

Hi Takashi,

On Aug  1 17:57, Takashi Yano wrote:
> After the commit a4705d387f78, printf() for floating-point values
> causes a memory leak. The legacy _ldtoa_r() assumed the char pointer
> returned will be free'ed by Bfree(). However, gdtoa-based _ldtoa_r()
> returns the pointer returned by gdtoa() which should be free'ed by
> freedtoa(). Due to this issue, the caller of _ldtoa_r() fails to free
> the allocated char buffer. This is the cause of the said memory leak.
> https://cygwin.com/pipermail/cygwin/2023-July/254054.html
> 
> With this patch, a new buffer is allocated using Balloc() and the
> buffer returned by gdtoa() is copied into it. Then, free the original
> buffer using freedtoa().

Basically this is ok. But the question is this.  Isn't it possible
to redefine the allocator in gdtoa so that it uses a Balloced buffer
right away?  Forinstance, by using overloading macros?  That would
allow to get away without having to copy stuff...


Thanks,
Corinna


> 
> Fixes: a4705d387f78 ("ldtoa: Import gdtoa from OpenBSD.")
> Reported-by: natan_b <natan_b@libero.it>
> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> ---
>  newlib/libc/stdlib/gdtoa-ldtoa.c | 21 +++++++++++++++++++--
>  winsup/cygwin/release/3.4.8      |  3 +++
>  2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/newlib/libc/stdlib/gdtoa-ldtoa.c b/newlib/libc/stdlib/gdtoa-ldtoa.c
> index 14b99042c..7a484613f 100644
> --- a/newlib/libc/stdlib/gdtoa-ldtoa.c
> +++ b/newlib/libc/stdlib/gdtoa-ldtoa.c
> @@ -63,7 +63,8 @@ _ldtoa_r(struct _reent *ptr,
>  #endif
>  	};
>  	int be, kind;
> -	char *ret;
> +	int i, j;
> +	char *ret, *outbuf;
>  	struct ieee_ext *p = (struct ieee_ext *)&ld;
>  	uint32_t bits[(LDBL_MANT_DIG + 31) / 32];
>  	void *vbits = bits;
> @@ -113,7 +114,23 @@ _ldtoa_r(struct _reent *ptr,
>  		abort();
>  	}
>  
> -	ret = gdtoa(ptr, &fpi, be, vbits, &kind, mode, ndigits, decpt, rve);
> +	outbuf = gdtoa(ptr, &fpi, be, vbits, &kind, mode, ndigits, decpt, rve);
> +
> +	i = strlen (outbuf);
> +	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));
> +
> +	/* Copy from gdtoa-type buffer (which is allocated by rv_alloc())
> +	   to the buffer used by ldtoa (which is allocated by Balloc()). */
> +	ret = (char *) _REENT_MP_RESULT (ptr);
> +	strcpy (ret, outbuf);
> +	if (rve)
> +		*rve = ret + (*rve - outbuf);
> +	freedtoa (ptr, outbuf);
> +
>  	if (*decpt == -32768)
>  		*decpt = INT_MAX;
>  	return ret;
> diff --git a/winsup/cygwin/release/3.4.8 b/winsup/cygwin/release/3.4.8
> index d37272eef..448831c65 100644
> --- a/winsup/cygwin/release/3.4.8
> +++ b/winsup/cygwin/release/3.4.8
> @@ -14,3 +14,6 @@ Bug Fixes
>  - Rename internal macros _NL_CTYPE_OUTDIGITSx_MB/WC to GLibc compatible
>    _NL_CTYPE_OUTDIGITx_MB/WC.
>    Addresses: https://cygwin.com/pipermail/cygwin-developers/2023-July/012637.html
> +
> +- Fix memory leak in printf() regarding gdtoa-based _ldtoa_r().
> +  Addresses: https://cygwin.com/pipermail/cygwin/2023-July/254054.html
> -- 
> 2.39.0

Re: [PATCH] newlib: Fix memory leak regarding gdtoa-based _ldtoa_r().

[Takashi Yano]

On Tue, 1 Aug 2023 14:35:54 +0200
Corinna Vinschen wrote:
> Hi Takashi,
> 
> On Aug  1 17:57, Takashi Yano wrote:
> > After the commit a4705d387f78, printf() for floating-point values
> > causes a memory leak. The legacy _ldtoa_r() assumed the char pointer
> > returned will be free'ed by Bfree(). However, gdtoa-based _ldtoa_r()
> > returns the pointer returned by gdtoa() which should be free'ed by
> > freedtoa(). Due to this issue, the caller of _ldtoa_r() fails to free
> > the allocated char buffer. This is the cause of the said memory leak.
> > https://cygwin.com/pipermail/cygwin/2023-July/254054.html
> > 
> > With this patch, a new buffer is allocated using Balloc() and the
> > buffer returned by gdtoa() is copied into it. Then, free the original
> > buffer using freedtoa().
> 
> Basically this is ok. But the question is this.  Isn't it possible
> to redefine the allocator in gdtoa so that it uses a Balloced buffer
> right away?  Forinstance, by using overloading macros?  That would
> allow to get away without having to copy stuff...

Thanks for the advice. I tried that. Please check v2 patch.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>