1516514 - AddressSanitizer: heap-buffer-overflow [@ js::LiveSavedFrameCache::FramePtr::operator==] with READ of size 1 with ReadableStream

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

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

evalInWorker(`
Set.prototype.filter = function filter(f) {}
try { evaluate(\`
  if (typeof assertDeepEq === 'undefined') {}
  async function test() {
    if (typeof newGlobal !== 'undefined')
        otherGlobal = newGlobal();
    OtherReadableStream = otherGlobal.ReadableStream;
    OtherReadableStreamReader = new otherGlobal.ReadableStream().getReader().constructor;
    try {
        let reader = new ReadableStream({
            start(c) {},
        }).getReader({ mode: "byob" });
    } catch (e) {}
    let chunk = { name: "chunk" };
    let controller;
    let otherStream;
    function getFreshInstances(type, otherType = type) {
        stream = new ReadableStream({ start(c) { controller = c; }, type });
        otherStream = new OtherReadableStream({ start(c) { otherController = c; }, type: otherType });
    }
    getFreshInstances();
    otherReader = OtherReadableStream.prototype.getReader.call(stream);
    Object.defineProperty(stream, "locked",
        Object.getOwnPropertyDescriptor(OtherReadableStream.prototype, "locked")
    );
    request = otherReader.read();
    test(this, [, , , undefined], 5) | ArrayBuffer;
    otherController.close.call(controller);
    x = await request instanceof otherGlobal.Object
    x.toString()
  }
  async function runTest() {
    await test();
  }
  runTest();
\`); } catch(exc) {}
`);


Backtrace:

==11540==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60c000041728 at pc 0x5583065deea2 bp 0x7fc74476dbf0 sp 0x7fc74476dbe8
READ of size 1 at 0x60c000041728 thread T10
    #0 0x5583065deea1 in mozilla::Variant<js::InterpreterFrame*, js::jit::CommonFrameLayout*, js::jit::RematerializedFrame*, js::wasm::DebugFrame*>::operator==(mozilla::Variant<js::InterpreterFrame*, js::jit::CommonFrameLayout*, js::jit::RematerializedFrame*, js::wasm::DebugFrame*> const&) const dist/include/mozilla/Variant.h:613:12
    #1 0x5583065deea1 in js::LiveSavedFrameCache::FramePtr::operator==(js::LiveSavedFrameCache::FramePtr const&) const js/src/vm/Stack.h:1258
    #2 0x5583065deea1 in js::LiveSavedFrameCache::Key::operator==(js::LiveSavedFrameCache::Key const&) const js/src/vm/Stack.h:1273
    #3 0x5583065deea1 in js::LiveSavedFrameCache::Key::operator!=(js::LiveSavedFrameCache::Key const&) const js/src/vm/Stack.h:1275
    #4 0x5583065deea1 in js::LiveSavedFrameCache::find(JSContext*, js::LiveSavedFrameCache::FramePtr&, unsigned char const*, JS::MutableHandle<js::SavedFrame*>) const js/src/vm/SavedStacks.cpp:124
    #5 0x5583065f0fb2 in js::SavedStacks::insertFrames(JSContext*, JS::MutableHandle<js::SavedFrame*>, mozilla::Variant<JS::AllFrames, JS::MaxFrames, JS::FirstSubsumedFrame>&&) js/src/vm/SavedStacks.cpp:1322:14
    #6 0x5583065eee27 in js::SavedStacks::saveCurrentStack(JSContext*, JS::MutableHandle<js::SavedFrame*>, mozilla::Variant<JS::AllFrames, JS::MaxFrames, JS::FirstSubsumedFrame>&&) js/src/vm/SavedStacks.cpp:1184:10
    #7 0x558306b66982 in JS::CaptureCurrentStack(JSContext*, JS::MutableHandle<JSObject*>, mozilla::Variant<JS::AllFrames, JS::MaxFrames, JS::FirstSubsumedFrame>&&) js/src/jsapi.cpp:6088:29
    #8 0x5583060cfe15 in PromiseDebugInfo::setResolutionInfo(JSContext*, JS::Handle<js::PromiseObject*>) js/src/builtin/Promise.cpp:397:10
    #9 0x55830608f10b in js::PromiseObject::onSettled(JSContext*, JS::Handle<js::PromiseObject*>) js/src/builtin/Promise.cpp:4540:3
    #10 0x55830608f10b in ResolvePromise(JSContext*, JS::Handle<js::PromiseObject*>, JS::Handle<JS::Value>, JS::PromiseState) js/src/builtin/Promise.cpp:1119
    #11 0x5583060b0149 in FulfillMaybeWrappedPromise(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>) js/src/builtin/Promise.cpp:1161:10
    #12 0x55830607e5a1 in ResolvePromiseInternal(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>) js/src/builtin/Promise.cpp:812:12
    #13 0x55830606092e in js::PromiseObject::resolve(JSContext*, JS::Handle<js::PromiseObject*>, JS::Handle<JS::Value>) js/src/builtin/Promise.cpp:4497:12
    #14 0x558306b5417c in ResolveOrRejectPromise(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, bool) js/src/jsapi.cpp:4021:19
    #15 0x5583066b5acb in ReadableStreamCloseInternal(JSContext*, JS::Handle<js::ReadableStream*>) js/src/builtin/Stream.cpp:1503:8
    #16 0x55830678bf87 in ReadableStreamDefaultControllerClose(JSContext*, JS::Handle<js::ReadableStreamDefaultController*>) js/src/builtin/Stream.cpp:2945:12
    #17 0x55830678bf87 in ReadableStreamDefaultController_close(JSContext*, unsigned int, JS::Value*) js/src/builtin/Stream.cpp:2374
    #18 0x558305f10f7e in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/vm/Interpreter.cpp:443:13
    #19 0x558305f10f7e in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:535
    #20 0x558305f13862 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:606:8
    #21 0x5583063f333f in js::fun_call(JSContext*, unsigned int, JS::Value*) js/src/vm/JSFunction.cpp:1192:10
    #22 0x558305f10f7e in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/vm/Interpreter.cpp:443:13
    #23 0x558305f10f7e in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:535
    #24 0x558305f13862 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:606:8
    #25 0x558306c56367 in js::ForwardingProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const js/src/proxy/Wrapper.cpp:162:10
    #26 0x558306c0b8b1 in js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const js/src/proxy/CrossCompartmentWrapper.cpp:304:19
    #27 0x558306c34316 in js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) js/src/proxy/Proxy.cpp:535:19
    #28 0x558305f121ef in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:509:14
    #29 0x558307194898 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:3910:10
    #30 0x7fc703fb7d47  (<unknown module>)

