Bug in reallocf(3) when calling realloc(3)

Bug in reallocf(3) when calling realloc(3)

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 3550 bytes --]

Bon dia Guillem,

I've just checked the implementation of reallocf(3bsd), and it looks
like it has a memory leak.  Admittedly, a small one, but I just wanted
to report it.  I'll suggest two proposals, plus keeping the status quo,
and explain the advantages of each of them.

Here's the current code in libbsd:

	$ grepc -tfd reallocf .
	./src/reallocf.c:void *
	reallocf(void *ptr, size_t size)
	{
		void *nptr;

		nptr = realloc(ptr, size);

		/*
		 * When the System V compatibility option (malloc "V" flag) is
		 * in effect, realloc(ptr, 0) frees the memory and returns NULL.
		 * So, to avoid double free, call free() only when size != 0.
		 * realloc(ptr, 0) can't fail when ptr != NULL.
		 */
		if (!nptr && ptr && size != 0)
			free(ptr);
		return (nptr);
	}

The last sentence in that comment is false.  realloc(nonnull,0) can fail
in systems in which realloc(nonnull,0) normally returns non-null, such
as the BSDs or musl.  Let's say the system is unable to find enough
space for the metadata necessary for the new 0-sized block of memory.
It would return NULL and keep the old pointer intact.

I've CCed musl@ so that they can verify that this is true on their
implementation.  I didn't see anything in their code suggesting that it
can't fail.

The same also seems to be true in FreeBSD, if I'm reading correctly.
I've also CCed them, so that they can confirm, and also because the
libbsd implementation is pasted from FreeBSD, so they have the same bug.

So, assuming that r(p,0) can indeed fail, there are two alternatives
that are portable to every POSIX system:

a)  Check ENOMEM.

	void *
	reallocf(void *ptr, size_t size)
	{
		int   e;
		void  *nptr;

 		e = errno;

		errno = 0;
		nptr = realloc(ptr, size);
		if (nptr == NULL) {
			if (errno == ENOMEM)
				free(ptr);
			return NULL;
		}

		errno = e;
		return nptr;
	}

    This approach is very complex, although it is the most pedantically
    correct approach (unless I introduced accidentally some bug, which
    might happen, due to the difficulty of calling realloc(3) portably;
    blame SysV).

b)  Don't do that at all; pass a dummy size of 1.

	void *
	reallocf(void *ptr, size_t size)
	{
		void  *nptr;

		nptr = realloc(ptr, size ? size : 1);
		if (nptr == NULL)
			free(ptr);

		return nptr;
	}

    This approach is the simplest, and it's portable to all realloc(3)s
    that have ever existed.  However, it allocates one byte too much,
    which has a bad consequence: it can hide logic errors where the
    programmer is accessing one byte after the requested size.
    Sanitizers would think that the :1 was done on purpose, and thus be
    silent.

c)  You could keep the memory leak.  It should be very rare, I guess.
    But you never know.  I think b) is simple enough and the drawback is
    also rare enough (because a logic error that only manifests for a
    size of 0 should be rare, even though not impossible).

BTW, I'm working on a proposal to restore the traditional specification
of realloc(3), the one that allocates a zero-sized object (returns
non-null), the one inherited from Unix V7.  There's some support, and i
think it might pass in the C Committee and the Austin Group.  But feel
free to chime in and voice your support if you're interested in it.
<https://inbox.sourceware.org/libc-alpha/limjtao3jpge6hrrjliko4w6p5t7cfpbc3m3etodqgsxayi3hw@gv4eml5icqb5/T/#u>


Have a lovely day!
Alex

-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Bug in reallocf(3) when calling realloc(3)

[Rich Felker]

On Sat, Jun 21, 2025 at 09:10:58PM +0200, Alejandro Colomar wrote:
> Bon dia Guillem,
> 
> I've just checked the implementation of reallocf(3bsd), and it looks
> like it has a memory leak.  Admittedly, a small one, but I just wanted
> to report it.  I'll suggest two proposals, plus keeping the status quo,
> and explain the advantages of each of them.
> 
> Here's the current code in libbsd:
> 
> 	$ grepc -tfd reallocf .
> 	./src/reallocf.c:void *
> 	reallocf(void *ptr, size_t size)
> 	{
> 		void *nptr;
> 
> 		nptr = realloc(ptr, size);
> 
> 		/*
> 		 * When the System V compatibility option (malloc "V" flag) is
> 		 * in effect, realloc(ptr, 0) frees the memory and returns NULL.
> 		 * So, to avoid double free, call free() only when size != 0.
> 		 * realloc(ptr, 0) can't fail when ptr != NULL.
> 		 */
> 		if (!nptr && ptr && size != 0)
> 			free(ptr);
> 		return (nptr);
> 	}
> 
> The last sentence in that comment is false.  realloc(nonnull,0) can fail
> in systems in which realloc(nonnull,0) normally returns non-null, such
> as the BSDs or musl.  Let's say the system is unable to find enough
> space for the metadata necessary for the new 0-sized block of memory.
> It would return NULL and keep the old pointer intact.
> 
> I've CCed musl@ so that they can verify that this is true on their
> implementation.  I didn't see anything in their code suggesting that it
> can't fail.

