1297360 - AddressSanitizer: heap-use-after-free [@ void mozilla::PodAssign<unsigned char>] with READ of size 1 with Profiling

Status: RESOLVED DUPLICATE of bug 1316150

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision f97a056ae623 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2, run with --fuzzing-safe --no-threads --ion-eager):

var lfLogBuffer = `
  gczeal(14);
  enableSPSProfiling();
  gczeal(15,3);
  var s = "";
  for (let i = 0; i != 30; i+=2) {}
  readSPSProfilingStack(s, "c0d1c0d1c0d1c0d1c0d1c0d1c0d1c0");
`;
loadFile(lfLogBuffer);
function loadFile(lfVarx) {
  let m = parseModule(lfVarx); 
  m.declarationInstantiation();
  m.evaluation();
}



Backtrace:

==24850==ERROR: AddressSanitizer: heap-use-after-free on address 0x6020000098b0 at pc 0x0000016dc3d0 bp 0x7ffff8295c90 sp 0x7ffff8295c88
READ of size 1 at 0x6020000098b0 thread T0
    #0 0x16dc3cf in void mozilla::PodAssign<unsigned char>(unsigned char*, unsigned char const*) dist/include/mozilla/PodOperations.h:87:3
    #1 0x16dc3cf in void mozilla::PodCopy<unsigned char>(unsigned char*, unsigned char const*, unsigned long) dist/include/mozilla/PodOperations.h:107
    #2 0x16dc3cf in JSInlineString* js::NewInlineString<(js::AllowGC)1, unsigned char>(js::ExclusiveContext*, mozilla::Range<unsigned char const>) js/src/vm/String-inl.h:60
    #3 0x16dc3cf in JSFlatString* js::NewStringCopyNDontDeflate<(js::AllowGC)1, unsigned char>(js::ExclusiveContext*, unsigned char const*, unsigned long) js/src/vm/String.cpp:1241
    #4 0x1a195a2 in JSFlatString* js::NewStringCopyN<(js::AllowGC)1>(js::ExclusiveContext*, char const*, unsigned long) js/src/vm/String.h:1190:12
    #5 0x1a195a2 in JSFlatString* js::NewStringCopyZ<(js::AllowGC)1>(js::ExclusiveContext*, char const*) js/src/vm/String.h:1210
    #6 0x1a195a2 in ReadSPSProfilingStack(JSContext*, unsigned int, JS::Value*) js/src/builtin/TestingFunctions.cpp:1696
[...]

