Skip to content

Commit ef8d20a

Browse files
committed
[Runtime] Fix incorrect memory ordering in ConcurrentReadableArray/HashMap.
When reallocating storage, the storing the pointer to the new storage had insufficiently strong ordering. This could cause writers to check the reader count before storing the new storage pointer. If a reader then came in between that load and store, it would end up using the old storage pointer, while the writer could end up freeing it. Also adjust the Concurrent.cpp tests to test with a variety of reader and writer counts. Counterintuitively, when freeing garbage is gated on there being zero readers, having a single reader will shake out problems that having lots of readers will not. We were testing with lots of readers in order to stress the code as much as possible, but this resulted in it being extremely rare for writers to ever see zero active readers. rdar://69798617
1 parent 7e7ab2f commit ef8d20a

File tree

2 files changed

+336
-294
lines changed

2 files changed

+336
-294
lines changed

include/swift/Runtime/Concurrent.h

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -527,7 +527,12 @@ template <class ElemTy> struct ConcurrentReadableArray {
527527

528528
storage = newStorage;
529529
Capacity = newCapacity;
530-
Elements.store(storage, std::memory_order_release);
530+
531+
// Use seq_cst here to ensure that the subsequent load of ReaderCount is
532+
// ordered after this store. If ReaderCount is loaded first, then a new
533+
// reader could come in between that load and this store, and then we
534+
// could end up freeing the old storage pointer while it's still in use.
535+
Elements.store(storage, std::memory_order_seq_cst);
531536
}
532537

533538
new(&storage->data()[count]) ElemTy(elem);
@@ -866,7 +871,11 @@ struct ConcurrentReadableHashMap {
866871
FreeListNode::add(&FreeList, elements);
867872
}
868873

869-
Elements.store(newElements, std::memory_order_release);
874+
// Use seq_cst here to ensure that the subsequent load of ReaderCount is
875+
// ordered after this store. If ReaderCount is loaded first, then a new
876+
// reader could come in between that load and this store, and then we
877+
// could end up freeing the old elements pointer while it's still in use.
878+
Elements.store(newElements, std::memory_order_seq_cst);
870879
return newElements;
871880
}
872881

@@ -900,7 +909,11 @@ struct ConcurrentReadableHashMap {
900909
newIndices.storeIndexAt(nullptr, index, newI, std::memory_order_relaxed);
901910
}
902911

903-
Indices.store(newIndices.Value, std::memory_order_release);
912+
// Use seq_cst here to ensure that the subsequent load of ReaderCount is
913+
// ordered after this store. If ReaderCount is loaded first, then a new
914+
// reader could come in between that load and this store, and then we
915+
// could end up freeing the old indices pointer while it's still in use.
916+
Indices.store(newIndices.Value, std::memory_order_seq_cst);
904917

905918
if (auto *ptr = indices.pointer())
906919
FreeListNode::add(&FreeList, ptr);

0 commit comments

Comments
 (0)