[models] UBSAN flags 0 offset to NULL in DrawMesh

[rcorre]

WARNING: Please, read this note carefully before submitting a new issue:

It is important to realise that this is NOT A SUPPORT FORUM, this is for reproducible BUGS with raylib ONLY.

There are lots of generous and helpful people ready to help you out on raylib Discord forum or raylib reddit.

Remember that asking for support questions here actively takes developer time away from improving raylib.

---

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
• My code has no errors or misuse of raylib

Issue description

Briefly describe the issue you are experiencing (or the feature you want to see added to raylib). Tell us what you were trying to do and what happened instead. Remember, this is not the best place to ask questions. For questions, go to raylib Discord server.

UBSAN flags

raylib/src/models.c

Line 1186 in 87b5420

if (mesh.indices != NULL) rlDrawVertexArrayElements(0, mesh.triangleCount*3, 0);

as undefined behavior (adding an offset to a NULL pointer, which happens at

raylib/src/rlgl.h

Line 3124 in 87b5420

glDrawElements(GL_TRIANGLES, count, GL_UNSIGNED_SHORT, (unsigned short*)buffer + offset);

)

rlgl.h:3124:84: runtime error: applying zero offset to null pointer

From some reading around (e.g. https://patchwork.kernel.org/project/git/patch/[email protected]/), it sounds like adding any offset (even 0) to NULL is technically undefined behavior.

This was introduced sometime between raylib 3.5.0 and raylib 3.7.0, and still exists on master.

Environment

Linux, desktop.

Issue Screenshot

If possible, provide a screenshot that illustrates the issue. Usually an image is better than a thousand words.

Code Example

Provide minimal reproduction code to test the issue. Please, format the code properly and try to keep it as simple as possible, just focusing on the experienced issue.

1. Compile raylib with the undefined behavior sanitizer
2. Run shaders_basic_lighting example

[rcorre]

Actually I'm kind of confused by this line anyways:

            if (mesh.indices != NULL) rlDrawVertexArrayElements(0, mesh.triangleCount*3, 0);

Why check if indices is non-null, only to pass 0? Is that supposed to be

        if (mesh.indices != NULL) rlDrawVertexArrayElements(0, mesh.triangleCount*3, mesh.indices);

instead?

[raysan5]

@rcorre Good catch! Reviewed it!

[rcorre]

Oops, that might have been the wrong suggestion, sorrry :(

I now get a SIGABRT on shaders_basic_lighting:

Program received signal SIGABRT, Aborted.
0x00007ffff714ad22 in raise () from /usr/lib/libc.so.6
(gdb) bt
#0  0x00007ffff714ad22 in raise () from /usr/lib/libc.so.6
#1  0x00007ffff7134862 in abort () from /usr/lib/libc.so.6
#2  0x00007ffff4c5ded9 in ?? () from /usr/lib/libnvidia-glcore.so.470.57.02
#3  0x00007ffff4dd46aa in ?? () from /usr/lib/libnvidia-glcore.so.470.57.02
#4  0x00007ffff4d80a89 in ?? () from /usr/lib/libnvidia-glcore.so.470.57.02
#5  0x00007ffff4c469e1 in ?? () from /usr/lib/libnvidia-glcore.so.470.57.02
#6  0x00007ffff648607f in ?? () from /usr/lib/libGLX_nvidia.so.0
#7  0x00007ffff6451620 in ?? () from /usr/lib/libGLX_nvidia.so.0
#8  0x0000555555a2f7d6 in swapBuffersGLX (window=0x555556745dd0) at ./external/glfw/src/glx_context.c:187
#9  0x0000555555994883 in glfwSwapBuffers (handle=0x555556745dd0) at ./external/glfw/src/context.c:653
#10 0x00005555558223a0 in SwapScreenBuffer () at core.c:4470
#11 0x0000555555821987 in EndDrawing () at core.c:1975
#12 0x00005555557b7952 in main () at shaders/shaders_basic_lighting.c:137

[raysan5]

@rcorre Actually, my bad. I should have reviewed this issue more carefully. On the DrawMeshInstanced() function, previous to the draw call, I'm calling:

if (mesh.indices != NULL) rlEnableVertexBufferElement(mesh.vboId[6]);

That call already binds the mesh.indices buffer:

glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, id);

That function is explained in the official OpenGL documentation, here the relevant part:

While a non-zero buffer object is bound to the GL_ELEMENT_ARRAY_BUFFER target, 
the indices parameter of glDrawElements, glDrawElementsInstanced, glDrawElementsBaseVertex, 
glDrawRangeElements, glDrawRangeElementsBaseVertex, glMultiDrawElements, or glMultiDrawElementsBaseVertex is 
interpreted as an offset within the buffer object measured in basic machine units.

So, the previous implementation was correct.

[rcorre]

Great, thanks! Sorry for creating confusion.

Still, is it possible to avoid the undefined behavior (possibly by adding a separate function call for when the buffer and/or offset are 0)?

I think adding 0 to a null pointer is technically undefined, but even if it works, this prevents anyone from running a sanitized build of raylib (which might help catch bugs?)

[raysan5]

I'll see if it's possible to manage it in another way to avoid the undefined behaviour...

[raysan5]

@rcorre I'm reviewing this issue but I'm afraid it's not easy to fix, if you read the official OpenGL implementation detail, if indices are mapped, then that pointer parameter is actually interpreted as an offset value:

While a non-zero buffer object is bound to the GL_ELEMENT_ARRAY_BUFFER target, 
the indices parameter of glDrawElements, glDrawElementsInstanced, glDrawElementsBaseVertex, 
glDrawRangeElements, glDrawRangeElementsBaseVertex, glMultiDrawElements, or glMultiDrawElementsBaseVertex is 
interpreted as an offset within the buffer object measured in basic machine units.

Actually, that's the reason current implementation passes the value as 0 instead of NULL, because it requires to be interpreted as an offset value instead of being understood as a pointer.

So, the problem is inherent to OpenGL library implementation, not on raylib side.

[rcorre]

@raysan5 the issue isn't passing NULL, it is performing pointer arithmetic on NULL, which does happen in the raylib codebase.

If a breaking change is ok, I think the easiest thing is to just remove the offset parameter:

diff --git a/src/models.c b/src/models.c
index 3d200d2a..0d13bb13 100644
--- a/src/models.c
+++ b/src/models.c
@@ -994,7 +994,7 @@ void DrawMeshInstanced(Mesh mesh, Material material, Matrix *transforms, int ins
                    material.maps[MATERIAL_MAP_DIFFUSE].color.b,
                    material.maps[MATERIAL_MAP_DIFFUSE].color.a);
 
-        if (mesh.indices != NULL) rlDrawVertexArrayElements(0, mesh.triangleCount*3, mesh.indices);
+        if (mesh.indices != NULL) rlDrawVertexArrayElements(mesh.triangleCount*3, mesh.indices);
         else rlDrawVertexArray(0, mesh.vertexCount);
     rlPopMatrix();
 
@@ -1212,7 +1212,7 @@ void DrawMeshInstanced(Mesh mesh, Material material, Matrix *transforms, int ins
         }
         else    // Draw mesh
         {
-            if (mesh.indices != NULL) rlDrawVertexArrayElements(0, mesh.triangleCount*3, 0);
+            if (mesh.indices != NULL) rlDrawVertexArrayElements(mesh.triangleCount*3, 0);
             else rlDrawVertexArray(0, mesh.vertexCount);
         }
     }