0x6020000098b0 is located 0 bytes inside of 9-byte region [0x6020000098b0,0x6020000098b9)
freed by thread T0 here:
    #0 0x5112e0 in __interceptor_free /srv/repos/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:38
    #1 0xb453ea in js_free(void*) dist/include/js/Utility.h:263:5
    #2 0xb453ea in js::jit::JitcodeGlobalEntry::IonEntry::destroy() js/src/jit/JitcodeMap.cpp:142
    #3 0xb4810f in js::jit::JitcodeGlobalEntry::destroy() js/src/jit/JitcodeMap.h:604:13
    #4 0xb4810f in js::jit::JitcodeGlobalTable::removeEntry(js::jit::JitcodeGlobalEntry&, js::jit::JitcodeGlobalEntry**, JSRuntime*) js/src/jit/JitcodeMap.cpp:553
    #5 0xb494f3 in js::jit::JitcodeGlobalTable::releaseEntry(js::jit::JitcodeGlobalEntry&, js::jit::JitcodeGlobalEntry**, JSRuntime*) js/src/jit/JitcodeMap.cpp:567:5
    #6 0xb494f3 in js::jit::JitcodeGlobalTable::Enum::removeFront() js/src/jit/JitcodeMap.cpp:432
    #7 0xb494f3 in js::jit::JitcodeGlobalTable::sweep(JSRuntime*) js/src/jit/JitcodeMap.cpp:826
    #8 0x10abd62 in js::gc::GCRuntime::beginSweepingZoneGroup(js::AutoLockForExclusiveAccess&) js/src/jsgc.cpp:5037:13
    #9 0x10af22f in js::gc::GCRuntime::beginSweepPhase(bool, js::AutoLockForExclusiveAccess&) js/src/jsgc.cpp:5189:5
    #10 0x10b613c in js::gc::GCRuntime::incrementalCollectSlice(js::SliceBudget&, JS::gcreason::Reason, js::AutoLockForExclusiveAccess&) js/src/jsgc.cpp:5888:9
    #11 0x10b7ede in js::gc::GCRuntime::gcCycle(bool, js::SliceBudget&, JS::gcreason::Reason) js/src/jsgc.cpp:6142:5
    #12 0x10b9560 in js::gc::GCRuntime::collect(bool, js::SliceBudget, JS::gcreason::Reason) js/src/jsgc.cpp:6249:25
    #13 0x10c0380 in js::gc::GCRuntime::gc(JSGCInvocationKind, JS::gcreason::Reason) js/src/jsgc.cpp:6314:5
    #14 0x10c0380 in js::gc::GCRuntime::runDebugGC() js/src/jsgc.cpp:6770
    #15 0x1ae2d0c in js::gc::GCRuntime::gcIfNeededPerAllocation(JSContext*) js/src/gc/Allocator.cpp:224:9
    #16 0x1aefef4 in bool js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1>(JSContext*, js::gc::AllocKind) js/src/gc/Allocator.cpp:189:14
    #17 0x1aefef4 in JSString* js::Allocate<JSString, (js::AllowGC)1>(js::ExclusiveContext*) js/src/gc/Allocator.cpp:139
    #18 0x16daf5c in JSThinInlineString* JSThinInlineString::new_<(js::AllowGC)1>(js::ExclusiveContext*) js/src/vm/String-inl.h:250:45
    #19 0x16daf5c in JSInlineString* js::AllocateInlineString<(js::AllowGC)1, unsigned char>(js::ExclusiveContext*, unsigned long, unsigned char**) js/src/vm/String-inl.h:30
    #20 0x16daf5c in JSInlineString* js::NewInlineString<(js::AllowGC)1, unsigned char>(js::ExclusiveContext*, mozilla::Range<unsigned char const>) js/src/vm/String-inl.h:56
    #21 0x16daf5c in JSFlatString* js::NewStringCopyNDontDeflate<(js::AllowGC)1, unsigned char>(js::ExclusiveContext*, unsigned char const*, unsigned long) js/src/vm/String.cpp:1241
    #22 0x1a195a2 in JSFlatString* js::NewStringCopyN<(js::AllowGC)1>(js::ExclusiveContext*, char const*, unsigned long) js/src/vm/String.h:1190:12
    #23 0x1a195a2 in JSFlatString* js::NewStringCopyZ<(js::AllowGC)1>(js::ExclusiveContext*, char const*) js/src/vm/String.h:1210
    #24 0x1a195a2 in ReadSPSProfilingStack(JSContext*, unsigned int, JS::Value*) js/src/builtin/TestingFunctions.cpp:1696
  [...]

