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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Fuzzing Team]

JSBugMon: Cannot process bug: Error: Failed to isolate test from comment

Comment 3

[Fuzzing Team]

JSBugMon: Bisection requested, failed due to error: Error: Failed to isolate test from comment

Comment 4

[Terrence Cole [:terrence]]

Will make a note to take a look later.

Comment 5

[Andrew Overholt [:overholt]]

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.

Comment 6

[Terrence Cole [:terrence]]

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

Comment 7

[Jan de Mooij [:jandem]]

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..

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Al, Dan, would you be able to comment on the severity of this issue? Is this a potential Fx47 release blocker? Thanks!

Comment 9

[Al Billings [:abillings - ex-MoCo]]

This doesn't seem to be a blocker and can wait for 48.

Comment 10

[Jan de Mooij [:jandem]]

Adding the NI that was (likely accidentally) cleared.

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(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.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

Naveed, can you find someone to investigate this issue?

Comment 13

[Kannan Vijayan [:djvj]]

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.

Comment 14

[Kannan Vijayan [:djvj]]

Attachment: fix-bug-1246680.patch

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.

Comment 15

[Kannan Vijayan [:djvj]]

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.

Comment 17

[Andrew McCreight [:mccr8]]

This is just a bug in a shell function, so I'm unhiding it. Thanks for looking into it, Kannan.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/514f963dfefd
Make ReadSPSProfilingStack check that profiling is not suppressed before proceeding. r=shu

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/514f963dfefd