0x60c000041728 is located 24 bytes to the left of 128-byte region [0x60c000041740,0x60c0000417c0)
allocated by thread T10 here:
    #0 0x558305d4ad08 in __interceptor_malloc (/mnt/LangFuzz/work/builds/opt64asan/dist/bin/js+0x631d08)
    #1 0x55830669350d in js_arena_malloc(unsigned long, unsigned long) dist/include/js/Utility.h:353:10
    #2 0x55830669350d in js::LiveSavedFrameCache::Entry* js_pod_arena_malloc<js::LiveSavedFrameCache::Entry>(unsigned long, unsigned long) dist/include/js/Utility.h:535
    #3 0x55830669350d in js::LiveSavedFrameCache::Entry* js_pod_malloc<js::LiveSavedFrameCache::Entry>(unsigned long) dist/include/js/Utility.h:540
    #4 0x55830669350d in js::LiveSavedFrameCache::Entry* js::AllocPolicyBase::maybe_pod_malloc<js::LiveSavedFrameCache::Entry>(unsigned long) dist/include/js/AllocPolicy.h:31
    #5 0x55830669350d in js::LiveSavedFrameCache::Entry* js::AllocPolicyBase::pod_malloc<js::LiveSavedFrameCache::Entry>(unsigned long) dist/include/js/AllocPolicy.h:43
    #6 0x55830669350d in mozilla::detail::VectorImpl<js::LiveSavedFrameCache::Entry, 0ul, js::SystemAllocPolicy, false>::growTo(mozilla::Vector<js::LiveSavedFrameCache::Entry, 0ul, js::SystemAllocPolicy>&, unsigned long) dist/include/mozilla/Vector.h:127
    #7 0x5583065de37f in mozilla::Vector<js::LiveSavedFrameCache::Entry, 0ul, js::SystemAllocPolicy>::growByUninitialized(unsigned long) dist/include/mozilla/Vector.h:1113:9
    #8 0x5583065de37f in bool mozilla::Vector<js::LiveSavedFrameCache::Entry, 0ul, js::SystemAllocPolicy>::emplaceBack<js::LiveSavedFrameCache::FramePtr&, unsigned char const*&, JS::Handle<js::SavedFrame*>&>(js::LiveSavedFrameCache::FramePtr&, unsigned char const*&, JS::Handle<js::SavedFrame*>&) dist/include/mozilla/Vector.h:673
    #9 0x5583065de37f in js::LiveSavedFrameCache::insert(JSContext*, js::LiveSavedFrameCache::FramePtr&, unsigned char const*, JS::Handle<js::SavedFrame*>) js/src/vm/SavedStacks.cpp:85
    #10 0x5583065f1c83 in js::SavedStacks::insertFrames(JSContext*, JS::MutableHandle<js::SavedFrame*>, mozilla::Variant<JS::AllFrames, JS::MaxFrames, JS::FirstSubsumedFrame>&&) js/src/vm/SavedStacks.cpp:1447:29
    #11 0x5583065eee27 in js::SavedStacks::saveCurrentStack(JSContext*, JS::MutableHandle<js::SavedFrame*>, mozilla::Variant<JS::AllFrames, JS::MaxFrames, JS::FirstSubsumedFrame>&&) js/src/vm/SavedStacks.cpp:1184:10
    #12 0x558306b66982 in JS::CaptureCurrentStack(JSContext*, JS::MutableHandle<JSObject*>, mozilla::Variant<JS::AllFrames, JS::MaxFrames, JS::FirstSubsumedFrame>&&) js/src/jsapi.cpp:6088:29
    #13 0x5583060daab0 in PromiseDebugInfo::create(JSContext*, JS::Handle<js::PromiseObject*>, mozilla::Maybe<mozilla::TimeStamp> const&) js/src/builtin/Promise.cpp:297:10
    #14 0x558306077d45 in CreatePromiseObjectInternal(JSContext*, JS::Handle<JSObject*>, bool, bool) js/src/builtin/Promise.cpp:2007:7
    #15 0x558306077d45 in CreatePromiseObjectWithoutResolutionFunctions(JSContext*) js/src/builtin/Promise.cpp:1177
    #16 0x55830607a69f in NewPromiseCapability(JSContext*, JS::Handle<JSObject*>, JS::MutableHandle<PromiseCapability>, bool) js/src/builtin/Promise.cpp:1241:17
    #17 0x558306081e18 in CommonStaticResolveRejectImpl(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, ResolutionMode) js/src/builtin/Promise.cpp:3125:8
    #18 0x558306082580 in js::PromiseObject::unforgeableResolve(JSContext*, JS::Handle<JS::Value>) js/src/builtin/Promise.cpp:3202:10
    #19 0x5583066b3b59 in SetUpReadableStreamDefaultController(JSContext*, JS::Handle<js::ReadableStream*>, SourceAlgorithms, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, double, JS::Handle<JS::Value>) js/src/builtin/Stream.cpp:3195:29
    #20 0x5583066b1f16 in SetUpReadableStreamDefaultControllerFromUnderlyingSource(JSContext*, JS::Handle<js::ReadableStream*>, JS::Handle<JS::Value>, double, JS::Handle<JS::Value>) js/src/builtin/Stream.cpp:3272:10
    #21 0x5583066b15c9 in js::ReadableStream::constructor(JSContext*, unsigned int, JS::Value*) js/src/builtin/Stream.cpp:622:10