previously allocated by thread T0 here:
    #0 0x511628 in __interceptor_malloc /srv/repos/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:52
    #1 0x5a2b6b in js_malloc(unsigned long) dist/include/js/Utility.h:235:12
    #2 0x5a2b6b in char* js_pod_malloc<char>(unsigned long) dist/include/js/Utility.h:483
    #3 0x5a2b6b in char* js::MallocProvider<js::ExclusiveContext>::maybe_pod_malloc<char>(unsigned long) js/src/vm/MallocProvider.h:57
    #4 0x5a2b6b in char* js::MallocProvider<js::ExclusiveContext>::pod_malloc<char>(unsigned long) js/src/vm/MallocProvider.h:90
    #5 0xb462fc in js::jit::JitcodeGlobalEntry::createScriptString(JSContext*, JSScript*, unsigned long*) js/src/jit/JitcodeMap.cpp:356:17
    #6 0xb4ce12 in js::jit::JitcodeIonTable::makeIonEntry(JSContext*, js::jit::JitCode*, unsigned int, JSScript**, js::jit::JitcodeGlobalEntry::IonEntry&) js/src/jit/JitcodeMap.cpp:1454:21
    #7 0x9620ab in js::jit::CodeGenerator::link(JSContext*, js::CompilerConstraintList*) js/src/jit/CodeGenerator.cpp:9341:14
    #8 0xabb20c in LinkCodeGen(JSContext*, js::jit::IonBuilder*, js::jit::CodeGenerator*) js/src/jit/Ion.cpp:537:10
    #9 0x9fdced in js::jit::IonCompile(JSContext*, JSScript*, js::jit::BaselineFrame*, unsigned char*, bool, bool, js::jit::OptimizationLevel) js/src/jit/Ion.cpp:2311:21
    #10 0x9fdced in js::jit::Compile(JSContext*, JS::Handle<JSScript*>, js::jit::BaselineFrame*, unsigned char*, bool, bool) js/src/jit/Ion.cpp:2469
    #11 0x9ff048 in BaselineCanEnterAtBranch(JSContext*, JS::Handle<JSScript*>, js::jit::BaselineFrame*, unsigned char*) js/src/jit/Ion.cpp:2656:27
[...]

I'm not sure if we have this on file somewhere already, I remember a similar issue a while ago.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

This is a recent regression?

Comment 2

[Fuzzing Team]

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

Comment 3

[Jon Coppeard (:jonco) (PTO until 17th May)]

ProfilingFrameIterator::extractStack fills in Frame::label with a malloced string pointer that is owned by the JIT code table.  These strings can be freed by GC, so this is not safe.

Comment 4

[Jon Coppeard (:jonco) (PTO until 17th May)]

Attachment: bug1297360-frame-label

Patch to copy the label string and make the label field a UniquePtr so that it gets cleaned up properly.

I made this abort silently on OOM.  I could make it fallible and report OOM to the caller if you think it's worth it.

Comment 5

[Kannan Vijayan [:djvj]]

Comment on attachment 8788474 [details] [diff] [review]
bug1297360-frame-label

Review of attachment 8788474 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Just wanted to confirm that with C++ structs, applying |move| to the struct correctly moves all the constituent fields?  If you know that, r=me.

Comment 6

[Jeff Walden [:Waldo]]

(In reply to Kannan Vijayan [:djvj] from comment #5)
> Just wanted to confirm that with C++ structs, applying |move|
> to the struct correctly moves all the constituent fields?

Moving doesn't "do" anything at all.  It just effectively says, "My contents are available to be taken from me, because I'm going to be destroyed soon."  If the *consumer* of the move decides to take advantage of that offer, then things are handled more intelligently than just a plain copy would.  If it doesn't take advantage of that, nothing special happens.

Put another way, |std::move(foo)| or |mozilla::Move(foo)| does nothing but cast its argument.  All the intelligence of moving is provided by typical consumers like |operator=(Foo&& foo)| or |Foo(Foo&& foo)|, or by less-common consumers like |void bar(Foo&& foo)| that take an rvalue reference (the result of a move-cast) and take advantage of the movability of |foo| in that case.

Comment 7

[Jon Coppeard (:jonco) (PTO until 17th May)]

(In reply to Kannan Vijayan [:djvj] from comment #5)

And further to what Waldo wrote, in this case Frame will get an implicitly defined move constructor which will move construct all its members.  UniquePtr implements a move constructor which will correctly move the label.  All other fields will be copied as normal.

Comment 8

[Jon Coppeard (:jonco) (PTO until 17th May)]

The test code causes an assertion failure in compacting GC builds.  I don't think this is related to the issue here so I'm going to land the patch without testcode and file a new bug for that.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/093a7b8b7285
Copy strings returned by ProfilingFrameIterator API r=djvj

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/093a7b8b7285

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c207eec1dcd8
Backout changeset 093a7b8b7285 for causing bug 1316150

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

[Carsten Book [:Tomcat]]

reopened because of the backout

Comment 14

[Jon Coppeard (:jonco) (PTO until 17th May)]

This is fixed by the patch in bug 1316150.