Check duplicate native surfaces #155
Conversation
In the error cleanup path in wlEglCreatePlatformWindowSurfaceHook, don't try to dereference the WlEglSurface if we never allocated it.
| if (wsurf == NULL) { | ||
| err = EGL_BAD_ALLOC; | ||
| goto fail; | ||
| } |
There was a problem hiding this comment.
Here we store our wl_surface in wsurf so we don't prematurely set it, but doesn't this leak the surface if we jump to fail later on (like in line 2708? Before this change I think this would get cleaned up as part of wlEglDestroySurface but now this isn't recorded in surface until later?
I think we can go ahead and set surface->wlSurface here? Afaict that shouldn't affect anything between here and 2779 and properly handles the surface cleanup.
There was a problem hiding this comment.
getWlEglWindowVersionAndSurface doesn't allocate anything, it just figures out the correct offset for the wl_surface pointer in the wl_egl_surface struct.
That said, using the same surface variable in the loop would cause problems in the failure path, because it'll try to clean up an existing WlEglSurface.
There was a problem hiding this comment.
Ah good point, thanks that looks better now.
In wlEglCreatePlatformWindowSurfaceHook, check if there's already a EGLSurface that uses the same wl_surface object, and if so, fail with EGL_BAD_ALLOC. We've got a check (using the wl_egl_window::driver_private pointer) to catch if the app tries to create multiple EGLSurfaces from the same wl_egl_window. But, an app could still call wl_egl_window_create multiple times, which would give it multiple wl_egl_window structs for the same wl_surface.
kbrenneman
force-pushed
the
check-duplicate-native-surface
branch
from
5e7652c
to
83ea2aa
Compare
| if (wsurf == NULL) { | ||
| err = EGL_BAD_ALLOC; | ||
| goto fail; | ||
| } |
There was a problem hiding this comment.
Ah good point, thanks that looks better now.
By the EGL spec, you're not allowed to create more than one EGLSurface from the same native window/pixmap. Currently, egl-wayland only partially checks for that, but you can still cause problems in a slightly more roundabout way.
While egl-wayland does check if the app tries to create a second EGLSurface from the same
wl_egl_window, it's still possible to create more than one EGLSurface from the samewl_surface. Sincewl_egl_window_createitself doesn't check for duplicates, you can create more than onewl_egl_windowfrom the samewl_surface, and then create an EGLSurface for each.With this change, egl-wayland will scan the list of EGLSurfaces to check if any of them use the same underlying
wl_surface, and if it finds one, it'll fail eglCreateWindowSurface with EGL_BAD_ALLOC.I also fixed a segfault that it would run into in early-failure cases like this one, where it hasn't allocated the
WlEglSurfaceyet, but the cleanup code tries to dereference it.