648705 - Possible security sensitive crash [@ mozilla::WebGLProgram::DetachShaders() ]

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Bas Schouten (:bas.schouten)]

When playing with possible infinite loop shaders I hit a crash that appears like it might be dangerous:

Crash Reason	EXCEPTION_ACCESS_VIOLATION_WRITE
Crash Address	0x253e70fc

0 	xul.dll 	mozilla::WebGLProgram::DetachShaders 	content/canvas/src/WebGLContext.h:1313
1 	xul.dll 	mozilla::WebGLContext::UseProgram 	content/canvas/src/WebGLContextGL.cpp:3573
2 	xul.dll 	nsIDOMWebGLRenderingContext_UseProgram 	obj-firefox/js/src/xpconnect/src/dom_quickstubs.cpp:31987
3 		@0x47ef8b 	
4 	mozjs.dll 	CheckStackAndEnterMethodJIT 	js/src/methodjit/MethodJIT.cpp:778
5 	mozjs.dll 	js::mjit::JaegerShot 	js/src/methodjit/MethodJIT.cpp:802
6 	mozjs.dll 	js::RunScript 	js/src/jsinterp.cpp:646
7 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:740
8 	mozjs.dll 	js::ExternalInvoke 	js/src/jsinterp.cpp:863
9 	mozjs.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:5173
10 	xul.dll 	nsJSContext::CallEventHandler 	dom/base/nsJSEnvironment.cpp:1914
11 	xul.dll 	nsGlobalWindow::RunTimeout 	dom/base/nsGlobalWindow.cpp:9107
12 	xul.dll 	nsGlobalWindow::TimerCallback 	dom/base/nsGlobalWindow.cpp:9452
13 	xul.dll 	nsTimerImpl::Fire 	xpcom/threads/nsTimerImpl.cpp:425
14 	xul.dll 	nsTimerEvent::Run 	xpcom/threads/nsTimerImpl.cpp:517
15 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:633
16 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:134
17 	xul.dll 	xul.dll@0xb367c7 	
18 	xul.dll 	MessageLoop::RunInternal 	ipc/chromium/src/base/message_loop.cc:219
19 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:202
20 	mozcrt19.dll 	_VEC_memzero 	
21 	xul.dll 	xul.dll@0x35d7cd 	
22 	firefox.exe 	firefox.exe@0x1bb7 	
23 	ntdll.dll 	WinSqmSetIfMaxDWORD 	
24 	ntdll.dll 	_RtlUserThreadStart 	
25 	firefox.exe 	firefox.exe@0x186f 	
26 	firefox.exe 	firefox.exe@0x186f 

1311 void DetachShaders() {
1312   for (PRUint32 i = 0; i < mAttachedShaders.Length(); ++i) {
1313     mAttachedShaders[i]->DecrementAttachCount();
1314   }
1315   mAttachedShaders.Clear();
1316 }

Comment 1

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

I can see this bug, but can't see any of the comments. Do I need more permissions?

Comment 2

[Daniel Veditz [:dveditz]]

No, we just need to un-double-hide the initial description: done.

Comment 3

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

Bas, when you can reproduce this bug, can you print the values of:
  i
  mAttachedShaders.Length()

Also, a test-case would greatly help.

Comment 4

[Bas Schouten (:bas.schouten)]

(In reply to comment #3)
> Bas, when you can reproduce this bug, can you print the values of:
>   i
>   mAttachedShaders.Length()
> 
> Also, a test-case would greatly help.

What I was doing was playing around with for (float i = 0.0; i < a; i += 1.0);

Where I was varying the output colors and 'a' between 10 and 100000.

On the ShaderToy site.

Comment 5

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

@ Christoph: could you please try to find a test-case for this, based on the description given in comment 4?

Comment 6

[Christoph Diehl [:posidron]]

Of course.

Comment 7

[Joe Drew (not getting mail)]

Over to Christoph to get a testcase.

Comment 8

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

The crash location

    void DetachShaders() {
        for (PRUint32 i = 0; i < mAttachedShaders.Length(); ++i) {
            mAttachedShaders[i]->DecrementAttachCount();
        }
        mAttachedShaders.Clear();
    }

and the fact that it's an invalid write at a random address means that the mAttachedShaders[i] pointer is stale.

This is a plain pointer, not a refptr, so one can be tempted to replace that by a refptr, but I don't understand how this pointer can have gone stale, as the WebGLContext keeps refptrs to all the shaders. As long as the WebGLContext is live, all shaders should be, and therefore pointers to them should stay valid.

Comment 9

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

Ah, WebGLContext::DeleteShader() causes the reference count to be decremented, so it's conceivable that calling UseProgram() after DeleteShader() would crash in this way. For this reason I think that we should let WebGLProgram store refptrs instead of plain ptrs to the attached shaders.

Comment 10

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

Some people have written to us about their intention to release a blog post about a WebGL security flaw we allegedly have on Monday, without giving us any usable information to start fixing it.

In anticipation of that, I will implement comment 9.

Comment 11

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

Attachment: fix dangling pointers by making them nsRefPtrs

This should fix it --- although I can't be sure, as I have yet to find a test case for this bug.

I tried first using WebGLObjectRefPtr, but got assertions (pasted below) so used nsRefPtr instead, as that's really just what I want:

#0  0x00007fe5c58db78d in nanosleep () at ../sysdeps/unix/syscall-template.S:82
#1  0x00007fe5c58db600 in __sleep (seconds=<value optimized out>)
    at ../sysdeps/unix/sysv/linux/sleep.c:138
#2  0x00007fe5c7713e40 in ah_crap_handler (signum=11)
    at /home/bjacob/mozilla-central/toolkit/xre/nsSigHandlers.cpp:119
#3  0x00007fe5c77193e7 in nsProfileLock::FatalSignalHandler (signo=11, info=0x7fff8c63c2f0, 
    context=0x7fff8c63c1c0) at /home/bjacob/build/firefox/toolkit/profile/nsProfileLock.cpp:226
