1652492 - Assertion failure: lock_.ownedByCurrentThread(), at gc/StoreBuffer.h:397 with WeakRef and clearKeptObjects

Status: VERIFIED FIXED

(Core :: JavaScript: GC, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20200712-22f5f7e91444 (debug build, run with --fuzzing-safe --no-threads):

function f1() {
  arr = f2;
  var p = arr[(gczeal(9))|0];
}
f2 = f1;
f2();
try {
  function allocObj() { return {}; }
  {
    let obj = allocObj();
    wr = new WeakRef(obj);
  }
  clearKeptObjects();
(new obj);
} catch(exc) {}
let obj = allocObj();
wr = new WeakRef(obj);

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x00005555558126cf in JSObject::writeBarrierPost(void*, JSObject*, JSObject*) ()
#1  0x0000555555f1ab67 in mozilla::HashMapEntry<js::HeapPtr<JSObject*>, js::WeakRefHeapPtrVector>::HashMapEntry(mozilla::HashMapEntry<js::HeapPtr<JSObject*>, js::WeakRefHeapPtrVector>&&) ()
#2  0x0000555555f1aa74 in void mozilla::detail::EntrySlot<mozilla::HashMapEntry<js::HeapPtr<JSObject*>, js::WeakRefHeapPtrVector> >::setLive<mozilla::HashMapEntry<js::HeapPtr<JSObject*>, js::WeakRefHeapPtrVector> >(unsigned int, mozilla::HashMapEntry<js::HeapPtr<JSObject*>, js::WeakRefHeapPtrVector>&&) ()
#3  0x0000555555f1a673 in void mozilla::detail::HashTable<mozilla::HashMapEntry<js::HeapPtr<JSObject*>, js::WeakRefHeapPtrVector>, mozilla::HashMap<js::HeapPtr<JSObject*>, js::WeakRefHeapPtrVector, js::MovableCellHasher<js::HeapPtr<JSObject*> >, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::forEachSlot<mozilla::detail::HashTable<mozilla::HashMapEntry<js::HeapPtr<JSObject*>, js::WeakRefHeapPtrVector>, mozilla::HashMap<js::HeapPtr<JSObject*>, js::WeakRefHeapPtrVector, js::MovableCellHasher<js::HeapPtr<JSObject*> >, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::changeTableSize(unsigned int, mozilla::detail::HashTable<mozilla::HashMapEntry<js::HeapPtr<JSObject*>, js::WeakRefHeapPtrVector>, mozilla::HashMap<js::HeapPtr<JSObject*>, js::WeakRefHeapPtrVector, js::MovableCellHasher<js::HeapPtr<JSObject*> >, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::FailureBehavior)::{lambda(mozilla::detail::EntrySlot<mozilla::HashMapEntry<js::HeapPtr<JSObject*>, js::WeakRefHeapPtrVector> >&)#1}>(char*, unsigned int, mozilla::detail::HashTable<mozilla::HashMapEntry<js::HeapPtr<JSObject*>, js::WeakRefHeapPtrVector>, mozilla::HashMap<js::HeapPtr<JSObject*>, js::WeakRefHeapPtrVector, js::MovableCellHasher<js::HeapPtr<JSObject*> >, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::changeTableSize(unsigned int, mozilla::detail::HashTable<mozilla::HashMapEntry<js::HeapPtr<JSObject*>, js::WeakRefHeapPtrVector>, mozilla::HashMap<js::HeapPtr<JSObject*>, js::WeakRefHeapPtrVector, js::MovableCellHasher<js::HeapPtr<JSObject*> >, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::FailureBehavior)::{lambda(mozilla::detail::EntrySlot<mozilla::HashMapEntry<js::HeapPtr<JSObject*>, js::WeakRefHeapPtrVector> >&)#1}&&) ()
#4  0x0000555555f1a537 in mozilla::detail::HashTable<mozilla::HashMapEntry<js::HeapPtr<JSObject*>, js::WeakRefHeapPtrVector>, mozilla::HashMap<js::HeapPtr<JSObject*>, js::WeakRefHeapPtrVector, js::MovableCellHasher<js::HeapPtr<JSObject*> >, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::changeTableSize(unsigned int, mozilla::detail::HashTable<mozilla::HashMapEntry<js::HeapPtr<JSObject*>, js::WeakRefHeapPtrVector>, mozilla::HashMap<js::HeapPtr<JSObject*>, js::WeakRefHeapPtrVector, js::MovableCellHasher<js::HeapPtr<JSObject*> >, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::FailureBehavior) ()
#5  0x0000555555ed6bc8 in js::WeakRefMap::sweep() ()
#6  0x0000555556237780 in js::gc::GCRuntime::sweepWeakRefs() ()
#7  0x0000555556269e0c in AutoRunParallelTask::run() ()
#8  0x000055555625403c in js::GCParallelTask::runTask() ()
#9  0x0000555556237a0a in js::gc::GCRuntime::startTask(js::GCParallelTask&, js::gcstats::PhaseKind, js::AutoLockHelperThreadState&) ()
#10 0x00005555562393b3 in js::gc::GCRuntime::beginSweepingSweepGroup(JSFreeOp*, js::SliceBudget&) ()
#11 0x000055555627ce01 in sweepaction::SweepActionSequence::run(js::gc::SweepAction::Args&) ()
#12 0x000055555626b4b7 in sweepaction::SweepActionForEach<js::gc::SweepGroupsIter, JSRuntime*>::run(js::gc::SweepAction::Args&) ()
#13 0x000055555623e249 in js::gc::GCRuntime::performSweepActions(js::SliceBudget&) ()
#14 0x000055555624320a in js::gc::GCRuntime::incrementalSlice(js::SliceBudget&, mozilla::Maybe<JSGCInvocationKind> const&, JS::GCReason, js::gc::AutoGCSession&) ()
#15 0x000055555624608b in js::gc::GCRuntime::gcCycle(bool, js::SliceBudget, mozilla::Maybe<JSGCInvocationKind> const&, JS::GCReason) ()
#16 0x0000555556247d10 in js::gc::GCRuntime::collect(bool, js::SliceBudget, mozilla::Maybe<JSGCInvocationKind> const&, JS::GCReason) ()
#17 0x000055555620eb8d in js::gc::GCRuntime::finishGC(JS::GCReason) ()
#18 0x00005555562167dc in js::gc::FinishGC(JSContext*, JS::GCReason) ()
#19 0x00005555557c301b in mozilla::ScopeExit<main::$_5>::~ScopeExit() ()
#20 0x00005555557bad07 in main ()
rax	0x555556fcc147	93825019986247
rbx	0x7ffff572c488	140737311327368
rcx	0x5555583ca840	93825040951360
rdx	0x0	0
rsi	0x7ffff6abd770	140737331844976
rdi	0x7ffff6abc540	140737331840320
rbp	0x7fffffffce30	140737488342576
rsp	0x7fffffffce10	140737488342544
r8	0x7ffff6abd770	140737331844976
r9	0x7ffff7fe3d40	140737354022208
r10	0x58	88
r11	0x7ffff67647a0	140737328334752
r12	0x20	32
r13	0x7ffff4e4f380	140737302033280
r14	0x7ffff571a9f0	140737311255024
r15	0x7ffff4e4f380	140737302033280
rip	0x5555558126cf <JSObject::writeBarrierPost(void*, JSObject*, JSObject*)+255>
=> 0x5555558126cf <_ZN8JSObject16writeBarrierPostEPvPS_S1_+255>:	movl   $0x18d,0x0
   0x5555558126da <_ZN8JSObject16writeBarrierPostEPvPS_S1_+266>:	callq  0x5555558492fe <abort>

Signature and test are different to that in bug 1652425, so filing this separately.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1652492 - Lock store buffer when sweeping weak ref map r?sfink

Similar to how we did for WeakCache<GCHashMap>, take the store buffer lock while destructing the WeakRefMap Enum.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1470369

Comment 4

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20200714153520-bca48c382991.
The bug appears to have been introduced in the following build range:
> Start: 74f73190afad2bbde97bd6009430b87445718a01 (20200616184413)
> End: 934e959205abe817c23015c326cff2413e1f040c (20200616184513)
> Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=74f73190afad2bbde97bd6009430b87445718a01&tochange=934e959205abe817c23015c326cff2413e1f040c

Comment 5

[Jon Coppeard (:jonco)]

Comment on attachment 9163441 [details]
Bug 1652492 - Lock store buffer when sweeping weak ref map r?sfink

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Very difficult.
• 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?: 79 is affected.
• If not all supported branches, which bug introduced the flaw?: Bug 1470369
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: Same patch should apply.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely. This technique was already used in other places for 1470369 but this case was missed.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9163441 [details]
Bug 1652492 - Lock store buffer when sweeping weak ref map r?sfink

Doesn't need sec-approval since it's rated moderate in severity.

Pushed to autoland:
https://hg.mozilla.org/integration/autoland/rev/54f7d812c432692f74fbf0f807683ffcf39da364

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/54f7d812c432

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 9

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20200717093907-c00b0b97ea65.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.

Comment 10

[Jon Coppeard (:jonco)]

Comment on attachment 9163441 [details]
Bug 1652492 - Lock store buffer when sweeping weak ref map r?sfink

Beta/Release Uplift Approval Request

• User impact if declined: Possible crash / security vulnerability for code that uses WeakRef.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch uses the same synchronisation approach used in bug 1470369 and applies it in one place that was missed from the original change.
• String changes made/needed: None

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9163441 [details]
Bug 1652492 - Lock store buffer when sweeping weak ref map r?sfink

Approved for 79.0rc1.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c5b1f43fec6e