WebGL crash [@mozilla::WebGLContext::Clear] during conformance test

RESOLVED FIXED in Firefox 18

Status

()

defect
--
critical
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: posidron, Assigned: milan)

Tracking

(Blocks 1 bug, {sec-critical, testcase})

Trunk
mozilla20
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox17 affected, firefox18+ verified, firefox19+ verified, firefox20+ verified, firefox-esr10 unaffected, firefox-esr1718+ verified, b2g18 fixed)

Details

(Whiteboard: [adv-main18+][adv-esr17+], )

Attachments

(6 attachments, 2 obsolete attachments)

Posted file callstack
https://www.khronos.org/registry/webgl/sdk/tests/conformance/extensions/webgl-depth-texture.html

Device ID: 0x fd5
GPU Accelerated Windows: 1/1 OpenGL
Vendor ID: 0x10de
WebGL Renderer: NVIDIA Corporation -- NVIDIA GeForce GT 650M OpenGL Engine
Keywords: testcase
What OS and/or driver version?
Also, comment 0 says it's webgl-depth-texture.html but the callstack attachment has a different URL at the top. Which is it?
ProductName:	Mac OS X
ProductVersion:	10.8.2
BuildVersion:	12C3006

It's webgl-depth-texture.html
Driver Version: NVIDIA-8.6.22
Assigning to Milan for now, maybe bjacob will steal it though :)
Assignee: nobody → milan
Summary: WebGL crash [@mozilla::WebGLContext::Clear] → WebGL crash [@mozilla::WebGLContext::Clear] during conformance test
Safari on this test:
FAIL Unable to fetch WebGL rendering context for Canvas
FAIL successfullyParsed should be true. Threw exception ReferenceError: Can't find variable: successfullyParsed
In Safari, WebGL isn't enabled by default. You have to go to the developer menu to enable it.

It would be very interesting to know if it reproduces there.

