1573266 - MOZ_CRASH(Mutex ordering violation) when thread creation fails

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The following testcase crashes on mozilla-central revision f719a2df1b96 (build with --enable-debug --enable-address-sanitizer --enable-more-deterministic, run with --fuzzing-safe --ion-offthread-compile=off --ion-eager):

addMarkObservers = function() { };
backtrace = function() { };
clearMarkObservers = function() { };
dumpHeap = function() { };
dumpObject = function() { };
dumpStringRepresentation = function() { };
evalInCooperativeThread = function() { };
evalInWorker = function() { };
getBacktrace = function() { };
getLcovInfo = function() { };
getMarks = function() { };
isAsmJSCompilationAvailable = function() { };
nukeAllCCWs = function() { };
Object.getOwnPropertyNames = function() { };
Object.getOwnPropertyDescriptors = function() { };
offThreadCompileScript = function() { };
oomTest = function() { };
printProfilerEvents = function() { };
saveStack = function() { };
wasmIsSupported = function() { return true; };
// DDBEGIN
function hashStr (s) { /* eslint-disable-line require-jsdoc */   var hash = 0;   var L = s.length;   for (var i = 0; i < L; i++) {     var c = s.charCodeAt(i);     hash = (Math.imul(hash, 31) + c) | 0;   }   return hash; }
function testMathyFunction (f, inputs) { /* eslint-disable-line require-jsdoc */   var results = [];   if (f) {     for (var j = 0; j < inputs.length; ++j) {       for (var k = 0; k < inputs.length; ++k) {         try {           results.push(f(inputs[j], inputs[k]));         } catch (e) {           results.push(errorToString(e));         }       }     }   }   /* Use uneval to distinguish -0, 0, "0", etc. */   /* Use hashStr to shorten the output and keep compare_jit files small. */   print(hashStr(uneval(results))); }
 try { tryRunning = useSpidermonkeyShellSandbox(1); } catch(e) { }

try{print(uneval(this));}catch(e){}
// DDEND

Backtrace:

==22821==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x5565afc729cd bp 0x7fff730f9a30 sp 0x7fff730f9940 T0)
==22821==The signal is caused by a WRITE memory access.
==22821==Hint: address points to the zero page.
    #0 0x5565afc729cc in js::Mutex::lock() js/src/threading/Mutex.cpp:49:7
    #1 0x5565aeed87e4 in js::LockGuard<js::Mutex>::LockGuard(js::Mutex&) js/src/threading/LockGuard.h:22:57
    #2 0x5565aeed87e4 in js::AutoLockHelperThreadState::AutoLockHelperThreadState(mozilla::detail::GuardObjectNotifier&&) js/src/vm/HelperThreads.h:686
    #3 0x5565aeed87e4 in js::HelperThread::destroy() js/src/vm/HelperThreads.cpp:2056
    #4 0x5565aeed7d47 in js::GlobalHelperThreadState::finishThreads() js/src/vm/HelperThreads.cpp:1205:12
/snip

For detailed crash information, see attachment.

Setting s-s because this is a ASan SEGV as a start.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Detailed Crash Information

Comment 2

[Fuzzing Team]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The testcase was not reliable at all, but I filed this first anyway to see if the ASan stack helps. As a result of the non-reproducibility, I cannot get a bisection result.

Paul, you've looked at Mutex-related bugs recently, do you know if the ASan stack makes sense?

Comment 4

[Paul Bone [:pbone]]

I have one vague idea, I'll test it report back.

Comment 5

[Paul Bone [:pbone]]

I initially suspected it was related to something I was working on, but it doesn't look like it.

In the backtrace we can see that it tried to create the helper threads but failed:

https://searchfox.org/mozilla-central/source/js/src/vm/HelperThreads.cpp#1162

It looks like that leaves the mutex stack in an inconsistent state which causes the assertion failure (a mutex ordering violation). It probably has nothing to do with the JS test case or asan (I've removed the testcase and regression flags).

Since the assertion is for mutex ordering it doesn't represent a security problem, at worst the process could have its threads deadlock (the security flag can be removed).

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Is it just an OOM situation manifesting as an ASan crash?

Comment 7

[Paul Bone [:pbone]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #6)

Is it just an OOM situation manifesting as an ASan crash?

Could be. but it might be some other resource the OS may need for threads.

Comment 9

[Paul Bone [:pbone]]

Attachment: Bug 1573266 - Make some corrections in js/src/jsapi-tests/README r=jonco

Comment 10

[Paul Bone [:pbone]]

Attachment: Bug 1573266 - Improve the NewContext OOM test r=jonco

We should end the test once the OOM simulation has found the last allocation
it can instrument, not when the context is created. This exercises 3 extra
allocations (they may be in the DestroyContext).

Also:

• Print a + or a . to standard output to show if a context was
  successfully created or not for each iteration.

• Rename this so all the OOM tests have OOM in their name.

Depends on D46560

Comment 11

[Paul Bone [:pbone]]

Attachment: Bug 1573266 - Test OOM simulation for helper thread state r=jonco

Also make DestroyHelperThreadsState() idempotent so we can call it from the
test regardless of what state the helper thread state is in.

Other changes to HelperThreads.cpp make this code safer when allocations
fail.

Depends on D46561

Comment 12

[Paul Bone [:pbone]]

Attachment: Bug 1573266 - Use OOM simulation for thread creation r=jonco

This is the patch that tests and fixes this bug. If we execute the error
case because the OS refused to create a thread we would create a nested
AutoLockHelperThreadsState and violate mutex orderings. We fix this by
reducing the AutoLockHelperThreadsState scope in ensureInitalized().

Also:

• Use OOM testing to fail thread creation.
• Remove the unsafe-OOM region from Thread::init(), it isn't needed anyway.

Depends on D46562

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69574a70c39f
Make some corrections in js/src/jsapi-tests/README r=jonco
https://hg.mozilla.org/integration/autoland/rev/8c85708d99a1
Improve the NewContext OOM test r=jonco
https://hg.mozilla.org/integration/autoland/rev/cbd0e65784ae
Test OOM simulation for helper thread state r=jonco
https://hg.mozilla.org/integration/autoland/rev/86aa1aeeaa99
Use OOM simulation for thread creation r=jonco

Comment 14

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/69574a70c39f
https://hg.mozilla.org/mozilla-central/rev/8c85708d99a1
https://hg.mozilla.org/mozilla-central/rev/cbd0e65784ae
https://hg.mozilla.org/mozilla-central/rev/86aa1aeeaa99

Comment 15

[Ryan VanderMeulen [:RyanVM]]

I assume this can ride the trains, but please nominate for uplift if you feel strongly otherwise.