1770514 - Use-after-free: gbm_device_destroy called before gbm_surface_destroy

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

STR: with a VA-API accelerated video playing in the only tab, close that tab. This probably needs a build type with NS_FREE_PERMANENT_DATA — debug, sanitizer, coverage, etc. — so that the RDD process isn't killed before this point in shutdown.

There's a crash in this call to gbm_surface_destroy (from a static destructor):

#0  gbm_surface_destroy (surf=0x7efd42391610) at ../src/gbm/main/gbm.c:718
#1  0x00007efd68102984 in mozilla::widget::nsGbmLib::DestroySurface(gbm_surface*) (surface=0x7efd42391610)
    at …/dist/include/mozilla/widget/DMABufLibWrapper.h:146
#2  0x00007efd680f54da in mozilla::gl::SavedGLSurface::~SavedGLSurface() (this=this@entry=0x7efd3fbfe4c0) at …/gfx/gl/GLContextProviderEGL.cpp:822
#3  0x00007efd680f275e in mozilla::gl::DeleteSavedGLSurface(void*) (surface=surface@entry=0x7efd3fbfe760) at …/gfx/gl/GLContextProviderEGL.cpp:115
#4  0x00007efd680f409f in mozilla::gl::DestroySurface(mozilla::gl::EglDisplay&, void*) (egl=<optimized out>, oldSurface=0x7efd42391610) at …/gfx/gl/GLContextProviderEGL.cpp:160
#5  0x00007efd680f46b1 in mozilla::gl::GLContextEGL::~GLContextEGL() (this=0x7efd40f79400) at …/gfx/gl/GLContextProviderEGL.cpp:396
#6  0x00007efd680f4705 in mozilla::gl::GLContextEGL::~GLContextEGL() (this=0x7efd42391610) at …/gfx/gl/GLContextProviderEGL.cpp:381
#7  0x00007efd680e30aa in mozilla::detail::GenericRefCounted<(mozilla::detail::RefCountAtomicity)0>::Release() (this=0x7efd40f79400)
    at …/dist/include/mozilla/GenericRefCounted.h:83
#8  0x00007efd72255f77 in __run_exit_handlers (status=0, listp=0x7efd723e9738 <__exit_funcs>, run_list_atexit=run_list_atexit@entry=true, run_dtors=run_dtors@entry=true) at exit.c:108
#9  0x00007efd7225611a in __GI_exit (status=<optimized out>) at exit.c:139
#10 0x00007efd7223e804 in __libc_start_main (main=
    0x556a1e297b55 <main(int, char**, char**)>, argc=13, argv=0x7ffeeafca018, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7ffeeafca008) at ../csu/libc-start.c:366
#11 0x0000556a1e2979f9 in _start ()

The problem is that the gbm_surface has a pointer to its gbm_device, which has previously been freed, here (from another static destructor, probably for dmaBufDevice in GetDMABufDevice):

#0  gbm_device_destroy (gbm=0x7efd4228ca90) at ../src/gbm/main/gbm.c:109
#1  0x00007efd6a93a66e in mozilla::widget::nsGbmLib::DestroyDevice(gbm_device*) (gdm=0x7efd4228ca90) at …/widget/gtk/DMABufLibWrapper.h:70
#2  0x00007efd6a91fe22 in mozilla::widget::nsDMABufDevice::~nsDMABufDevice() (this=0x7efd6f2935b8 <mozilla::widget::GetDMABufDevice()::dmaBufDevice>)
    at …/widget/gtk/DMABufLibWrapper.cpp:217
#3  0x00007efd72255f77 in __run_exit_handlers (status=0, listp=0x7efd723e9738 <__exit_funcs>, run_list_atexit=run_list_atexit@entry=true, run_dtors=run_dtors@entry=true) at exit.c:108
#4  0x00007efd7225611a in __GI_exit (status=<optimized out>) at exit.c:139
#5  0x00007efd7223e804 in __libc_start_main (main=
    0x556a1e297b55 <main(int, char**, char**)>, argc=13, argv=0x7ffeeafca018, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7ffeeafca008) at ../csu/libc-start.c:366
#6  0x0000556a1e2979f9 in _start ()

The gbm_device has a table of function pointers for operations, including surface destruction, so libgbm reads a function pointer from freed memory and jumps to it.

(Security note: this requires a feature that's preffed off, although a number of users have turned the pref on for testing, and currently it also requires turning off the sandbox (see bug 1769499 comment #11), and it's not clear if it affects release builds in any case (see the note about build types at the topic of this bug). So, the severity probably isn't very high.)

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1769499

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

:stransky, since you are the author of the regressor, bug 1769499, could you take a look?
For more information, please visit auto_nag documentation.

Comment 3

[Martin Stránský [:stransky] (ni? me)]

Sure, will look at it.

Comment 4

[Martin Stránský [:stransky] (ni? me)]

Will be fixed by Bug 1770407 - we'll replace GBM backend with surfaceless one.

Comment 5

[Darkspirit]

Can bug 1769716 (slightly reformatted by bug 1771146) be backed out now as cleanup?

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1769499

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I think this was fixed in bug 1770407 — it removed the call to nsGbmLib::DestroySurface seen here. (Also, it looks like that was its only call site.)