1493497 - Crash in gl::Framebuffer::Framebuffer

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect, P1)

Description

[Julian Seward [:jseward]]

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

=============================================================

Comment 1

[Calixte Denizet (:calixte)]

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 [1] to fix bug 1489279.

[1] https://hg.mozilla.org/mozilla-central/rev?node=7aff94b0bba6

Comment 2

[Julien Cristau [:jcristau]]

This is spiking in 64.0 release, crash addresses point at UAF.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Jessie, could you please help with that?
Thanks

Comment 4

[Sotaro Ikeda [:sotaro]]

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.

Comment 5

[Sotaro Ikeda [:sotaro]]

(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

Comment 6

[Sotaro Ikeda [:sotaro]]

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

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Clear EGLSurfaceOverride correctly in SharedSurface_ANGLEShareHandle.

Comment 8

[Sotaro Ikeda [:sotaro]]

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.

Comment 9

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk. We'll want this on beta as well.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41d9d168940d

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/41d9d168940d

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Let's wait on nominating this for Beta uplift until bug 1516787 has gotten some investigation.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Bug 1516787 has hardly any crash instances on 66. Why is the uplift to beta on this bug gated on that bug?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Because it wasn't clear what the volume was at the time of the initial report and I didn't want to risk introducing a bigger regression. Anyway, Jeff is back from PTO tomorrow, so hopefully it'll be a moot point and we can get this + the follow-up in b10 on Thursday.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 16

[Sotaro Ikeda [:sotaro]]

Comment on attachment 9032356 [details] [diff] [review]
patch - Clear EGLSurfaceOverride correctly in SharedSurface_ANGLEShareHandle.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1489279

User impact if declined: It might cause the crash.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce:

List of other uplifts needed: Bug 1516787

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Fix is relatively simple, though the patch caused a regression(Bug 1516787). It is already addressed.

String changes made/needed: None

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9032356 [details] [diff] [review]
patch - Clear EGLSurfaceOverride correctly in SharedSurface_ANGLEShareHandle.

[Triage Comment]
Fixes a sec-high WebGL crash. Approved for 65.0b12.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4b5ae6076e03