Closed Bug 1493497 Opened 2 years ago Closed 1 year ago
Crash in gl::Framebuffer::Framebuffer
This bug was filed from the Socorro interface and is report bp-e594a59a-df9f-461f-9690-a6c4f0180922. ============================================================= This crash happened in two different installations in the Windows nightly 20180920100522. Top 10 frames of crashing thread: 0 libglesv2.dll gl::Framebuffer::Framebuffer gfx/angle/checkout/src/libANGLE/Framebuffer.cpp:668 1 libglesv2.dll gl::Context::makeCurrent gfx/angle/checkout/src/libANGLE/Context.cpp:657 2 libglesv2.dll egl::Display::makeCurrent gfx/angle/checkout/src/libANGLE/Display.cpp:815 3 libglesv2.dll egl::Display::destroyContext gfx/angle/checkout/src/libANGLE/Display.cpp:909 4 libglesv2.dll egl::DestroyContext gfx/angle/checkout/src/libGLESv2/entry_points_egl.cpp:388 5 xul.dll void mozilla::gl::GLContextEGL::~GLContextEGL gfx/gl/GLContextProviderEGL.cpp:328 6 xul.dll void mozilla::gl::GLContextEGL::~GLContextEGL gfx/gl/GLContextProviderEGL.cpp:316 7 xul.dll mozilla::layers::CopyableCanvasRenderer::~CopyableCanvasRenderer gfx/layers/CopyableCanvasRenderer.cpp:53 8 xul.dll void mozilla::layers::WebRenderCanvasRendererAsync::~WebRenderCanvasRendererAsync gfx/layers/wr/WebRenderCanvasRenderer.cpp:32 9 xul.dll void mozilla::layers::WebRenderCanvasData::~WebRenderCanvasData gfx/layers/wr/WebRenderUserData.cpp:316 =============================================================
There are 8 crashes (from 3 installations) in nightly 64 starting with buildid 20180920100522. In analyzing the backtrace, the regression may have been introduced by patch  to fix bug 1489279.  https://hg.mozilla.org/mozilla-central/rev?node=7aff94b0bba6
This is spiking in 64.0 release, crash addresses point at UAF.
Jessie, could you please help with that? Thanks
Assignee: nobody → sotaro.ikeda.g
From the crash stack, it seems that egl::Surface was already destroyed. All crashes happened in Display::destroyContext() in GLContextEGL::~GLContextEGL(). Further, Context::makeCurrent() was called in Display::destroyContext(). The makeCurrent() is called only when destroying gl::Context is not current context. https://dxr.mozilla.org/mozilla-central/source/gfx/angle/checkout/src/libANGLE/Display.cpp#880 In Context::makeCurrent(), it seems that thread->getCurrentDrawSurface() is already destroyed surface.
(In reply to Sotaro Ikeda [:sotaro out of office 28/Dec - 3/Jan] from comment #4) > > https://dxr.mozilla.org/mozilla-central/source/gfx/angle/checkout/src/libANGLE/Display.cpp#880 The related code was added by Bug 1489279 (angle-64). https://github.com/google/angle/commit/2dabb4a367c3967f0499e8711251077f44a8d186
I found one problem in SharedSurface_ANGLEShareHandle::~SharedSurface_ANGLEShareHandle(). It destroys EGLSurface just by calling eglDestroySurface(). The ~SharedSurface_ANGLEShareHandle() does not clear the destroying EGLSurface from current context. It could cause UAF. https://dxr.mozilla.org/mozilla-central/source/gfx/gl/SharedSurfaceANGLE.cpp#122
Attachment #9032356 - Flags: review?(jgilbert)
Attachment #9032356 - Flags: review?(jgilbert) → review+
Attachment #9032356 - Attachment description: patch - Clear EGLSurfaceOverride during destroying EGLSurface in SharedSurface_ANGLEShareHandle destructor. → patch - Clear EGLSurfaceOverride during destroying EGLSurface in SharedSurface_ANGLEShareHandle.
Attachment #9032356 - Attachment description: patch - Clear EGLSurfaceOverride during destroying EGLSurface in SharedSurface_ANGLEShareHandle. → patch - Clear EGLSurfaceOverride correctly in SharedSurface_ANGLEShareHandle.
Comment on attachment 9032356 [details] [diff] [review] patch - Clear EGLSurfaceOverride correctly in SharedSurface_ANGLEShareHandle. [Security Approval Request] How easily could an exploit be constructed based on the patch?: It is not easy. I tried to reproduce the problem locally with WebGL. But normal WebGL usages did not hit the crash code path. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No Which older supported branches are affected by this flaw?: firefox64 If not all supported branches, which bug introduced the flaw?: Bug 1489279 Do you have backports for the affected branches?: No If not, how different, hard to create, and risky will they be?: It is easy to back port. How likely is this patch to cause regressions; how much testing does it need?: It is not likely to cause the regression. It is a simple fix to address UAF. Current auto tests already cover the code. And there is no specific STR to hit the problem.
Attachment #9032356 - Flags: sec-approval?
sec-approval+ for trunk. We'll want this on beta as well.
Attachment #9032356 - Flags: sec-approval? → sec-approval+
Let's wait on nominating this for Beta uplift until bug 1516787 has gotten some investigation.
Attachment #9032356 - Flags: approval-mozilla-beta?
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main65+]
You need to log in before you can comment on or make changes to this bug.