Closed Bug 1263572 Opened 8 years ago Closed 8 years ago

Intermittent Crash [@ js::ShapeTable::getEntry] with GC

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- fixed
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: decoder, Assigned: jonco)

References

Details

(Keywords: crash, regression, testcase, Whiteboard: [jsbugmon:][debug-only code])

Crash Data

Attachments

(3 files)

The following testcase crashes on mozilla-central revision 3930bfe289c8 (build with --enable-gczeal --enable-optimize="-O2 -g" --enable-address-sanitizer --enable-posix-nspr-emulation --disable-jemalloc --disable-tests --disable-debug, run with --fuzzing-safe --thread-count=2 --disable-oom-functions --baseline-eager --ion-extra-checks --ion-eager):

See attachment.


Backtrace:

==38685==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x0000015fdcd2 sp 0x7fff6c79c3a0 bp 0x7fff6c79c3b0 T0)
    #0 0x15fdcd1 in js::ShapeTable::getEntry(unsigned int) const js/src/vm/Shape.h:239
    #1 0x15fdcd1 in js::ShapeTable::checkAfterMovingGC() js/src/vm/Shape.cpp:351
    #2 0x10e7066 in js::gc::CheckHashTablesAfterMovingGC(JSRuntime*) js/src/jsgc.cpp:7366
    #3 0x18f2a83 in js::Nursery::collect(JSRuntime*, JS::gcreason::Reason, mozilla::Vector<js::ObjectGroup*, 0ul, js::SystemAllocPolicy>*) js/src/gc/Nursery.cpp:501
    #4 0x1102edd in js::gc::GCRuntime::minorGCImpl(JS::gcreason::Reason, mozilla::Vector<js::ObjectGroup*, 0ul, js::SystemAllocPolicy>*) js/src/jsgc.cpp:6691
    #5 0x1102edd in js::gc::GCRuntime::evictNursery(JS::gcreason::Reason) js/src/gc/GCRuntime.h:612
    #6 0x10dc8d7 in js::gc::GCRuntime::gcCycle(bool, js::SliceBudget&, JS::gcreason::Reason) js/src/jsgc.cpp:6302
    #7 0x10de23b in js::gc::GCRuntime::collect(bool, js::SliceBudget, JS::gcreason::Reason) js/src/jsgc.cpp:6465
    #8 0x10e5281 in js::gc::GCRuntime::runDebugGC() js/src/jsgc.cpp:6987
    #9 0x1889b88 in js::gc::GCRuntime::gcIfNeededPerAllocation(JSContext*) js/src/gc/Allocator.cpp:28
    #10 0x189a957 in bool js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1>(JSContext*, js::gc::AllocKind) js/src/gc/Allocator.cpp:55
    #11 0x189a957 in JSObject* js::Allocate<JSObject, (js::AllowGC)1>(js::ExclusiveContext*, js::gc::AllocKind, unsigned long, js::gc::InitialHeap, js::Class const*) js/src/gc/Allocator.cpp:121
    #12 0x11154f2 in JSObject::create(js::ExclusiveContext*, js::gc::AllocKind, js::gc::InitialHeap, JS::Handle<js::Shape*>, JS::Handle<js::ObjectGroup*>) js/src/jsobjinlines.h:343
    #13 0x1138ce5 in NewObject(js::ExclusiveContext*, JS::Handle<js::ObjectGroup*>, js::gc::AllocKind, js::NewObjectKind, unsigned int) js/src/jsobj.cpp:668:21
    #14 0x1138394 in js::NewObjectWithGivenTaggedProto(js::ExclusiveContext*, js::Class const*, JS::Handle<js::TaggedProto>, js::gc::AllocKind, js::NewObjectKind, unsigned int) js/src/jsobj.cpp:729:29
    #15 0x113934e in js::NewObjectWithClassProtoCommon(js::ExclusiveContext*, js::Class const*, JS::Handle<JSObject*>, js::gc::AllocKind, js::NewObjectKind) js/src/jsobj.cpp:757
    #16 0x108d8d8 in js::NewObjectWithClassProto(js::ExclusiveContext*, js::Class const*, JS::Handle<JSObject*>, js::gc::AllocKind, js::NewObjectKind) js/src/jsobjinlines.h:694
    #17 0x108d8d8 in js::NewFunctionWithProto(js::ExclusiveContext*, bool (*)(JSContext*, unsigned int, JS::Value*), unsigned int, JSFunction::Flags, JS::Handle<JSObject*>, JS::Handle<JSAtom*>, JS::Handle<JSObject*>, js::gc::AllocKind, js::NewObjectKind, js::NewFunctionProtoHandling) js/src/jsfun.cpp:1867
    #18 0x109b89d in FunctionConstructor(JSContext*, unsigned int, JS::Value*, js::GeneratorKind) js/src/jsfun.cpp:1681
    #19 0x14ca7e8 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:235
    #20 0x14ca7e8 in js::CallJSNativeConstructor(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:268
    #21 0x14ca7e8 in InternalConstruct(JSContext*, js::AnyConstructArgs const&) js/src/vm/Interpreter.cpp:553
    #22 0x14ca41b in js::ConstructFromStack(JSContext*, JS::CallArgs const&) js/src/vm/Interpreter.cpp:592
    #23 0x846c24 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:6093

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV js/src/vm/Shape.h:239 js::ShapeTable::getEntry(unsigned int) const
==38685==ABORTING