[...]
    #26 0x5583077ce7da in EnterJit(JSContext*, js::RunState&, unsigned char*) js/src/jit/Jit.cpp:103:5
    #27 0x5583077ce7da in js::jit::MaybeEnterJit(JSContext*, js::RunState&) js/src/jit/Jit.cpp:168
    #28 0x558305edc1b8 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:408:32
    #29 0x558305f11a5b in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:563:13
    #30 0x558305f13862 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:606:8
    #31 0x558306614180 in js::CallSelfHostedFunction(JSContext*, JS::Handle<js::PropertyName*>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) js/src/vm/SelfHosting.cpp:1854:10
    #32 0x558306124cfc in AsyncFunctionResume(JSContext*, JS::Handle<js::PromiseObject*>, JS::Handle<JS::Value>, ResumeKind, JS::Handle<JS::Value>) js/src/vm/AsyncFunction.cpp:202:8
    #33 0x558306123def in AsyncFunctionStart(JSContext*, JS::Handle<js::PromiseObject*>, JS::Handle<JS::Value>) js/src/vm/AsyncFunction.cpp:217:10
    #34 0x558306123def in WrappedAsyncFunction(JSContext*, unsigned int, JS::Value*) js/src/vm/AsyncFunction.cpp:94
[...]
    #39 0x5583077ce7da in EnterJit(JSContext*, js::RunState&, unsigned char*) js/src/jit/Jit.cpp:103:5
    #40 0x5583077ce7da in js::jit::MaybeEnterJit(JSContext*, js::RunState&) js/src/jit/Jit.cpp:168

