Skip to content

emscripten: Fix undefined behavior in opengles2 renderer #12581

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 19, 2025

Conversation

castholm
Copy link
Contributor

@castholm castholm commented Mar 19, 2025

Discovered this while building SDL3 and the Snake example for Emscripten with UBSan. With the sanitizer enabled, running the example logs:

<omitted>/src/render/opengles2/SDL_render_gles2.c:1024:74: runtime error: subtraction of unsigned offset from 0x00000000 overflowed to 0x00000000
<omitted>/src/render/opengles2/SDL_render_gles2.c:1025:118: runtime error: member access within null pointer of type 'SDL_VertexSolid' (aka 'struct SDL_VertexSolid')
<omitted>/src/render/opengles2/SDL_render_gles2.c:1026:131: runtime error: member access within null pointer of type 'SDL_VertexSolid' (aka 'struct SDL_VertexSolid')

The first error happens because the program is adding an offset to a null pointer, which is UB.
The second and third happen because expressions like &foo->bar invoke UB when foo is a null pointer.

These only occur on Emscripten, because on Emscripten the renderer uses VBOs instead of client-side arrays and reassigns vertices to 0 after uploading the data.

By rewriting relevant code to use uintptr_t and offsetof we get the same results but without invoking UB.

From quickly skimming over SDL_render_gles2.c in the SDL2 branch it looks like the affected code is present there as well, so if merged this could probably be backported to SDL2 as well.

@slouken slouken merged commit 14deef9 into libsdl-org:main Mar 19, 2025
39 checks passed
@slouken
Copy link
Collaborator

slouken commented Mar 19, 2025

Merged, thanks!

@castholm castholm deleted the emscripten-render-gles2-ub branch March 20, 2025 23:44
@dschuff
Copy link

dschuff commented May 1, 2025

A recent change in clang (llvm/llvm-project#130742) has started to exploit this UB, resulting in SDL rendering being broken on Emscripten. What would we need to do to get this backported to SDL2? Do you just use PRs for that?

@slouken
Copy link
Collaborator

slouken commented May 1, 2025

What would we need to do to get this backported to SDL2? Do you just use PRs for that?

Yep, go ahead and create a tested PR and I'll be happy to merge it.

@dschuff
Copy link

dschuff commented May 1, 2025

Hm, I ported this patch more-or-less directly to the SDL2 branch but unfortunately it seems to be breaking our tests (in a way that seems unrelated to the original UB here; it seems to be rendering without any red). I'm wondering if I need to do something to get this to work in SDL2 beyond just applying the change here; before I go digging for that, does this ring any bells?

@castholm
Copy link
Contributor Author

castholm commented May 1, 2025

Hm, I ported this patch more-or-less directly to the SDL2 branch but unfortunately it seems to be breaking our tests (in a way that seems unrelated to the original UB here; it seems to be rendering without any red).

SDL_VertexSolid *verts = (SDL_VertexSolid *)(((Uint8 *)vertices) + cmd->data.draw.first);
data->glVertexAttribPointer(GLES2_ATTRIBUTE_POSITION, 2, GL_FLOAT, GL_FALSE, stride, (const GLvoid *)&verts->position);
data->glVertexAttribPointer(GLES2_ATTRIBUTE_COLOR, 4, GL_UNSIGNED_BYTE, GL_TRUE /* Normalized */, stride, (const GLvoid *)&verts->color);

uintptr_t base = (uintptr_t)vertices + cmd->data.draw.first; // address of first vertex, or base offset when using VBOs.
data->glVertexAttribPointer(GLES2_ATTRIBUTE_POSITION, 2, GL_FLOAT, GL_FALSE, stride, (const GLvoid *)(base + offsetof(SDL_VertexSolid, position)));
data->glVertexAttribPointer(GLES2_ATTRIBUTE_COLOR, 4, GL_FLOAT, GL_TRUE /* Normalized */, stride, (const GLvoid *)(base + offsetof(SDL_VertexSolid, color)));

In SDL2 the color is a GL_UNSIGNED_BYTE, in SDL3 it was changed to GL_FLOAT in 554f062 so if you applied the patch as-is you have to change it back to GL_UNSIGNED_BYTE. I think that's the only affected line that differs between SDL2 and SDL3 but please double check for any similar potential issues.

dschuff added a commit to dschuff/SDL that referenced this pull request May 1, 2025
This fixes undefined behavior resulting from adding offsets to nullptr.
@dschuff
Copy link

dschuff commented May 1, 2025

Thanks! I opened #12937

slouken pushed a commit that referenced this pull request May 2, 2025
This fixes undefined behavior resulting from adding offsets to nullptr.
slouken pushed a commit that referenced this pull request May 2, 2025
This fixes undefined behavior resulting from adding offsets to nullptr.

(cherry picked from commit a220e7c)
GPF pushed a commit to GPF/SDL that referenced this pull request May 26, 2025
This fixes undefined behavior resulting from adding offsets to nullptr.
Wohlstand pushed a commit to Wohlstand/SDL that referenced this pull request May 30, 2025
This fixes undefined behavior resulting from adding offsets to nullptr.
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.

3 participants