#4  <signal handler called>
#5  0x00007fe5c7e6096b in mozilla::WebGLObjectBaseRefPtr::Zero (this=0x282bb30)
    at /home/bjacob/mozilla-central/content/canvas/src/WebGLContext.h:121
#6  0x00007fe5c7e60c35 in mozilla::WebGLZeroingObject::ZeroOwners (this=0x2adaed8)
    at /home/bjacob/mozilla-central/content/canvas/src/WebGLContext.h:605
#7  0x00007fe5c7e60db5 in mozilla::WebGLShader::Delete (this=0x2adaed0)
    at /home/bjacob/mozilla-central/content/canvas/src/WebGLContext.h:1236
#8  0x00007fe5c7e6826e in mozilla::WebGLContext::DeleteShader (this=0x7fe5985185c0, 
    sobj=0x2adaed0) at /home/bjacob/mozilla-central/content/canvas/src/WebGLContextGL.cpp:1037
#9  0x00007fe5c87149ed in nsIDOMWebGLRenderingContext_DeleteShader (cx=0x2b5cd20, argc=1, 
    vp=0x7fe5b56ed248) at /home/bjacob/build/firefox/js/src/xpconnect/src/dom_quickstubs.cpp:29672
#10 0x00007fe5c92ed564 in js::CallJSNative (cx=0x2b5cd20, 
    native=0x7fe5c871489b <nsIDOMWebGLRenderingContext_DeleteShader>, argc=1, vp=0x7fe5b56ed248)
    at /home/bjacob/mozilla-central/js/src/jscntxtinlines.h:277
#11 0x00007fe5c954a972 in js::Interpret (cx=0x2b5cd20, entryFrame=0x7fe5b56ed048, 
    inlineCallCount=2, interpMode=js::JSINTERP_NORMAL)
    at /home/bjacob/mozilla-central/js/src/jsinterp.cpp:4664

Comment 12

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

http://hg.mozilla.org/mozilla-central/rev/f65a3762c7de

With that in, I really don't think that this bug could still happen; but without anyone being able to reproduce this bug, I don't know if/how I can close it.

Should this be a WORKSFORME?

Comment 13

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

OK, I'm feeling confident enough to mark this fixed. Reopen if needed.

Comment 14

[Asa Dotzler [:asa]]

Does this impact Firefox 5 (aurora)? Should we consider taking the fix there?

Comment 15

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

I would say that's a good idea. Also in Firefox 4 if there will be another release of it.

Just wait until TBPL shows a green mochitest-1, to be sure.

Comment 16

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

To be clear: yes, this impacts Firefox 4 and 5 in exactly the same way.

Comment 17

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

It's green on TBPL. Recommend backporting this to Firefox 4 and 5. Changeset link:
  http://hg.mozilla.org/mozilla-central/rev/f65a3762c7de

Comment 18

[JP Rosevear [:jpr]]

Please land for Aurora by Monday May 16 or the approval will potentially be lost.  Please mark as status-firefox5 fixed when you do.

Comment 19

[LegNeato]

Please land this on releases/mozilla-aurora TODAY or approval will be rescinded.

Comment 20

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

Landed on mozilla-aurora:
http://hg.mozilla.org/mozilla-aurora/rev/f8b08657dfea

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 530639 [details] [diff] [review]
fix dangling pointers by making them nsRefPtrs

Approved for the mozilla2.0 repository, a=dveditz for release-drivers if Cameron needs this for TenFourFox (although that may not support WebGL).

When landed please add a link to the changeset and flip the status2.0 field to ".x-fixed". If you don't need this on the 2.0 branch please unset the approval requests so we don't have an unfixed approval to track in bug triage.

Comment 22

[Cameron Kaiser [:spectre]]

It doesn't, but I certainly wouldn't mind it landed just in case. Thanks very much!