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

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 <[email protected]>
> Signed-off-by: Takashi Yano <[email protected]>
> ---
>  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