This testcase is highly intermittent. It reproduces for me both in ASan and non-ASan optimized-only builds in only about 1 out of 500 runs.
Attached file Testcase
Probably fallout from bug 1257903.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
I haven't yet been able to reproduce this.
This looks like a rare null-deref crash in debugging code, so I'll mark it sec-audit for now. Clear or adjust the flag if that's incorrect.
Keywords: sec-audit
From decoder's more recent signature provided on IRC this we're dereferencing the swept tenured poison value so this is a use after free.

CheckHashTablesAfterMovingGC is called following a minor GC and uses ZoneCellIterUnderGC, but this doesn't wait for background sweeping to finish before iterating so probably we're racing with this and returning freed cells to the caller.

The best fix is probably to make both types of ZoneCellIter wait for BG sweeping if necessary since it's an easy check.

This crash in particular is specific to CheckHashTablesAfterMovingGC() which is not present in release builds so this probably doesn't need to be security sensitive.  I couldn't find any other instances of ZoneCellIterUnderGC being called when inside a minor GC that might also be affected.
Assignee: nobody → jcoppeard
Attachment #8743753 - Flags: review?(terrence)
Group: javascript-core-security
Whiteboard: [jsbugmon:] → [jsbugmon:][debug-only code]
Comment on attachment 8743753 [details] [diff] [review]
bug1263572-wait-for-bg-sweeping

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

Right, makes sense.
Attachment #8743753 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/ee059add9eac
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
This just missed the merge window - we should probably get this on aurora as well, to reduce our noise there.
Flags: needinfo?(jcoppeard)
Comment on attachment 8743753 [details] [diff] [review]
bug1263572-wait-for-bg-sweeping

Approval Request Comment
[Feature/regressing bug #]: Bug 1257903.
[User impact if declined]: Intermittent crashes in debug builds.
[Describe test coverage new/current, TreeHerder]: On m-c since 25th April.
[Risks and why]: Low.
[String/UUID change made/needed]: None.
Flags: needinfo?(jcoppeard)
Attachment #8743753 - Flags: approval-mozilla-aurora?
Comment on attachment 8743753 [details] [diff] [review]
bug1263572-wait-for-bg-sweeping

Crash fix, Aurora48+
Attachment #8743753 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 1264300
Comment on attachment 8743753 [details] [diff] [review]
bug1263572-wait-for-bg-sweeping

Approval Request Comment
[Feature/regressing bug #]: Bug 1257903.
[User impact if declined]: Requesting uplift so we can uplift the fix for bug 1264300 which depends on this.
[Describe test coverage new/current, TreeHerder]: On m-c since 26th April.
[Risks and why]: Low.
[String/UUID change made/needed]: None.
Attachment #8743753 - Flags: approval-mozilla-beta?
Comment on attachment 8743753 [details] [diff] [review]
bug1263572-wait-for-bg-sweeping

Crash fix, Beta47+
Attachment #8743753 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I'm hitting conflicts applying this to beta:
grafting 341365:8ba304b4b4c6 "Bug 1263572 - Wait for background sweeping to finish before checking base shapes r=terrence a=ritu"
merging js/src/jsgcinlines.h
warning: conflicts while merging js/src/jsgcinlines.h! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
Flags: needinfo?(jcoppeard)
Attached patch bug1263572-betaSplinter Review
Backport for beta.
Flags: needinfo?(jcoppeard)
Attachment #8750683 - Flags: review+
You need to log in before you can comment on or make changes to this bug.