1357012 - [wasm] AddressSanitizer: heap-buffer-overflow [@ void mozilla::detail::VectorImpl<js::SourceCompressionTask*, ...>] with WRITE of size 8 or Assertion failure: mLength + 1 <= mTail.mReserved, at mozilla/Vector.h:1255 or various crashes

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision ce69b6e1773e (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --disable-debug --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2, run with --fuzzing-safe):

var lfModule = new WebAssembly.Module(wasmTextToBinary(`
    (module
        (import "global" "func" (result i32))
        (memory (export "mem") 10)
    )
`));
evaluate(`
  var bigScript = \`
    function asmModule() { ret; function f() { return 43 } return f}
  \` + ' '.repeat(10 * 1024 * 1024);
  eval(bigScript);
`);
processModule(lfModule, `
  var x = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx";
  var x = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx";
  var x = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx";
  var bigScript = \`
    function asmModule() { ret; function f() { return 43 } return f}
  \` + ' '.repeat(10 * 1024 * 1024);
  eval(bigScript);
`);
function processModule(module, jscode) {
    while (true) {
        imports = {}
        for (let descriptor of WebAssembly.Module.imports(module)) {
            imports[descriptor.module] = {}
            imports[descriptor.module][descriptor.name] = new Function("x", "y", "z", jscode);
        }
        try {
            instance = new WebAssembly.Instance(module, imports);
        } catch (exc) {}
    }
}



Backtrace:

==11438==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60300002b020 at pc 0x00000173e1b5 bp 0x7ffef2b5ff10 sp 0x7ffef2b5ff08
WRITE of size 8 at 0x60300002b020 thread T0
    #0 0x173e1b4 in void mozilla::detail::VectorImpl<js::SourceCompressionTask*, 0ul, js::SystemAllocPolicy, true>::new_<js::SourceCompressionTask*&>(js::SourceCompressionTask**, js::SourceCompressionTask*&) mozilla/Vector.h:172:11
    #1 0x173e1b4 in void mozilla::Vector<js::SourceCompressionTask*, 0ul, js::SystemAllocPolicy>::internalAppend<js::SourceCompressionTask*&>(js::SourceCompressionTask*&) mozilla/Vector.h:1257
    #2 0x173e1b4 in void mozilla::Vector<js::SourceCompressionTask*, 0ul, js::SystemAllocPolicy>::infallibleAppend<js::SourceCompressionTask*&>(js::SourceCompressionTask*&) mozilla/Vector.h:714
    #3 0x173e1b4 in js::GlobalHelperThreadState::scheduleCompressionTasks(js::AutoLockHelperThreadState const&) js/src/vm/HelperThreads.cpp:1132
    #4 0x173e1b4 in js::GlobalHelperThreadState::startHandlingCompressionTasks(js::AutoLockHelperThreadState const&) js/src/vm/HelperThreads.cpp:1116
    #5 0x12c516a in js::gc::GCRuntime::beginMarkPhase(JS::gcreason::Reason, js::AutoLockForExclusiveAccess&) js/src/jsgc.cpp:3950:9
    #6 0x12e768b in js::gc::GCRuntime::incrementalCollectSlice(js::SliceBudget&, JS::gcreason::Reason, js::AutoLockForExclusiveAccess&) js/src/jsgc.cpp:6133:14
    #7 0x12ea076 in js::gc::GCRuntime::gcCycle(bool, js::SliceBudget&, JS::gcreason::Reason) js/src/jsgc.cpp:6489:5
    #8 0x12ee499 in js::gc::GCRuntime::collect(bool, js::SliceBudget, JS::gcreason::Reason) js/src/jsgc.cpp:6638:25
    #9 0x12bb6ce in js::gc::GCRuntime::startGC(JSGCInvocationKind, JS::gcreason::Reason, long) js/src/jsgc.cpp:6716:5
    #10 0x12bb0f6 in js::gc::GCRuntime::gcIfRequested() js/src/jsgc.cpp:6914:13
    #11 0x1bd517a in js::gc::GCRuntime::gcIfNeededPerAllocation(JSContext*) js/src/gc/Allocator.cpp:236:9
    #12 0x1bdb4a0 in bool js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1>(JSContext*, js::gc::AllocKind) js/src/gc/Allocator.cpp:191:14
    #13 0x1bdb4a0 in JSObject* js::Allocate<JSObject, (js::AllowGC)1>(JSContext*, js::gc::AllocKind, unsigned long, js::gc::InitialHeap, js::Class const*) js/src/gc/Allocator.cpp:51
    #14 0x134cd88 in js::NativeObject::create(JSContext*, js::gc::AllocKind, js::gc::InitialHeap, JS::Handle<js::Shape*>, JS::Handle<js::ObjectGroup*>) js/src/vm/NativeObject-inl.h:418:21
    #15 0x13695c6 in NewObject(JSContext*, JS::Handle<js::ObjectGroup*>, js::gc::AllocKind, js::NewObjectKind, unsigned int) js/src/jsobj.cpp:677:9
    #16 0x1368a44 in js::NewObjectWithGivenTaggedProto(JSContext*, js::Class const*, JS::Handle<js::TaggedProto>, js::gc::AllocKind, js::NewObjectKind, unsigned int) js/src/jsobj.cpp:738:26
    #17 0x1c91a3a in js::NewObjectWithGivenTaggedProto(JSContext*, js::Class const*, JS::Handle<js::TaggedProto>, js::NewObjectKind, unsigned int) js/src/jsobjinlines.h:543:12
    #18 0x1c91a3a in js::PlainObject* js::NewObjectWithGivenTaggedProto<js::PlainObject>(JSContext*, JS::Handle<js::TaggedProto>, js::NewObjectKind, unsigned int) js/src/jsobjinlines.h:552
    #19 0x1c91a3a in js::PlainObject* js::NewObjectWithGivenProto<js::PlainObject>(JSContext*, JS::Handle<JSObject*>, js::NewObjectKind) js/src/jsobjinlines.h:586
    #20 0x1c91a3a in CreateExportObject(JSContext*, JS::Handle<js::WasmInstanceObject*>, JS::Handle<JS::GCVector<JSFunction*, 0ul, js::TempAllocPolicy> >, JS::Handle<js::WasmTableObject*>, JS::Handle<js::WasmMemoryObject*>, mozilla::Vector<js::wasm::Val, 0ul, js::SystemAllocPolicy> const&, mozilla::Vector<js::wasm::Export, 0ul, js::SystemAllocPolicy> const&, JS::MutableHandle<JSObject*>) js/src/wasm/WasmModule.cpp:823
    #21 0x1c91a3a in js::wasm::Module::instantiate(JSContext*, JS::Handle<JS::GCVector<JSFunction*, 0ul, js::TempAllocPolicy> >, JS::Handle<js::WasmTableObject*>, JS::Handle<js::WasmMemoryObject*>, mozilla::Vector<js::wasm::Val, 0ul, js::SystemAllocPolicy> const&, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) const js/src/wasm/WasmModule.cpp:923
    #22 0x1c9c976 in Instantiate(JSContext*, js::wasm::Module const&, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/wasm/WasmJS.cpp:1055:12
    #23 0x1c9c2a2 in js::WasmInstanceObject::construct(JSContext*, unsigned int, JS::Value*) js/src/wasm/WasmJS.cpp:1080:10
    #24 0x7bbe1f in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:291:15
