Skip to content

Conversation

cgutman
Copy link
Collaborator

@cgutman cgutman commented Apr 5, 2022

Description

libGL.so may register callbacks that can be invoked upon XCloseDisplay(). If XCloseDisplay() is called after libGL.so is unloaded, the callback pointer will point at freed memory and invoking it will crash.

The texture framebuffer check optimized out in f37e4a9 was causing libGL.so to never be unloaded as a side-effect. Skipping it exposed this bug by allowing libGL.so to actually unload.

This should probably be reviewed for correctness by someone more familiar with X11 and/or GLX. #5484 has more detailed analysis.

Existing Issue(s)

Fixes #5484

libGL.so may register callbacks that can be invoked upon XCloseDisplay().
If XCloseDisplay() is called after libGL.so is unloaded, the callback pointer
will point at freed memory and invoking it will crash.

The texture framebuffer check optimized out in f37e4a9 was causing libGL.so to
never be unloaded as a side-effect. Skipping it exposed this bug by allowing
libGL.so to actually unload.
@slouken
Copy link
Collaborator

slouken commented Apr 5, 2022

Can you instead do #if 0 with a comment explaining why we're not doing this cleanup? Otherwise future us will "fix" it by adding the cleanup again.

@cgutman
Copy link
Collaborator Author

cgutman commented Apr 5, 2022

To me, the comment in X11_GL_UnloadLibrary() explains it well. It looked a little odd to see that GL_UnloadObject() there, especially when we just invoked a function called X11_GL_UnloadLibrary() (which you'd assume would take care of that).

I can add a specific comment there though if you want it more explicitly noted that X11_GL_UnloadLibrary() does nothing and it's intended to be that way.

@slouken
Copy link
Collaborator

slouken commented Apr 5, 2022

Ah yes, never mind. :)

@slouken slouken merged commit 45372b1 into libsdl-org:main Apr 5, 2022
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.

XCloseDisplay() calls wild pointer after GLX->EGL transition
2 participants