Memory safety (use after free) issue in `nk_buffer_realloc`

[linkdd]

Actual Behavior

The nk_allocator interface requires an allocation function which takes a pointer as argument. This suggests that we should use realloc() if said pointer is not NULL (but the default implementation actually ignores this argument, and simply use malloc()).

In the function nk_buffer_realloc(), there is a call to the allocation function which passes the buffer memory as argument. But then, if the result of this function is different from the old buffer memory pointer, the old pointer is freed:

https://github.com/Immediate-Mode-UI/Nuklear/blob/master/nuklear.h#L8592-L8595

This is introduce a memory safety issue (use after free). The realloc() function might return the same memory block at a different address:

https://manpages.org/realloc/3

The realloc() function returns a pointer to the newly allocated memory, which is suitably aligned for any built-in type and may be different from ptr, or NULL if the request fails.

This means that if realloc() moves the pointer (and it often does), the nk_buffer_realloc() will free what it expects to be another allocation, and then the rest of the lib will try to use an allocation that was actually freed.

Desired Behavior

Option 1: You should only check for NULL and trust the behavior of nk_buffer_realloc() (which would rely on realloc() instead of malloc(), no more free+memcpy.

Option 2: Remove the ptr argument of the nk_plugin_alloc callback, so that it is 100% clear that realloc() is not expected here.

[sleeptightAnsiC]

[EDIT: Sorry for such a long message. There is no easy TLDR to describe it. I wrote this all by hand]

I also encountered this issue and wanted to point few things out, because it's a complex problem...

Note, that this only occurs when user provides its own nk_allocator, and only in cases where nk_buffer_realloc results in realloc claiming new memory index.

The nk_allocator interface requires an allocation function which takes a pointer as argument. This suggests that we should use realloc() if said pointer is not NULL (but the default implementation actually ignores this argument, and simply use malloc()).

True, the current implementation of nk_malloc is wrong:

Nuklear/src/nuklear_buffer.c

Lines 10 to 16 in fcd64f8

	NK_LIB void*
	nk_malloc(nk_handle unused, void *old,nk_size size)
	{
	NK_UNUSED(unused);
	NK_UNUSED(old);
	return malloc(size);
	}

^ this thing should NOT ignore old pointer, and should use realloc instead, because this is how allocator was designed in the first place. Also the name nk_malloc is missleading (it should be nk_realloc). It's an internal symbol so it can be easily changed.

In the function nk_buffer_realloc(), there is a call to the allocation function which passes the buffer memory as argument. But then, if the result of this function is different from the old buffer memory pointer, the old pointer is freed [...] This is introduce a memory safety issue (use after free). The realloc() function might return the same memory block at a different address.

True, this whole code block here is just broken:

Nuklear/src/nuklear_buffer.c

Lines 106 to 115 in fcd64f8

	buffer_size = b->memory.size;
	temp = b->pool.alloc(b->pool.userdata, b->memory.ptr, capacity);
	NK_ASSERT(temp);
	if (!temp) return 0;
	*size = capacity;
	if (temp != b->memory.ptr) {
	NK_MEMCPY(temp, b->memory.ptr, buffer_size);
	b->pool.free(b->pool.userdata, b->memory.ptr);
	}

^ after grepping the sources, this appears to be the only place in Nuklear, where the semantic of realloc(old, size) is used. The rest of library simply uses realloc(NULL, size) (which has the same behavior as malloc).

That's why nk_buffer_realloc is the only place affected by this problem, despite that whole library depends on broken behavior of nk_malloc.

---

Now, about why this is problematic to fix...

Option 1: You should only check for NULL and trust the behavior of nk_buffer_realloc() (which would rely on realloc() instead of malloc(), no more free+memcpy.

Yes, someone who wrote this thing in the first place clearly encountered this issue, and attempted to fix it by hacking around it. The current hack, for some reason, tries to use realloc but also fallbacks to malloc+memcpy+free, which makes no sense at all, and made me super confused while reading it.

Option 2: Remove the ptr argument of the nk_plugin_alloc callback, so that it is 100% clear that realloc() is not expected here.

Changing nk_plugin_alloc would break the API. It seems that a lot of people depended on the broken behavior of it being always malloc, but it would still be a breaking change.

I think the correct approach should be:

• nuke the nk_malloc and replace it with "nk_realloc" (this symbol is internal anyway)
• nuke the free+memcpy hack from nk_buffer_realloc (this branch makes no sense anyway)
• if someone's code breaks due to those changes, it's not our problem, because this can only happen due to wrong use of nk_allocator

However, I'm still afraid this will piss off someone who depends on current broken behavior, and so I'm not entirely sure that my approach is the best one...

[linkdd]

Thank you for the feedback.

Though, I do believe, people depending on such a broken behavior deserve to have their code broken by the fix :)

I don't know if Nuklear follows semantic versioning, but if it does then it could be a major release (since it would be a breaking change).

https://xkcd.com/1172/

[sleeptightAnsiC]

Just for cross reference:

Not so long ago I've added the demo that references this exact issue with a fixme note.
We have to remember to update said code after fixing this.

Nuklear/demo/sdl3_renderer/nuklear_sdl3_renderer.h

Lines 111 to 125 in 9afb3dd

	NK_INTERN void *
	nk_sdl_alloc(nk_handle user, void *old, nk_size size)
	{
	NK_UNUSED(user);
	/* FIXME: nk_sdl_alloc should use SDL_realloc here, not SDL_malloc
	* but this could cause a double-free due to bug within Nuklear, see:
	* https://github.com/Immediate-Mode-UI/Nuklear/issues/768
	* */
	#if 0
	return SDL_realloc(old, size);
	#else
	NK_UNUSED(old);
	return SDL_malloc(size);
	#endif
	}

[sleeptightAnsiC]

I think the correct approach should be:

• nuke the nk_malloc and replace it with "nk_realloc" (this symbol is internal anyway)
• nuke the free+memcpy hack from nk_buffer_realloc (this branch makes no sense anyway)
• if someone's code breaks due to those changes, it's not our problem, because this can only happen due to wrong use of nk_allocator

However, I'm still afraid this will piss off someone who depends on current broken behavior, and so I'm not entirely sure that my approach is the best one...

I do believe, people depending on such a broken behavior deserve to have their code broken by the fix :)

I thought about this solution for quite some time and although I still think it is "technically correct", I've found several repositories that include Nuklear in its single-header-mode and reuse various private symbols all over the place (example).

Also, the fact that nk_allocator/nk_malloc were fundamentally broken since forever means it's very likely that people implemented their own allocators based on the same assumptions. Breaking someone's custom allocator just in order to allow this one specific place in the library to call realloc is really not a good solution.

Given all the reasoning above, my current idea about fixing this is as follows:

• inside of nk_buffer_realloc: remove realloc attempt and always call malloc+memcpy+free
• inside of nk_malloc: replace UNUSED(old) with assert(old)
• forbid the use of realloc in whole library (the assert in nk_malloc will enforce this rule)

This solution will fix the crash, keep the old code running, and won't break anything outside of library, and if for some reason this will not be enough, we will still have a way to change it later (unlike my previously suggested fix).

[sleeptightAnsiC]

@linkdd mind retesting with #878 ?