[...]

0x60300002b020 is located 0 bytes to the right of 32-byte region [0x60300002b000,0x60300002b020)
allocated by thread T0 here:
    #0 0x51979d in __interceptor_realloc compiler-rt/lib/asan/asan_malloc_linux.cc:71
    #1 0x175da82 in js_realloc(void*, unsigned long) js/Utility.h:252:12
    #2 0x175da82 in js::SourceCompressionTask** js_pod_realloc<js::SourceCompressionTask*>(js::SourceCompressionTask**, unsigned long, unsigned long) js/Utility.h:441
    #3 0x175da82 in js::SourceCompressionTask** js::SystemAllocPolicy::maybe_pod_realloc<js::SourceCompressionTask*>(js::SourceCompressionTask**, unsigned long, unsigned long) js/src/jsalloc.h:35
    #4 0x175da82 in js::SourceCompressionTask** js::SystemAllocPolicy::pod_realloc<js::SourceCompressionTask*>(js::SourceCompressionTask**, unsigned long, unsigned long) js/src/jsalloc.h:40
    #5 0x175da82 in mozilla::detail::VectorImpl<js::SourceCompressionTask*, 0ul, js::SystemAllocPolicy, true>::growTo(mozilla::Vector<js::SourceCompressionTask*, 0ul, js::SystemAllocPolicy>&, unsigned long) mozilla/Vector.h:230
    #6 0x175da82 in mozilla::Vector<js::SourceCompressionTask*, 0ul, js::SystemAllocPolicy>::growStorageBy(unsigned long) mozilla/Vector.h:1031
    #7 0x1746ba0 in mozilla::Vector<js::SourceCompressionTask*, 0ul, js::SystemAllocPolicy>::reserve(unsigned long) mozilla/Vector.h:1089:9
    #8 0x1746ba0 in js::EnqueueOffThreadCompression(JSContext*, js::SourceCompressionTask*) js/src/vm/HelperThreads.cpp:1770
    #9 0x14f2a16 in BytecodeCompiler::enqueueOffThreadSourceCompression() js/src/frontend/BytecodeCompiler.cpp:242:14
    #10 0x14f8d54 in BytecodeCompiler::compileStandaloneFunction(JS::MutableHandle<JSFunction*>, js::GeneratorKind, js::FunctionAsyncKind, mozilla::Maybe<unsigned int> const&) js/src/frontend/BytecodeCompiler.cpp:528:10
    #11 0x14ff084 in js::frontend::CompileStandaloneFunction(JSContext*, JS::MutableHandle<JSFunction*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, mozilla::Maybe<unsigned int> const&, JS::Handle<js::Scope*>) js/src/frontend/BytecodeCompiler.cpp:774:12
    #12 0x128ee2f in FunctionConstructor(JSContext*, JS::CallArgs const&, js::GeneratorKind, js::FunctionAsyncKind) js/src/jsfun.cpp:1806:18
    #13 0x128cf46 in js::Function(JSContext*, unsigned int, JS::Value*) js/src/jsfun.cpp:1818:12
    #14 0x7bbe1f in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:291:15