Another thing to do is APItrace it.
Right - with WebGL enabled, it admits it doesn't support that extension:
PASS WebGL context exists
Testing binding enum with extension disabled
PASS gl.texImage2D(gl.TEXTURE_2D, 0, gl.DEPTH_COMPONENT, 1, 1, 0, gl.DEPTH_COMPONENT, gl.UNSIGNED_SHORT, null) generated expected GL error: INVALID_ENUM.
PASS gl.texImage2D(gl.TEXTURE_2D, 0, gl.DEPTH_COMPONENT, 1, 1, 0, gl.DEPTH_COMPONENT, gl.UNSIGNED_INT, null) generated expected GL error: INVALID_ENUM.
PASS No WEBGL_depth_texture support -- this is legal
PASS WEBGL_depth_texture not listed as supported and getExtension failed -- this is legal
PASS successfullyParsed is true
TEST COMPLETE
Do we know if this is crash happens outside of OSX?
Keywords: qawanted
Whiteboard: [qawanted - what platforms crash]
And, on OSX, is it just Nvidia graphics?
Benoit/Jeff, why is gl->IsExtensionSupported(GLContext::EXT_packed_depth_stencil) and indication that WEBGL_depth_texture is supported?  Is there a better test, before we start special casing OSX (and Nvidia?)
Option 1, suggested by bjacob
Attachment #692433 - Flags: feedback?(jgilbert)
Attachment #692433 - Flags: feedback?(bjacob)
Is this the right place instead?
Attachment #692434 - Flags: feedback?(jgilbert)
Attachment #692434 - Flags: feedback?(bjacob)
(In reply to Milan Sreckovic from comment #11)
> Benoit/Jeff, why is
> gl->IsExtensionSupported(GLContext::EXT_packed_depth_stencil) and indication
> that WEBGL_depth_texture is supported?  Is there a better test, before we
> start special casing OSX (and Nvidia?)

Hm, so the test we have here is

            if (gl->IsGLES2() && 
                gl->IsExtensionSupported(GLContext::OES_packed_depth_stencil) &&
                gl->IsExtensionSupported(GLContext::OES_depth_texture)) 

So it is testing for OES_depth_texture as it should, but you're right, it also tests for OES_packed_depth_stencil and I have no idea why it does that. Jeff reviewed that patch (bug 738866) so he should know.

Anyhow, this is only adding a _restriction_ on the condition to enable this extension, so it can't be responsible for enabling it when it shouldn't.
Comment on attachment 692433 [details] [diff] [review]
Blacklist OSX & Nvidia in WebGLContext::IsExtensionSupported

Review of attachment 692433 [details] [diff] [review]:
-----------------------------------------------------------------

That is the right place.
Attachment #692433 - Flags: feedback?(jgilbert)
Attachment #692433 - Flags: feedback?(bjacob)
Attachment #692433 - Flags: feedback+
Comment on attachment 692434 [details] [diff] [review]
Blacklist OSX & Nvidia in WebGLContext::InitAndValidateGL

Review of attachment 692434 [details] [diff] [review]:
-----------------------------------------------------------------

The interesting --- I didn't know that we had MarkExtensionUnsupported. As said above, the extension to disable here would be the depth_texture extension since we do check for it too; but if we were to take that route, I'd rather do it directly in gfx/gl/GLContext.* since that isn't WebGL specific.
Attachment #692434 - Flags: feedback?(jgilbert)
Attachment #692434 - Flags: feedback?(bjacob)
Attachment #692434 - Flags: feedback-
I meant: "Ah, Interesting!"
Wait a minute, what tree does your patch apply to ? In all the trees I looked at, the code looks like in comment 14, with the gl->IsExtensionSupported(GLContext::OES_depth_texture).
Comment on attachment 692433 [details] [diff] [review]
Blacklist OSX & Nvidia in WebGLContext::IsExtensionSupported

Sorry -- I was confused above, was looking at the wrong if().

Jeff, please comment on the rationale for this EXT_packed_depth_stencil test?
Attachment #692433 - Flags: feedback?(jgilbert)
(In reply to Milan Sreckovic from comment #10)
> And, on OSX, is it just Nvidia graphics?

Tested on a Mac with AMD graphics, works fine. Crash is NVIDIA specific.
Removing qawanted, bjacob confirmed it runs on AMD.
Keywords: qawanted
Whiteboard: [qawanted - what platforms crash]
(In reply to Benoit Jacob [:bjacob] from comment #19)
> Comment on attachment 692433 [details] [diff] [review]
> Blacklist OSX & Nvidia in WebGLContext::IsExtensionSupported
> 
> Sorry -- I was confused above, was looking at the wrong if().
> 
> Jeff, please comment on the rationale for this EXT_packed_depth_stencil test?

WEBGL_depth_texture requires both depth and depth-stencil textures.
That said, WebGL actually requires depth-stencil in the core spec, so technically we need to blocklist WebGL on systems where this is not supported, until we can emulate it.

This means that while we shouldn't check for {OES,EXT}_packed_depth_stencil, it's not because we don't need it, but because this functionality is required already by WebGL.
Comment on attachment 692433 [details] [diff] [review]
Blacklist OSX & Nvidia in WebGLContext::IsExtensionSupported

Review of attachment 692433 [details] [diff] [review]:
-----------------------------------------------------------------

This should be done at the end of GLContext::InitExtensions.
Attachment #692433 - Flags: feedback?(jgilbert) → feedback-
Option 3, suggested by jgilbert
Attachment #692433 - Attachment is obsolete: true
Attachment #692434 - Attachment is obsolete: true
Attachment #693020 - Flags: review?(jgilbert)
Attachment #693020 - Flags: review?(bjacob)
Comment on attachment 693020 [details] [diff] [review]
Blacklist OSX & Nvidia in GLContext::InitExtensions

Review of attachment 693020 [details] [diff] [review]:
-----------------------------------------------------------------

Note: as I have never used MarkExtensionUnsupported, I suggest that you run this locally to verify that it does take effect.

::: gfx/gl/GLContext.cpp
@@ +599,5 @@
> +    // to properly support this.  See 814839
> +    if (WorkAroundDriverBugs() &&
> +        Vendor() == gl::GLContext::VendorNVIDIA) {
> +        MarkExtensionUnsupported(gl::GLContext::EXT_packed_depth_stencil);
> +    }

Just a style nit: after a multi-line if() condition, the opening curly brace '{' should be on a new line.
Attachment #693020 - Flags: review?(bjacob) → review+
Attachment #693020 - Flags: review?(jgilbert) → review+
Applying nit comments to the r+ patch.
Attachment #693086 - Flags: review+
Attachment #693086 - Flags: checkin?
Comment on attachment 693086 [details] [diff] [review]
Updated with code review comments (trivial)

[Security approval request comment]
How easily can the security issue be deduced from the patch?

Nothing specific, aside from "there is a security problem with depth textures on Mac NVIDIA", can be deduced from the patch, as this patch just disables this functionality on this driver. I think it's safe to land now.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No, they just show that there exists a security issue with depth textures on this driver.

Which older supported branches are affected by this flaw?

All branches >= 17 when bug 738866 landed, exposing this functionality.

If not all supported branches, which bug introduced the flaw?

Bug 738866.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Should be simple enough to backport, not necessarily trivial if MarkExtensionUnsupported wasn't available yet in some branch, but still simple.

How likely is this patch to cause regressions; how much testing does it need?

Safe, won't cause regressions. Only testing it needs is verify that it does disable depth textures on Mac NVIDIA.
Attachment #693086 - Flags: sec-approval?
Adding release management so we can discuss when we want to get this in since it affects 17 and later.
As long as there won't be a major perf hit, we're good to land on all branches given the low risk of regression. Please prepare an ESR17 patch as well.
I'll put together the backports.
In comments 28 and 29 did you mean to grant sec-approval? Or should we wait a bit longer before landing on central? (My recommendation is not to wait as this fix is not giving away a precise vulnerability).
Comment on attachment 693086 [details] [diff] [review]
Updated with code review comments (trivial)

Giving explicit sec-approval+.
Attachment #693086 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/d5f731147e6c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [adv-main18+][adv-esr17+]
Attachment #693086 - Flags: checkin?
Confirmed crash on 17.0.1 ESR, confirmed fix on 17.0.2 ESR.
Confirmed crash on trunk, 2012-11-24
Confirmed fixed on trunk, 2013-01-09
Confirmed fixed on Aurora, 2013-01-09
Confirmed fixed on beta, 2013-01-09
Group: core-security
Blocks: 907946
Blocks: 685184
Blocks: 908905
This does not reproduce for me on 10.8.3 with a GeForce 9400M.
Also, I believe this fix was over-cautious, and we should have only blocked depth_texture on the affected machines. I filed bug 908905 for this.
You need to log in before you can comment on or make changes to this bug.