Thread T10 created by T0 here:
    #0 0x558305c9f09d in __interceptor_pthread_create (/mnt/LangFuzz/work/builds/opt64asan/dist/bin/js+0x58609d)
    #1 0x558306c5eb4c in js::Thread::create(void* (*)(void*), void*) js/src/threading/posix/Thread.cpp:97:7
    #2 0x558305e0640a in bool js::Thread::init<void (&)(WorkerInput*), WorkerInput*&>(void (&)(WorkerInput*), WorkerInput*&) js/src/threading/Thread.h:124:12
    #3 0x558305dc50d1 in EvalInWorker(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:4000:29
    #4 0x558305f10f7e in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/vm/Interpreter.cpp:443:13
[...]

SUMMARY: AddressSanitizer: heap-buffer-overflow dist/include/mozilla/Variant.h:613:12 in mozilla::Variant<js::InterpreterFrame*, js::jit::CommonFrameLayout*, js::jit::RematerializedFrame*, js::wasm::DebugFrame*>::operator==(mozilla::Variant<js::InterpreterFrame*, js::jit::CommonFrameLayout*, js::jit::RematerializedFrame*, js::wasm::DebugFrame*> const&) const
Shadow bytes around the buggy address:
  0x0c18800002c0: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa
  0x0c18800002d0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c18800002e0: fa fa fa fa fa[fa]fa fa 00 00 00 00 00 00 00 00
  0x0c18800002f0: 00 00 00 00 00 00 00 00 fa fa fa fa fa fa fa fa
  0x0c1880000300: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
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
==11540==ABORTING


With a debug build, I get the same assert as in bug 1451268 but that bug uses Debugger and this one does not. It uses ReadableStream instead which I think we already enabled by default on Nightly at least. The assertion match could be coincidence (since it is a fairly generic assertion) but if it is not, we should mark 1451268 also as s-s.

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/4f9a88ca20eb
user:        Tooru Fujisawa
date:        Tue Nov 27 19:18:52 2018 +0900
summary:     Bug 1509768 - Handle the case that String#replace is called with a empty string pattern on a rope. r=evilpie

This iteration took 538.759 seconds to run.

Comment 2

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

Tooru, can you take a look at this?

Comment 3

[Tooru Fujisawa [:arai]]

The code added by 4f9a88ca20eb is not used in the testcase, except that there's a new testing function added to the global.
If the failure starts from 4f9a88ca20eb, I suspect the failure is discovered by the change to the number of global properties.

anyway, on debug build on m-c 0cf7daf34a37 (today) and 4f9a88ca20eb, and f3361f0816dd (parent of 4f9a88ca20eb), on macOS,
I hit the following assertion:

