794025 - AddressSanitizer heap-use-after-free in [@ js::mjit::CallCompiler::generateNativeStub]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase triggers an ASan error on mozilla-central revision e327e66a027d (no options required):


function getterFunction(v) { return "getter"; }
Object.defineProperty(Array.prototype, 1,{ 
  get: getterFunction, 
});
var N = (10000);
repeat_str("try { f(); } finally {\n", N),
repeat_str("}", ("" ));
function repeat_str(str, repeat_count) {
  var arr = new Array(--repeat_count);
  while (repeat_count != 0)
    arr[--repeat_count] = str;
  return str.concat.apply(str, arr);
}


ASan trace:

==24366== ERROR: AddressSanitizer heap-use-after-free on address 0x7f19004f036c at pc 0xc1a8fb bp 0x7fff512c8450 sp 0x7fff512c8448
READ of size 8 at 0x7f19004f036c thread T0
    #0 0xc1a8fa in js::mjit::CallCompiler::generateNativeStub() js/src/methodjit/MonoIC.cpp:1082
    #1 0xc191a2 in js::mjit::ic::NativeCall(js::VMFrame&, js::mjit::ic::CallICInfo*) js/src/methodjit/MonoIC.cpp:1308
    #2 0xa6da22 in throwpoline_exit js/src/methodjit/MethodJIT.cpp:0
    #3 0xa6e249 in js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, JS::Value*, bool) js/src/methodjit/MethodJIT.cpp:1045
    #4 0xa6ed7b in JSContext::fp() const js/src/methodjit/MethodJIT.cpp:1103
    #5 0x5d9873 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) js/src/jsinterp.cpp:1487
    #6 0x5bbb03 in js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) js/src/jsinterp.cpp:324
    #7 0x5e8fbf in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) js/src/jsinterp.cpp:509
    #8 0x5e9519 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/jsinterp.cpp:546
    #9 0x46d6f3 in JS_ExecuteScript js/src/jsapi.cpp:5632
    #10 0x41f5b4 in Process(JSContext*, JSObject*, char const*, bool) js/src/shell/js.cpp:439
    #11 0x41d3ed in ProcessArgs(JSContext*, JSObject*, js::cli::OptionParser*) js/src/shell/js.cpp:4700
    #12 0x41e309 in main js/src/shell/js.cpp:4944
    #13 0x7f1906bafcdc in __libc_start_main ??:0
0x7f19004f036c is located 748 bytes inside of 1028-byte region [0x7f19004f0080,0x7f19004f0484)
freed by thread T0 here:
    #0 0xf56840 in __interceptor_free ??:0
    #1 0xa72f57 in js_free ./dist/include/js/Utility.h:170
previously allocated by thread T0 here:
    #0 0xf56971 in __interceptor_calloc ??:0
    #1 0xaa2ed9 in js_calloc ./dist/include/js/Utility.h:159
    #2 0xa79e92 in js::mjit::Compiler::performCompilation() js/src/methodjit/Compiler.cpp:564
    #3 0xa78cb5 in js::mjit::Compiler::compile() js/src/methodjit/Compiler.cpp:140
    #4 0x5d8876 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) js/src/jsinterp.cpp:1480
    #5 0x5bbb03 in js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) js/src/jsinterp.cpp:324
    #6 0x5e8fbf in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) js/src/jsinterp.cpp:509
    #7 0x5e9519 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/jsinterp.cpp:546
    #8 0x46d6f3 in JS_ExecuteScript js/src/jsapi.cpp:5632
    #9 0x41f5b4 in Process(JSContext*, JSObject*, char const*, bool) js/src/shell/js.cpp:439
    #10 0x41d3ed in ProcessArgs(JSContext*, JSObject*, js::cli::OptionParser*) js/src/shell/js.cpp:4700
    #11 0x41e309 in main js/src/shell/js.cpp:4944
    #12 0x7f1906bafcdc in __libc_start_main ??:0


The issue is also visible in Valgrind which might be easier for debugging purposes. Assuming sec-critical because it seems that we're using a JITChunk (compiled part of a script) after it has been free'd.

Comment 1

[Sean Stangl [:sstangl]]

Attachment: Properly detect IC purging.

ic::SplatApplyArgs() can cause a GC, but the RecompilationMonitor was only instantiated after that function was called, so the GC was undetected and we referenced an already-freed IC.

Comment 2

[Sean Stangl [:sstangl]]

Comment on attachment 664659 [details] [diff] [review]
Properly detect IC purging.

[Security approval request comment]
How easily can the security issue be deduced from the patch?

Very easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Yes.

Which older supported branches are affected by this flaw?

All

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Patch is the same for all branches.

How likely is this patch to cause regressions; how much testing does it need?

Can't cause regressions. No testing is needed.

Comment 3

[Daniel Veditz [:dveditz]]

Comment on attachment 664659 [details] [diff] [review]
Properly detect IC purging.

sec-approval+ to land this bug now if you can get it landed on all branches (esp Beta) by Friday Sept 28.

Still need separate drivers approval for those branches AFAIK (since part of asking them is so they aren't surprised by landings). The patch looks like it would apply fine back to ESR-10.

Comment 4

[Alex Keybl [:akeybl]]

Comment on attachment 664659 [details] [diff] [review]
Properly detect IC purging.

[Triage Comment]
Feel free to land this on Aurora/Beta/ESR10 as soon as you see green on m-i/m-c. As Dan notes, please do not land later than Friday.

Comment 5

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f8daf97f682b

Comment 6

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/releases/mozilla-esr10/rev/89026001a0a9
http://hg.mozilla.org/releases/mozilla-beta/rev/0b6fd624e847
http://hg.mozilla.org/releases/mozilla-aurora/rev/66f30e40a741

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f8daf97f682b

Comment 8

[Christian Holler (:decoder)]

Test added:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a51d9ab333b9

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a51d9ab333b9

Comment 10

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old