[...]

SUMMARY: AddressSanitizer: heap-buffer-overflow mozilla/Vector.h:172:11 in void mozilla::detail::VectorImpl<js::SourceCompressionTask*, 0ul, js::SystemAllocPolicy, true>::new_<js::SourceCompressionTask*&>(js::SourceCompressionTask**, js::SourceCompressionTask*&)
Shadow bytes around the buggy address:
  0x0c067fffd5f0: fd fa fa fa fd fd fd fd fa fa fd fd fd fa fa fa
=>0x0c067fffd600: 00 00 00 00[fa]fa 00 00 00 00 fa fa fd fd fd fa
  0x0c067fffd610: fa fa fd fd fd fd fa fa fd fd fd fd fa fa 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd



Marking s-s and sec-critical because this looks like a bad out-of-bounds write.

Comment 1

[Christian Holler (:decoder)]

Needinfo for Benjamin due to wasm.

Comment 2

[Jan de Mooij [:jandem]]

NI shu too, probably related to bug 1348134.

Comment 3

[Shu-yu Guo [:shu]]

This is my stupid mistake. I tried to reserve the compression task worklist's capacity ahead of time so I could infallible append by reserving pending.length() whenever a new task is enqueued. However, it's very possible that as tasks get scheduled and are in-flight, worklist.length() needs to be > pending.length().

I'll have a patch shortly.

Comment 4

[Shu-yu Guo [:shu]]

Not Benjamin's fault.

Comment 5

[Shu-yu Guo [:shu]]

Attachment: Use fallible append for compression tasks and use UniquePtrs.

Stupid bug, see comment 3. I made tasks into UniquePtrs while I was at it to
avoid screwing up the js_deletes, since now these are all over the place.

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 8858950 [details] [diff] [review]
Use fallible append for compression tasks and use UniquePtrs.

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

Great, this looks like a nice simplification too.

::: js/src/vm/HelperThreads.h
@@ +164,5 @@
>      // Helper method for removing items from the vectors below while iterating over them.
>      template <typename T>
>      void remove(T& vector, size_t* index)
>      {
> +        vector[(*index)--] = mozilla::Move(vector.back());

What happens if the vector has only one element, or if index refers to the last element?  It looks like this move the element to itself.  Isn't that undefined behaviour?

@@ +713,5 @@
>  
>    public:
>      // The majorGCNumber is used for scheduling tasks. If the task is being
>      // enqueued from an off-thread parsing task, leave the GC number
>      // UINT64_MAX to be fixed up when the parse task finishes.

Does this comment still apply?

Comment 7

[Shu-yu Guo [:shu]]

(In reply to Jon Coppeard (:jonco) from comment #6)
> Comment on attachment 8858950 [details] [diff] [review]
> Use fallible append for compression tasks and use UniquePtrs.
> 
> Review of attachment 8858950 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great, this looks like a nice simplification too.
> 
> ::: js/src/vm/HelperThreads.h
> @@ +164,5 @@
> >      // Helper method for removing items from the vectors below while iterating over them.
> >      template <typename T>
> >      void remove(T& vector, size_t* index)
> >      {
> > +        vector[(*index)--] = mozilla::Move(vector.back());
> 
> What happens if the vector has only one element, or if index refers to the
> last element?  It looks like this move the element to itself.  Isn't that
> undefined behaviour?

Oh ouch, good catch!

> 
> @@ +713,5 @@
> >  
> >    public:
> >      // The majorGCNumber is used for scheduling tasks. If the task is being
> >      // enqueued from an off-thread parsing task, leave the GC number
> >      // UINT64_MAX to be fixed up when the parse task finishes.
> 
> Does this comment still apply?

Nope, will delete.

Comment 8

[Carsten Book [:Tomcat]]

backed out for errors like https://treeherder.mozilla.org/logviewer.html#?job_id=92583194&repo=mozilla-inbound

Comment 9

[Carsten Book [:Tomcat]]

Hey Shu, seems the latest backout was wrong and your push + follow up innocent.

Could you push the original patch and the follow up patch again ?

Sorry for the inconvenience normally i would do this myself but since the follow-up patch is not attached i better not mess around in sec bugs :) so would be awesome if you could push again. Thanks!

Comment 10

[Shu-yu Guo [:shu]]

No problem. Re-pushed.

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 12

[Andrew McCreight [:mccr8]]

It looks like this was fixed.

Comment 13

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.