Skip to content

Fix crash in nk_buffer_realloc and forbid the use of realloc#878

Open
sleeptightAnsiC wants to merge 1 commit intoImmediate-Mode-UI:masterfrom
sleeptightAnsiC:fix__realloc_crash
Open

Fix crash in nk_buffer_realloc and forbid the use of realloc#878
sleeptightAnsiC wants to merge 1 commit intoImmediate-Mode-UI:masterfrom
sleeptightAnsiC:fix__realloc_crash

Conversation

@sleeptightAnsiC
Copy link
Contributor

@sleeptightAnsiC sleeptightAnsiC commented Jan 27, 2026

Fixes: #768

Please read the linked issue for explanation.

The default implementation of nk_allocator (aka nk_malloc) is fundamentally broken, and should have used realloc instead of malloc in the first place. This must have led to the workaround in nk_buffer_realloc that would crash whenever you use an allocator with correct realloc assumption.

We cannot change nk_malloc at this point as people could have implemented their own allocators based on same assumptions, nor we cannot change the nk_allocator interface because it would be a breaking change (and because we may actually want to use realloc in a future) but we can ensure that Nuklear itself never tries to reallocate memory in its internal code (which is already happening anyway, this only enforces it)

This is kinda ugly, but it still leaves a room for making a breaking change later.

@sleeptightAnsiC sleeptightAnsiC marked this pull request as draft February 16, 2026 22:49
@sleeptightAnsiC

This comment was marked as outdated.

sleeptightAnsiC added a commit to sleeptightAnsiC/Nuklear that referenced this pull request Mar 11, 2026
In short: the default implementation of nk_allocator (aka nk_malloc)
is fundamentally broken, and should have used realloc instead of malloc.
This led to the strange workaround in nk_buffer_realloc that would
crash whenever you use custom allocator with correct realloc assumption.
We cannot change nk_malloc at this point, as people could have
implemented their own allocators based on it, but we can ensure that
Nuklear itself will never try to reallocate memory.

Additionally, I removed the assert checking for alloc failure,
because Nuklear never do this for allocations; and also added
a different one that checks agains shrink, because this code never
suported such case, so it's better to catch it than to corrupt memory.

Fixes: Immediate-Mode-UI#768
Ref: Immediate-Mode-UI#878 (comment)
Ref: Immediate-Mode-UI#878 (comment)
@sleeptightAnsiC
Copy link
Contributor Author

sleeptightAnsiC commented Mar 11, 2026

@RobLoach any chance for getting this reviewed again?

Fixed all the points from my previous comments, and removed the changelog entry (because it's about to be removed in #909) I've decided to patch every suspicious line in that function. If if (temp != b->memory.ptr) { branch went unnoticed for so long, I don't believe I should really trust the existing code there.

@sleeptightAnsiC sleeptightAnsiC marked this pull request as ready for review March 11, 2026 14:03
In short: the default implementation of nk_allocator (aka nk_malloc)
is fundamentally broken, and should have used realloc instead of malloc.
This led to the strange workaround in nk_buffer_realloc that would
crash whenever you use custom allocator with correct realloc assumption.
We cannot change nk_malloc at this point, as people could have
implemented their own allocators based on it, but we can ensure that
Nuklear's internal code will never try to reallocate memory.

Fixes: Immediate-Mode-UI#768
Reported-by: David Delassus <david.jose.delassus@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Memory safety (use after free) issue in nk_buffer_realloc

2 participants