890432 - WebGL conformance-test-suite crash with long-expressions-should-not-crash [@glpCGNode]

Status: VERIFIED FIXED

(Core :: Canvas: WebGL, defect, critical)

Description

[Christoph Diehl [:posidron]]

Testcase: https://www.khronos.org/registry/webgl/sdk/tests/conformance/glsl/bugs/long-expressions-should-not-crash.html

Stack:

#0  0x00000001256ee609 in glpCGNode ()
#1  0x00000001256f03ad in glpCGPPStreamOpNode ()
#2  0x00000001256eff8f in glpCGNode ()
#3  0x00000001256f03ad in glpCGPPStreamOpNode ()
#4  0x00000001256eff8f in glpCGNode ()
[...]
#4403 0x00000001256f03ad in glpCGPPStreamOpNode ()
#4404 0x00000001256eff8f in glpCGNode ()
#4405 0x00000001256f03ad in glpCGPPStreamOpNode ()
#4406 0x00000001256eff8f in glpCGNode ()
#4407 0x00000001256f03ad in glpCGPPStreamOpNode ()
[...]

Tested with http://hg.mozilla.org/integration/mozilla-inbound/rev/a9d6d86a8090
Firefox 22 is also affected.

  Chipset Model: NVIDIA GeForce GT 650M
  Type: GPU
  Bus: PCIe
  PCIe Lane Width: x8
  VRAM (Total):	1024 MB
  Vendor: NVIDIA (0x10de)
  Device ID: 0x0fd5
  Revision ID: 0x00a2
  ROM Revision: 3688
  gMux Version: 3.2.19 [3.2.8]


ProductName:	Mac OS X
ProductVersion:	10.8.4
BuildVersion:	12E55

Comment 1

[Jeff Gilbert [:jgilbert]]

Is there anything under all those repeating pairs on the stack?

Comment 2

[Christoph Diehl [:posidron]]

I haven't looked deeper into it and I do not have any symbols for the graphic drivers.

Comment 3

[Daniel Veditz [:dveditz]]

Is there any more information along with the call tree you can get out of gdb? registers, a bit of dissassembly, the reason for the crash?

Comment 4

[Christoph Diehl [:posidron]]

Attachment: callstack.txt

At frame #5467 we finally get some real stack to see. I have attached the stack for those frames.

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Jeff what do you make of the stack in comment 4?

Comment 6

[Jeff Gilbert [:jgilbert]]

