Skip to content

Commit 29c9f2a

Browse files
authored
Merge pull request #35348 from mikeash/fix-concurrentarray-memory-order2
[Runtime] Fix incorrect memory ordering in ConcurrentReadableArray/HashMap
2 parents 9688a55 + ef8d20a commit 29c9f2a

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)