Closed Bug 1246680 Opened 4 years ago Closed 3 years ago

Crash [@ __interceptor_strlen] with use-after-free and ReadSPSProfilingStack

Categories

(Core :: JavaScript Engine, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox47 --- wontfix
firefox48 --- ?
firefox49 --- ?
firefox-esr45 --- ?
firefox50 --- fixed

People

(Reporter: decoder, Unassigned)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:])

Crash Data

Attachments

(1 file, 1 obsolete file)

The following crash occurred on mozilla-central revision 5f9ba76eb3b1 (build with --enable-gczeal --enable-optimize="-O2 -g" --enable-address-sanitizer --target=i686-pc-linux-gnu --without-intl-api --enable-posix-nspr-emulation --disable-jemalloc --disable-tests --disable-debug, run with --fuzzing-safe --thread-count=2 --baseline-eager --ion-eager --ion-offthread-compile=off).

The testcase didn't yield any reproducible results for me, but I thought the trace alone might help since it's a detailed ASan use-after-free trace:


Backtrace:

==20778==ERROR: AddressSanitizer: heap-use-after-free on address 0xf6120c70 at pc 0x80be4d2 bp 0xff951af8 sp 0xff951adc
READ of size 2 at 0xf6120c70 thread T0
    #0 0x80be4d1 in __interceptor_strlen /srv/repos/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:509
    #1 0x94d117e in JSFlatString* js::NewStringCopyZ<(js::AllowGC)1>(js::ExclusiveContext*, char const*) js/src/vm/String.h:1201
    #2 0x94d117e in ReadSPSProfilingStack(JSContext*, unsigned int, JS::Value*) js/src/builtin/TestingFunctions.cpp:1514
    #3 0x95b2162 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:235
[...]
    #6 0x84cdf8f in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) js/src/jit/BaselineIC.cpp:6135

0xf6120c70 is located 0 bytes inside of 9-byte region [0xf6120c70,0xf6120c79)
freed by thread T0 here:
    #0 0x80f1d3c in __interceptor_free /srv/repos/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64
    #1 0x887f273 in js_free(void*) js/src/opt32asan/dist/include/js/Utility.h:249
    #2 0x887f273 in js::jit::JitcodeGlobalEntry::IonEntry::destroy() js/src/jit/JitcodeMap.cpp:140
    #3 0x8883da4 in js::jit::JitcodeGlobalEntry::destroy() js/src/jit/JitcodeMap.h:604
    #4 0x8883da4 in js::jit::JitcodeGlobalTable::removeEntry(js::jit::JitcodeGlobalEntry&, js::jit::JitcodeGlobalEntry**, JSRuntime*) js/src/jit/JitcodeMap.cpp:579
    #5 0x88856a7 in js::jit::JitcodeGlobalTable::releaseEntry(js::jit::JitcodeGlobalEntry&, js::jit::JitcodeGlobalEntry**, JSRuntime*) js/src/jit/JitcodeMap.cpp:593
    #6 0x88856a7 in js::jit::JitcodeGlobalTable::Enum::removeFront() js/src/jit/JitcodeMap.cpp:432
    #7 0x88856a7 in js::jit::JitcodeGlobalTable::sweep(JSRuntime*) js/src/jit/JitcodeMap.cpp:852
    #8 0x8698b06 in js::jit::JitRuntime::SweepJitcodeGlobalTable(JSRuntime*) js/src/jit/Ion.cpp:710
[...]
    #16 0x9a0b66a in js::gc::GCRuntime::gcIfNeededPerAllocation(JSContext*) js/src/gc/Allocator.cpp:28
    #17 0x9a15b90 in JSContext::runtime() const js/src/gc/Allocator.cpp:55
    #18 0x9a15b90 in JSString* js::Allocate<JSString, (js::AllowGC)1>(js::ExclusiveContext*) js/src/gc/Allocator.cpp:211
    #19 0x97d7723 in JSThinInlineString* JSThinInlineString::new_<(js::AllowGC)1>(js::ExclusiveContext*) js/src/vm/String.h:250
    #20 0x97d7723 in JSInlineString* js::AllocateInlineString<(js::AllowGC)1, unsigned char>(js::ExclusiveContext*, unsigned int, unsigned char**) js/src/vm/String-inl.h:30
    #21 0x97d7723 in JSInlineString* js::NewInlineString<(js::AllowGC)1, unsigned char>(js::ExclusiveContext*, mozilla::Range<unsigned char const>) js/src/vm/String-inl.h:56
    #22 0x97d7723 in JSFlatString* js::NewStringCopyNDontDeflate<(js::AllowGC)1, unsigned char>(js::ExclusiveContext*, unsigned char const*, unsigned int) js/src/vm/String.cpp:1138
    #23 0x94d105d in JSFlatString* js::NewStringCopyN<(js::AllowGC)1>(js::ExclusiveContext*, char const*, unsigned int) js/src/vm/String.h:1181
    #24 0x94d105d in JSFlatString* js::NewStringCopyZ<(js::AllowGC)1>(js::ExclusiveContext*, char const*) js/src/vm/String.h:1201
    #25 0x94d105d in ReadSPSProfilingStack(JSContext*, unsigned int, JS::Value*) js/src/builtin/TestingFunctions.cpp:1507
    #26 0x95b2162 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:235
[...]
    #29 0x84cdf8f in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) js/src/jit/BaselineIC.cpp:6135