(In reply to David Bolter [:davidb] from comment #5)
> Jeff what do you make of the stack in comment 4?

It looks like a driver bug we're hitting. I imagine the newer versions of ANGLE work around such issues.

Comment 7

[Daniel Veditz [:dveditz]]

Jeff: do we have a bug open to update ANGLE that this could depend on? Or do we morph this bug into the "update ANGLE (this time)" bug?

What is our regular process for tracking when we need to update ANGLE?

Comment 8

[Jeff Gilbert [:jgilbert]]

The most recent ANGLE update is bug 883478.

Comment 9

[Daniel Veditz [:dveditz]]

The stack looks like it went into a recursive loop until it ran out of resources. Those generally aren't exploitable unless we see other scary signs.

Comment 10

[Jeff Gilbert [:jgilbert]]

Oops, I was testing Win7.

Comment 11

[Jeff Gilbert [:jgilbert]]

Looks like we hit this even with that ANGLE update.

Comment 12

[Jeff Gilbert [:jgilbert]]

Attachment: limit-mac-nv-complex

This should do it. I'll spin up a Try build and test it locally.

Comment 13

[Jeff Gilbert [:jgilbert]]

Attachment: patch: Limit expressions complexity to 1000 on Mac+NV

Oops, fixed whitespace. The EOF change here is adding a newline at EOF, where there wasn't one before.

Comment 14

[Jeff Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&showall=1&rev=b5ef6527d679

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 806939 [details] [diff] [review]
patch: Limit expressions complexity to 1000 on Mac+NV

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +3021,5 @@
> +        if (gl->WorkAroundDriverBugs()) {
> +#ifdef XP_MACOSX
> +            if (gl->Vendor() == gl::GLContext::VendorNVIDIA) {
> +                // Work around bug 890432
> +                resources.MaxExpressionComplexity = 1000;

Please make this a std::min instead.

@@ +3107,5 @@
>          int compileOptions = SH_ATTRIBUTES_UNIFORMS |
>                               SH_ENFORCE_PACKING_RESTRICTIONS;
>  
> +        if (resources.MaxExpressionComplexity > 0) {
> +            compileOptions |= SH_LIMIT_EXPRESSION_COMPLEXITY;

Is this needed outside of the above case?

If not, please reorganize this code so that it's only done at the same place.

@@ +3119,5 @@
>  
>          if (useShaderSourceTranslation) {
>              compileOptions |= SH_OBJECT_CODE
>                              | SH_MAP_LONG_VARIABLE_NAMES;
> +

Whitespace

Comment 16

[Jeff Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #15)
> Comment on attachment 806939 [details] [diff] [review]
> patch: Limit expressions complexity to 1000 on Mac+NV
> 
> Review of attachment 806939 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContextGL.cpp
> @@ +3021,5 @@
> > +        if (gl->WorkAroundDriverBugs()) {
> > +#ifdef XP_MACOSX
> > +            if (gl->Vendor() == gl::GLContext::VendorNVIDIA) {
> > +                // Work around bug 890432
> > +                resources.MaxExpressionComplexity = 1000;
> 
> Please make this a std::min instead.
std::min doesn't make sense here. The default for this is 0, but it's also disabled by default.
> 
> @@ +3107,5 @@
> >          int compileOptions = SH_ATTRIBUTES_UNIFORMS |
> >                               SH_ENFORCE_PACKING_RESTRICTIONS;
> >  
> > +        if (resources.MaxExpressionComplexity > 0) {
> > +            compileOptions |= SH_LIMIT_EXPRESSION_COMPLEXITY;
> 
> Is this needed outside of the above case?
> 
> If not, please reorganize this code so that it's only done at the same place.
I think it's much better here. We always want to activate SH_LIMIT_EXPR_COMPL whenever we have a value set for MaxExprCompl. We could set it in the same place, but this would require me to hoist this code about 80 lines further up, leaving us with a mysterious 'compileOptions' var that isn't used for a while yet.

I think it's better to do it this way, such that we activate it if we use it. When other cases come up where we might want to limit complexity, we don't have to do anything here: We just need to set MaxExprCompl to something non-zero. (non-default)
> 
> @@ +3119,5 @@
> >  
> >          if (useShaderSourceTranslation) {
> >              compileOptions |= SH_OBJECT_CODE
> >                              | SH_MAP_LONG_VARIABLE_NAMES;
> > +
> 
> Whitespace
I think I'm going to put my editor back in kill-all-whitespace mode, and deal with the side-effects.

Comment 17

[Jeff Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1430526cf6e5

I've also verified that this is fixed on the machine I could repro this on.

Comment 18

[Jeff Gilbert [:jgilbert]]

Comment on attachment 806939 [details] [diff] [review]
patch: Limit expressions complexity to 1000 on Mac+NV

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: Leaves users of Macs with NVidia GPUs vulnerable to DOS.
Testing completed (on m-c, etc.): On inbound.
Risk to taking this patch (and alternatives if risky): very low
String or IDL/UUID changes made by this patch: none

This is probably not worth uplifting to beta. (It might not even be possible, if our older version of ANGLE doesn't support this)

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1430526cf6e5

Comment 20

[Alex Keybl [:akeybl]]

Doesn't need to track as long as this is a DOS. Will leave to approval triage on whether or not to uplift.

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 806939 [details] [diff] [review]
patch: Limit expressions complexity to 1000 on Mac+NV

Low risk, can take on Aurora.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2f3a72f707c9

(In reply to Jeff Gilbert [:jgilbert] from comment #17)

BTW, TBPL links will go stale after 30 days, so please post links to hg.mozilla.org when pushing to inbound :)

Comment 23

[Jeff Gilbert [:jgilbert]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #22)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/2f3a72f707c9
> 
> (In reply to Jeff Gilbert [:jgilbert] from comment #17)
> 
> BTW, TBPL links will go stale after 30 days, so please post links to
> hg.mozilla.org when pushing to inbound :)

Alright, and thanks!

Comment 24

[Tracy Walker [:tracy]]

oops, I shouldn't have marked this secure bug with [good first verify]. New testers won't have privs to see it.  

The test case doesn't crash latest Nightly nor Aurora builds