Yes this is true. realloc returning null *always* indicates failure
(and accordingly, memory has not been freed) in our past and current
implementations, and indeed it can fail when resizing down (to zero or
otherwise).

Rich

Re: Bug in reallocf(3) when calling realloc(3)

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 2092 bytes --]

Hi Rich,

On Sat, Jun 21, 2025 at 03:27:51PM -0400, Rich Felker wrote:
> On Sat, Jun 21, 2025 at 09:10:58PM +0200, Alejandro Colomar wrote:
> > Bon dia Guillem,
> > 
> > I've just checked the implementation of reallocf(3bsd), and it looks
> > like it has a memory leak.  Admittedly, a small one, but I just wanted
> > to report it.  I'll suggest two proposals, plus keeping the status quo,
> > and explain the advantages of each of them.
> > 
> > Here's the current code in libbsd:
> > 
> > 	$ grepc -tfd reallocf .
> > 	./src/reallocf.c:void *
> > 	reallocf(void *ptr, size_t size)
> > 	{
> > 		void *nptr;
> > 
> > 		nptr = realloc(ptr, size);
> > 
> > 		/*
> > 		 * When the System V compatibility option (malloc "V" flag) is
> > 		 * in effect, realloc(ptr, 0) frees the memory and returns NULL.
> > 		 * So, to avoid double free, call free() only when size != 0.
> > 		 * realloc(ptr, 0) can't fail when ptr != NULL.
> > 		 */
> > 		if (!nptr && ptr && size != 0)
> > 			free(ptr);
> > 		return (nptr);
> > 	}
> > 
> > The last sentence in that comment is false.  realloc(nonnull,0) can fail
> > in systems in which realloc(nonnull,0) normally returns non-null, such
> > as the BSDs or musl.  Let's say the system is unable to find enough
> > space for the metadata necessary for the new 0-sized block of memory.
> > It would return NULL and keep the old pointer intact.
> > 
> > I've CCed musl@ so that they can verify that this is true on their
> > implementation.  I didn't see anything in their code suggesting that it
> > can't fail.
> 
> Yes this is true. realloc returning null *always* indicates failure
> (and accordingly, memory has not been freed) in our past and current
> implementations, and indeed it can fail when resizing down (to zero or
> otherwise).

Thanks!

BTW, my message didn't reach freebsd-bugs@, as I'm not subscribed.  If
anyone is subscribed to that list and can resend my message, or can CC a
FreeBSD mainainer, it would be good.


Have a lovely day!
Alex

-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Bug in reallocf(3) when calling realloc(3)

[Florian Weimer]

* Rich Felker:

> Yes this is true. realloc returning null *always* indicates failure
> (and accordingly, memory has not been freed) in our past and current
> implementations, and indeed it can fail when resizing down (to zero or
> otherwise).

(limited Cc: list)

What's the rationale for that?  Wouldn't it better not to honor the
resize request at all?  The application might be able to continue to run
if no error is reported (although it's of course likely that it will
encounter failing fresh allocation soon after that).

Thanks,
Florian

Re: Bug in reallocf(3) when calling realloc(3)

[Rich Felker]

On Sun, Jun 22, 2025 at 06:29:00PM +0200, Florian Weimer wrote:
> * Rich Felker:
> 
> > Yes this is true. realloc returning null *always* indicates failure
> > (and accordingly, memory has not been freed) in our past and current
> > implementations, and indeed it can fail when resizing down (to zero or
> > otherwise).
> 
> (limited Cc: list)
> 
> What's the rationale for that?  Wouldn't it better not to honor the
> resize request at all?  The application might be able to continue to run
> if no error is reported (although it's of course likely that it will
> encounter failing fresh allocation soon after that).

I addressed roughly the same topic elsewhere in the thread:

https://www.openwall.com/lists/musl/2025/06/21/6

But I'll summarize here in a form directly answering the request for a
rationale:

If you lie and say the realloc "succeeded" when it just left the
allocation size alone, you've destroyed the ability for the
application to know where it has memory tied up and to be able to
recover from that, and deferred an OOM condition that the application
could potentially handled earlier by backing out and freeing large
objects that were actually responsible for tying up all the memory,
into a late one that it likely can't handle because now
supposedly-small objects are permanently tying up huge slots.

Further, if you lie and say the realloc "succeeded" when it just left
the allocation size alone, now the compiler "knows" the size of the
object is only the new, small size, but malloc_usable_size returns
some much larger number. And all sorts of fun things happen when the
compiler optimizes based on this "knowledge"... Having a byte-exact
malloc_usable_size to fix this issue was a key design goal, and it's
broken as soon as you make realloc lie.

Rich