previously allocated by thread T0 here:
    #0 0x80f1fb4 in __interceptor_malloc /srv/repos/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74
    #1 0x8168df2 in js_malloc(unsigned int) js/src/opt32asan/dist/include/js/Utility.h:221
    #2 0x8168df2 in _ZL13js_pod_mallocIcEPT_j js/src/opt32asan/dist/include/js/Utility.h:412
    #3 0x8168df2 in char* js::MallocProvider<js::ExclusiveContext>::maybe_pod_malloc<char>(unsigned int) js/src/vm/MallocProvider.h:57
    #4 0x8168df2 in char* js::MallocProvider<js::ExclusiveContext>::pod_malloc<char>(unsigned int) js/src/vm/MallocProvider.h:90
    #5 0x8881269 in js::jit::JitcodeGlobalEntry::createScriptString(JSContext*, JSScript*, unsigned int*) js/src/jit/JitcodeMap.cpp:356
    #6 0x888a41e in js::jit::JitcodeIonTable::makeIonEntry(JSContext*, js::jit::JitCode*, unsigned int, JSScript**, js::jit::JitcodeGlobalEntry::IonEntry&) js/src/jit/JitcodeMap.cpp:1486
    #7 0x861e6bb in js::jit::CodeGenerator::link(JSContext*, js::CompilerConstraintList*) js/src/jit/CodeGenerator.cpp:8478
    #8 0x86bb5a2 in LinkCodeGen(JSContext*, js::jit::IonBuilder*, js::jit::CodeGenerator*, JS::MutableHandle<js::GCVector<JSScript*, 0u, js::TempAllocPolicy> >, OnIonCompilationInfo*) js/src/jit/Ion.cpp:586
    #9 0x86b1a3d in js::jit::IonCompile(JSContext*, JSScript*, js::jit::BaselineFrame*, unsigned char*, bool, bool, js::jit::OptimizationLevel) js/src/jit/Ion.cpp:2287
[...]
    #15 0x8ca93d7 in js::ModuleObject::evaluate(JSContext*, JS::Handle<js::ModuleObject*>, JS::MutableHandle<JS::Value>) js/src/builtin/ModuleObject.cpp:825
    #16 0x977d2f3 in intrinsic_EvaluateModule(JSContext*, unsigned int, JS::Value*) js/src/vm/SelfHosting.cpp:1573

Shadow bytes around the buggy address:
  0x3ec24170: fa fa fd fd fa fa fd fd fa fa 00 00 fa fa fd fd
=>0x3ec24180: fa fa fd fa fa fa fd fd fa fa fd fa fa fa[fd]fd
  0x3ec24190: fa fa fd fa fa fa fd fa fa fa fd fd fa fa fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Heap left redzone:       fa
  Freed heap region:       fd
Attached file Testcase (obsolete) —
Attachment #8717006 - Attachment is obsolete: true
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
JSBugMon: Cannot process bug: Error: Failed to isolate test from comment
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
JSBugMon: Bisection requested, failed due to error: Error: Failed to isolate test from comment
Will make a note to take a look later.
Flags: needinfo?(terrence)
Did you get a chance to take a look, Terrence? This is marked as a regression in 47 which goes to release on June 7th.
Sorry, this fell through the cracks. 

According to [1], the label field that is being read after being freed must be statically allocated. Jan, is the JIT violating this assumption?

1- https://dxr.mozilla.org/mozilla-central/source/js/src/vm/SPSProfiler.h#287
Flags: needinfo?(terrence) → needinfo?(jdemooij)
We're crashing in ReadSPSProfilingStack, here:

  frameLabel = NewStringCopyZ<CanGC>(cx, frames[inlineFrameNo].label);

We crash because our label was freed by a GC that was triggered earlier in the same loop (the first NewStringCopyZ call).

Since it's an IonEntry, one possibility is that the GC invalidated some Ion script on the stack, and then it was able to free its corresponding IonEntry. I don't see how that's possible though, because we should still mark the invalidated IonScript's JitCode - see the checkInvalidation case in MarkIonJSFrame.

Kannan, any thoughts?

Marking this s-s just to be safe..
Group: javascript-core-security
Flags: needinfo?(jdemooij) → needinfo?(kvijayan)
Hello Al, Dan, would you be able to comment on the severity of this issue? Is this a potential Fx47 release blocker? Thanks!
Flags: needinfo?(kvijayan)
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
This doesn't seem to be a blocker and can wait for 48.
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
Keywords: sec-high
Adding the NI that was (likely accidentally) cleared.
Flags: needinfo?(kvijayan)
(In reply to Al Billings [:abillings] from comment #9)
> This doesn't seem to be a blocker and can wait for 48.

Thanks Al! This is now a wontfix for Fx47.
Naveed, can you find someone to investigate this issue?
Flags: needinfo?(nihsanullah)
So.. IMHO ReadSPSProfilingStack should not be scanning the tables when a GC is running.  The situation is inherently racey.  Suppressing sampling during JitcodeGlobalTable::sweep() should resolve this.  I'll post a test patch after I repro.
So, the API-based stack sampling should not be happening at all when sampling has been suppressed.  We suppress sampling in many places to prevent race conditions when updating the table.

The testing API function has been modified to return an empty stack array when profiling has been suppressed.
Attachment #8770715 - Flags: review?(shu)
This bug is definitely not a blocker.  The issue is showing up because a function that's only exposed to to the JS shell test environment(ReadSPSProfilingStack) did not respect sample-suppression.
Flags: needinfo?(kvijayan)
Flags: needinfo?(nihsanullah)
Duplicate of this bug: 1282783
This is just a bug in a shell function, so I'm unhiding it. Thanks for looking into it, Kannan.
Group: javascript-core-security
Keywords: sec-high
Attachment #8770715 - Flags: review?(shu) → review+
Pushed by kvijayan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/514f963dfefd
Make ReadSPSProfilingStack check that profiling is not suppressed before proceeding. r=shu
https://hg.mozilla.org/mozilla-central/rev/514f963dfefd
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.