diff --git a/src/rlgl.h b/src/rlgl.h
index 93227c2b..7aee2caa 100644
--- a/src/rlgl.h
+++ b/src/rlgl.h
@@ -569,7 +569,7 @@ RLAPI void rlSetVertexAttribute(unsigned int index, int compSize, int type, bool
 RLAPI void rlSetVertexAttributeDivisor(unsigned int index, int divisor);
 RLAPI void rlSetVertexAttributeDefault(int locIndex, const void *value, int attribType, int count); // Set vertex attribute default value
 RLAPI void rlDrawVertexArray(int offset, int count);
-RLAPI void rlDrawVertexArrayElements(int offset, int count, void *buffer);
+RLAPI void rlDrawVertexArrayElements(int count, void *buffer);
 RLAPI void rlDrawVertexArrayInstanced(int offset, int count, int instances);
 RLAPI void rlDrawVertexArrayElementsInstanced(int offset, int count, void *buffer, int instances);
 
@@ -3119,9 +3119,9 @@ void rlDrawVertexArray(int offset, int count)
     glDrawArrays(GL_TRIANGLES, offset, count);
 }
 
-void rlDrawVertexArrayElements(int offset, int count, void *buffer)
+void rlDrawVertexArrayElements(int count, void *buffer)
 {
-    glDrawElements(GL_TRIANGLES, count, GL_UNSIGNED_SHORT, (unsigned short *)buffer + offset);
+    glDrawElements(GL_TRIANGLES, count, GL_UNSIGNED_SHORT, (unsigned short *)buffer);
 }
 
 void rlDrawVertexArrayInstanced(int offset, int count, int instances)

The offset isn't used anywhere inside raylib, and I couldn't find any github projects using it either. Here's a search I generated with

gh api -X GET search/code -f q=rlDrawVertexArrayElements -H 'Accept: application/vnd.github.v3.text-match+json' | jq '.items[] | {repo: .repository.full_name, file: .name, text: .text_matches[] | .fragment}'

uses.json

[raysan5]

So, applying only the offset would be valid?

if (buffer == 0) glDrawElements(GL_TRIANGLES, count, GL_UNSIGNED_SHORT, offset);
else glDrawElements(GL_TRIANGLES, count, GL_UNSIGNED_SHORT, (unsigned short *)buffer + offset);

Note that raylib uses that offset internally for the batch rendering system, so, it could be useful for the users using rlgl.

[rcorre]

@raysan5 that works -- I just tried it to confirm.

[raysan5]

Just reverted this change, now the warnings pop in the compiler. I'll leave it AS DESIGN, independently of what UBSAN thinks. Feel free to modify it in your fork.

[rcorre]

@raysan5 do you remember why this had to be reverted? I couldn't find any info in the commit that reverted it.

[raysan5]

@rcorre I don't remember exactly the reason (warnings on compiler side) but I can give another review. The point is that current implementation works and any attempt to change it could potentially break it.

[raysan5]

@rcorre I reviewed the function again, please, could you verify if it still complaints?

[rcorre]

Yes, that fixed it @raysan5, thank you! Should we revert #3292?

[raysan5]

@rcorre probably yes, feel free to send a PR