https://searchfox.org/mozilla-central/rev/7a922172a94cfe24c7a48e0a581577895e1da8c4/js/src/vm/SavedStacks.cpp#141
> void LiveSavedFrameCache::find(JSContext* cx, FramePtr& framePtr,
>                                const jsbytecode* pc,
>                                MutableHandleSavedFrame frame) const {
> ...
>   while (key != frames->back().key) {
> ...
> 
>     // If the frame's bit was set, the frame should always have an entry in
>     // the cache. (If we purged the entire cache because its SavedFrames had
>     // been captured for a different compartment, then we would have
>     // returned early above.)
>     MOZ_ALWAYS_TRUE(!frames->empty());

which looks more related to the buffer overflow.

Comment 4

[Jason Orendorff [:jorendorff]]

Partial analysis here, more tomorrow.

Factors present:
- OOM
- async function calling itself recursively, on-stack (before the first await)
- SavedStack cache being purged due to a realm mismatch

My current suspicion is that the SavedStack cache stuff might not be robust against OOM followed by immediately trying again. Note that ReadableStreamCloseInternal() calls ResolvePromise in two places:
1. https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/js/src/builtin/Stream.cpp#1488
2. https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/js/src/builtin/Stream.cpp#1503

In this test case, OOM happens first under newGlobal; then two or three times under getOrCreateArrayPrototype; then under SavedFrame::create() with the following stack (most recent call first, many frames omitted):

  ReportOutOfMemory()
  js::Allocate()
  SavedFrame::create()
  SavedStacks::insertFrames()
  JS::CaptureCurrentStack()
  PromiseDebugInfo::setResolutionInfo()
  JS::ResolvePromise()
  ReadableStreamCloseInternal() -- at ResolvePromise call site #1

SavedStacks::insertFrames() bails out with "out of memory" pending.
PromiseDebugInfo::setResolutionInfo silently catches this pending exception and discards it.
So JS::ResolvePromise() reports success.

This is the most unusual thing about this test case: OOM is caught and squelched, so we get to retry JS::CaptureCurrentStack() without having popped even one stack frame.

The second call to JS::ResolvePromise() also ends up in JS::CaptureCurrentStack(), and that is when the assertion fails. The topmost frame, I guess, has the cached bit set, but isn't cached.

Comment 5

[Jason Orendorff [:jorendorff]]

Actual test case I was looking at:

evalInWorker(`
p1 = 0;
p2 = null;
p3 = null;
p4 = null;
x = null;
async function test() {
  print(p1++);
  let otherGlobal = newGlobal();  // OOM 1 here
  let OtherReadableStream = otherGlobal.ReadableStream;
  let OtherReadableStreamReader = new otherGlobal.ReadableStream().getReader().constructor;
  try {
    DIE;
  } catch (e) {}
  let controller;
  let stream = new ReadableStream({ start(c) { controller = c; },  undefined });
  let otherController;
  let otherStream = new OtherReadableStream({ start(c) { otherController = c; }, type: undefined });
  let otherReader = OtherReadableStream.prototype.getReader.call(stream);
  let request = otherReader.read();
  test();
  otherController.close.call(controller);
  x = await request;  // OOM 2 here perhaps
}
eval('test();');
`);

Comment 6

[Jason Orendorff [:jorendorff]]

Got it. Or part of it, at least. LiveSavedFrameCache has some too-clever invariants that are not robust against OOM. I'm guessing the main reason oomTest() hasn't found this is that it's a cache. (Caches cause oomTest iterations to interfere with one another in a way that can let bugs slip through. If anyone reading this wants to hear all about it, ping me on IRC.)

Asserting test case without promises, async functions, or ReadableStream:

----

let g = newGlobal();

function oomTest() {
  let done = false;
  for (let j = 1; !done; j++) {
    saveStack();
    oomAtAllocation(j);
    try { g.saveStack(); } catch {}
    done = !resetOOMFailure();
    try { g.saveStack(); } catch {}
  }
}

oomTest();

Comment 7

[Jim Blandy :jimb]

It makes sense for me to take this.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Hi Jim, we've got about a week left in the Fx65 cycle before we start RC week. Have you been able to make any progress on this?

Comment 9

[Jim Blandy :jimb]

I'll have a fix for this by tomorrow afternoon.

Comment 10

[Jim Blandy :jimb]

The assertion at hand claims that, if a frame has its hasCachedSavedFrame bit set, then it must have a cache entry. This is untrue after we clear the cache due to compartment mismatch, but the assertion is never reached when the cache is empty - or so we had been led to believe.

Here's the scenario:

• A capture successfully inserts two frames into the cache, setting the hasCachedSavedFrame bit on each.

• A second capture flushes the cache for a compartment mismatch. Now the cache is empty, although both frames still have their bits set.

• A third capture tolerates the spurious hasCachedSavedFrame bits, because the cache is empty, but then OOMs after inserting only the older frame's entry.

• A fourth capture sees that there is no cache entry for the younger frame, yet the cache is not empty, and asserts.

Comment 11

[Jim Blandy :jimb]

I'd dealt with a similar case in bug 1445973, but I didn't recognize this other way for the same situation to arise.

Comment 12

[Jim Blandy :jimb]

Attachment: Bug 1516514: Clear the hasCachedSavedFrame bit on frames on compartment mismatch. r?jorendorff

The code that manages the LiveSavedFrameCache would very much like to assert
that, if a frame has its hasCachedSavedFrame bit set, then it actually does have
an entry in the LiveSavedFrameCache. However, in the presence of compartment
mismatches, this becomes temporarily untrue, and OOMs can make 'temporarily'
longer than expected.

This patch more aggressively clears frames' hasCachedSavedFrame bits, so that
when we do purge the cache for a compartment mismatch, all frames get their bits
cleared before we start repopulating the cache.

Comment 13

[Jim Blandy :jimb]

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=991a3b03d800e8e3e3b4da632ad9fcd7899c9b35

Comment 14

[Jim Blandy :jimb]

Patch revised; new try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=928eb1251a48721c936793aeb0d2dd71b23f4bb8

Comment 15

[Ryan VanderMeulen [:RyanVM]]

This was pushed to autoland:
https://hg.mozilla.org/integration/autoland/rev/a3092a304863

Jim, this should have had sec-approval first because it's sec-high and affects all releases AFAICT. Please request that still and also Beta & ESR60 approval.

Comment 16

[Jim Blandy :jimb]

Comment on attachment 9036824 [details]
Bug 1516514: Clear the hasCachedSavedFrame bit on frames on compartment mismatch. r?jorendorff

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: I don't think the patch reveals much. The bug arises from interactions between three separate JavaScript stack captures that must take place from different compartments, and requires OOMs to occur at specific points. Even after we'd reduced the test case it wasn't obvious to us why it happened. The patch just makes the code more aggressive about keeping a flag accurate, so it is not apparent from the code why that would cause a buffer underflow.

However, if one can force the bug to happen, JavaScript stack capture will pop more items off a vector than it contains, leaving the 'end' of the vector pointing outside its buffer. Subsequent attempts to push new items into the vector will then write LiveSavedFrameCache::Entry structures to those addresses, which contain data under user control (a pointer to a script url and function name; a line and column number).

Stack capture occurs whenever a JavaScript Error object is constructed, so the affected code is used in both parent and content processes. However, the attacker would presumably have less control, if any, over LiveSavedFrameCache::Entry contents in the parent process.

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

Which older supported branches are affected by this flaw?: esr60

If not all supported branches, which bug introduced the flaw?: Bug 1443592

Do you have backports for the affected branches?: No

If not, how different, hard to create, and risky will they be?: They should be much the same. There is a simple variant of the patch that will guarantee a crash if the bug is triggered.

How likely is this patch to cause regressions; how much testing does it need?: It's unlikely to cause regressions.

Comment 17

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

Comment on attachment 9036824 [details]
Bug 1516514: Clear the hasCachedSavedFrame bit on frames on compartment mismatch. r?jorendorff

Sigh.
Approvals given after the fact in discussion with Ryan.

Comment 18

[Jim Blandy :jimb]

(In reply to Al Billings [:abillings] from comment #17)

Sigh.

Yeah. I'm sorry. I will be more careful in the future.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/666a44dfa6eb

Comment 20

[Jim Blandy :jimb]

Attachment: Bug 1516514: Don't misuse MOZ_ALWAYS_TRUE. For ESR60 backport. r?jorendorff

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/256453759958

Note for anybody verifying this later that the ESR60 patch is just making this a safe crash because it doesn't have the clearHasCachedSavedFrame method.

Comment 23

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.