Undefined behavior in the implementation of `nk_ptr_add`

[SnootierMoon]

On adding an integer to a pointer, the C standard says

If both the pointer operand and the result point to elements of the same array object, or one past the last element of the array object, the evaluation shall not produce an overflow; otherwise, the behavior is undefined.

This means this usage of nk_ptr_add triggers undefined behavior when b->size < size because it causes an overflow, and this can happen when the buffer needs to grow. In fact, I caught this error when trying to compile and run Nuklear for a Zig project, since Zig catches various kinds of undefined behavior in debug mode.

I think the solution is to cast the i parameter of nk_ptr_add to a signed type like ptrdiff_t to avoid the overflow. This requires adding a new type to the list of integer types here.

[PROP65]

I don't think casting i to a signed type in nk_ptr_add would be an appropriate fix as the underflow would occur before the cast and might cause it's own issues. I looked through the ANSI C standard and couldn't find any defined behavior for casting an unsigned type to a signed type incapable of storing it.

/* like this */
nk_byte b = 200;
nk_char c = (nk_char)b;

I believe you're correct, if b->size < size it could cause undefined behavior, but if this occurs something else has likely gone wrong to cause those values.

Since nk_buffer_alloc is an internal library function that is not intended for the end user, I think adding an assert to nk_buffer_alloc would be better if you believe another Nuklear function may pass arguments that could cause this.

[SnootierMoon]

You're right, I don't think the solution is to modify nk_ptr_add. I'm not sure why I thought this was the solution initially, because this would seemingly point unaligned to a negative offset of b->memory.ptr which is out-of-bounds.

It's been a while since I looked at this issue, but IIRC b->size < size seems to be a valid possibility in that line of code. If the buffer starts off small enough and the allocation is big enough, then b->size < size will be true. I still have the stacktrace from when I ran into this:

Illegal instruction at address 0x1c9e4e6
nuklear.h:8631:22: 0x1c9e4e6 in nk_buffer_alloc
    else unaligned = nk_ptr_add(void, b->memory.ptr, b->size - size);
                     ^
Nuklear/nuklear.h:9905:9: 0x1db067a in nk_draw_list_push_command
        nk_buffer_alloc(list->buffer, NK_BUFFER_BACK, cmd_size, cmd_align);
        ^
Nuklear/nuklear.h:9945:9: 0x1cbed5a in nk_draw_list_add_clip
        nk_draw_list_push_command(list, rect, list->config.tex_null.texture);
        ^
Nuklear/nuklear.h:10561:9: 0x1cbe9e4 in nk_draw_list_path_line_to
        nk_draw_list_add_clip(list, nk_null_rect);
        ^
Nuklear/nuklear.h:10644:9: 0x1cbfd70 in nk_draw_list_path_rect_to
        nk_draw_list_path_line_to(list, a);
        ^
Nuklear/nuklear.h:10727:9: 0x1cc0e60 in nk_draw_list_fill_rect
        nk_draw_list_path_rect_to(list, nk_vec2(rect.x, rect.y),
        ^
Nuklear/nuklear.h:10994:13: 0x1cc5465 in nk_convert
            nk_draw_list_fill_rect(&ctx->draw_list, nk_rect(r->x, r->y, r->w, r->h),
            ^

So e.g. size is cmd_size = sizeof(struct nk_draw_command) = 32 and b->size is the initial size from nk_buffer_init which I had set to less than 32. I probably should have used a bigger initial buffer size, but there should probably be a check for b->size < size in nk_buffer_alloc too.

[sleeptightAnsiC]

I believe you're correct, if b->size < size it could cause undefined behavior, but if this occurs something else has likely gone wrong to cause those values.

b->size is the initial size from nk_buffer_init which I had set to less than 32

Oh well, 32bytes is not a lot for an allocator to work with :D

This seems like an OOM condition. Allocator run out of memory and it's now reaching the code it should not, and only then you get the underflow as a symptom. What is a bit funny here is that there is an OOM check just few lines later but it's already too late to detect it.