crash nsScriptLoader not thread-safe nsScriptLoader.cpp:178

RESOLVED FIXED in Firefox 35

Status

()

defect
RESOLVED FIXED
5 years ago
4 months ago

People

(Reporter: mcmanus, Assigned: khuey)

Tracking

({sec-moderate})

32 Branch
mozilla37
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox34 wontfix, firefox35 fixed, firefox36 fixed, firefox37 fixed, firefox-esr3135+ fixed, b2g-v1.4 wontfix, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [adv-main35+][adv-esr31.4+])

Attachments

(1 attachment, 1 obsolete attachment)

during shutdown .. release off main thread

(gdb) bt
#0  0x00007f710150699d in nanosleep () at ../sysdeps/unix/syscall-template.S:81
#1  0x00007f7101506834 in __sleep (seconds=0) at ../sysdeps/unix/sysv/linux/sleep.c:137
#2  0x00007f7106f000f6 in ah_crap_handler (signum=11) at /home/mcmanus/src/mozilla2/wd/gecko-dev/toolkit/xre/nsSigHandlers.cpp:101
#3  0x00007f7106f001fb in child_ah_crap_handler (signum=11) at /home/mcmanus/src/mozilla2/wd/gecko-dev/toolkit/xre/nsSigHandlers.cpp:113
#4  0x00007f7107da51b9 in AsmJSFaultHandler (signum=11, info=0x7f70f5afc470, context=0x7f70f5afc340) at /home/mcmanus/src/mozilla2/wd/gecko-dev/js/src/asmjs/AsmJSSignalHandlers.cpp:912
#5  <signal handler called>
#6  nsScriptLoader::Release (this=0x7f70f10bc080) at /home/mcmanus/src/mozilla2/wd/gecko-dev/dom/base/nsScriptLoader.cpp:178
#7  0x00007f7104011cdd in nsRefPtr<nsScriptLoader>::~nsRefPtr (this=0x7f70ef090c30) at ../../dist/include/nsRefPtr.h:59
#8  0x00007f7104706e05 in (anonymous namespace)::NotifyOffThreadScriptLoadCompletedRunnable::~NotifyOffThreadScriptLoadCompletedRunnable (this=0x7f70ef090c10) at /home/mcmanus/src/mozilla2/wd/gecko-dev/dom/base/nsScriptLoader.cpp:788
#9  0x00007f7104706e49 in (anonymous namespace)::NotifyOffThreadScriptLoadCompletedRunnable::~NotifyOffThreadScriptLoadCompletedRunnable (this=0x7f70ef090c10) at /home/mcmanus/src/mozilla2/wd/gecko-dev/dom/base/nsScriptLoader.cpp:788
#10 0x00007f71030209e9 in nsRunnable::Release (this=0x7f70ef090c10) at /home/mcmanus/src/mozilla2/wd/gecko-dev/xpcom/glue/nsThreadUtils.cpp:32
#11 0x00007f71046f4944 in nsRefPtr<(anonymous namespace)::NotifyOffThreadScriptLoadCompletedRunnable>::~nsRefPtr (this=0x7f70f5afca38) at ../../dist/include/nsRefPtr.h:59
#12 0x00007f71046f47bd in OffThreadScriptLoaderCallback (aToken=0x7f70eb5c3e60, aCallbackData=0x7f70ef090c10) at /home/mcmanus/src/mozilla2/wd/gecko-dev/dom/base/nsScriptLoader.cpp:847
#13 0x00007f71085d9dd2 in js::HelperThread::handleParseWorkload (this=0x7f70f5c6e4e0) at ../../../gecko-dev/js/src/vm/HelperThreads.cpp:1182
#14 0x00007f71085d8cba in js::HelperThread::threadLoop (this=0x7f70f5c6e4e0) at ../../../gecko-dev/js/src/vm/HelperThreads.cpp:1367
#15 0x00007f71085d71b7 in js::HelperThread::ThreadMain (arg=0x7f70f5c6e4e0) at ../../../gecko-dev/js/src/vm/HelperThreads.cpp:977
#16 0x00007f710b7b298b in _pt_root (arg=0x7f70f9c5e800) at ../../../../../gecko-dev/nsprpub/pr/src/pthreads/ptthread.c:212
#17 0x00007f710b3c30a5 in start_thread (arg=0x7f70f5afd700) at pthread_create.c:309
#18 0x00007f710153f88d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
Er, yes.  This should proxy-release on the main thread, because there's no guarantee about which thread a runnable is destroyed on.
Looks like a standard "runnable that doesn't realize its dtor can run on either thread" bug.
Posted patch Patch (obsolete) — Splinter Review
Untested, but like so.
Assignee: nobody → khuey
Status: NEW → ASSIGNED
Attachment #8536725 - Flags: review?(bzbarsky)
I suspect the reason we haven't seen this before is that processing the script takes a while.  At shutdown we probably skip that.  This could in theory be exploitable.
Group: core-security
Comment on attachment 8536725 [details] [diff] [review]
Patch

Please use forget() or swap()
Attachment #8536725 - Flags: review?(bzbarsky) → review+
Comment on attachment 8536725 [details] [diff] [review]
Patch

What if the runnable never runs?
During shutdown?  Just because the thread gets shut down before it gets a chance to?
More verbosely, NS_DispatchToMainThread works until after xpcom-shutdown-threads ... and I really hope the JS engine is joining threads that could call into Gecko by then ...
I can't check this in because the tree has been closed all day.
Keywords: checkin-needed
Comment on attachment 8539637 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Bug 906371
[User impact if declined]: Possible crashes/security issues
[Describe test coverage new/current, TBPL]: This is the script loading path, it's pretty well tested.
[Risks and why]: Low risk.  This makes us explicitly release things on the thread we already virtually always release them on.
[String/UUID change made/needed]: None.
Attachment #8539637 - Flags: approval-mozilla-beta?
Attachment #8539637 - Flags: approval-mozilla-aurora?
Comment on attachment 8539637 [details] [diff] [review]
Patch

See above.
Attachment #8539637 - Flags: approval-mozilla-esr31?
Attachment #8539637 - Flags: approval-mozilla-b2g34?
Attachment #8539637 - Flags: approval-mozilla-b2g32?
Comment on attachment 8539637 [details] [diff] [review]
Patch

Approving now for Beta since we only have one beta this week and it would be best to have this in the build, out there getting tested by users.
Attachment #8539637 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8539637 [details] [diff] [review]
Patch

also approving for aurora, in irc we discussed and it's getting merged without issue.
Attachment #8539637 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/4fc84f5e110f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Attachment #8539637 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Whiteboard: [adv-main35+][adv-esr31.4+]
Attachment #8539637 - Flags: approval-mozilla-b2g34?
Attachment #8539637 - Flags: approval-mozilla-b2g34+
Attachment #8539637 - Flags: approval-mozilla-b2g32?
Attachment #8539637 - Flags: approval-mozilla-b2g32+
Group: core-security → core-security-release
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.