[rlgl] Undefined behavior in `rlLoadTexture` when computing applying a zero offset to a null pointer

[ashn-dot-dev]

Please, before submitting a new issue verify and check:

• I tested it on latest raylib version from master branch
• I checked there is no similar issue already reported
  • Similar issue in scope (but separate in cause) to [models] UBSAN flags 0 offset to NULL in DrawMesh #1891.
• I checked the documentation on the wiki
• My code has no errors or misuse of raylib
  • Can be replicated using an in-tree example program.

Issue description

The function rlLoadTexture contains the call glTexImage2D(GL_TEXTURE_2D, i, glInternalFormat, mipWidth, mipHeight, 0, glFormat, glType, (unsigned char *)data + mipOffset) here. When data is NULL, the expression (unsigned char *)data + mipOffset will resolve to NULL + 0 in the first iteration of the mipmap load loop.

Environment

Discovered on commit a86c93e using a PLATFORM_DESKTOP build for Linux via Windows Subsystem for Linux with CUSTOM_CFLAGS='-fsanitize=address,undefined'.

OpenGL information:

INFO: GL: OpenGL device information:
INFO:     > Vendor:   Microsoft Corporation
INFO:     > Renderer: D3D12 (NVIDIA GeForce RTX 3070 Ti)
INFO:     > Version:  4.2 (Core Profile) Mesa 23.0.4-0ubuntu1~22.04.1
INFO:     > GLSL:     4.20

Issue Screenshot

N/A

Code Example

The UBSan error will trigger on the in-tree example examples/shapes/shapes_top_down_lights.c on commit a86c93e without modification.

$ (cd /path/to/raylib/src/ && make clean all CC=clang CUSTOM_CFLAGS='-fsanitize=address,undefined' && sudo make install)
$ (cd /path/to/raylib/examples/ && make clean shapes/shapes_top_down_lights)
$ /path/to/raylib/examples/shapes/shapes_top_down_lights 2>&1 | grep 'runtime error'
rlgl.h:3014:175: runtime error: applying zero offset to null pointer

[ashn-dot-dev]

Note that the call to glCompressedTexImage2D just below the offending line here performs the same calculation.

[raysan5]

@ashn-dot-dev In raylib is up to the user to validate the provided data, in many functions there is no data validation. Due to the undefined-behaviour nature of this case, I just send a small review with a security check.

[ashn-dot-dev]

@raysan5 Thanks for the review.

Looking into this bug a bit more this morning, I was able to reproduce the UB via UBSan with the minimal example provided below:

#include <raylib.h>

int main(void)
{
    InitWindow(800, 600, "test");

    RenderTexture texture = LoadRenderTexture(GetScreenWidth(), GetScreenHeight());
    UnloadRenderTexture(texture);

    CloseWindow();
    return 0;
}

So RE your comment about data validation:

In raylib is up to the user to validate the provided data, in many functions there is no data validation.

I think this might have been a specific case where additional user validation would not have helped. This information is not particularly relevant now that there is the fix provided by e4dcbd5, but I thought it was worth noting for anyone looking at this issue in the future.

[Not-Nik]

This is a regression and breaks LoadRenderTexture because it calls the function with NULL

[raysan5]

@Not-Nik Good catch, reverting this change.

[Not-Nik]

The correct way to fix this would be to check if the pointer is NULL and then don't do any arithmetic on it/mip-mapping for that texture.

If that's too ugly you can avoid undefined behaviour by first casting the data pointer to an integer (uintptr_t), then adding the offset and casting it back. This is well-defined on the raylib side, because we're not accessing the resulting pointer, but may cause undefined behaviour on the OpenGL side, because such integer additions aren't guaranteed to end up pointing to an element in the array. However this is a risk we run either way, in case the pixel data isn't actually encoded as a unsigned char array. Even an array of Color's might not work when the compiler adds padding to the struct.

I, personally, prefer the latter option.

[raysan5]

@Not-Nik Afaik, this issue has never transalated into a real problem, so I prefer to keep as it is for now. Maybe adding a comment explaining that could be enough, just in case someone really finds this problem in a future.