[Runtime] Avoid operator new/delete in ConcurrentReadableArray.

The callbacks made in ImageInspectionMachO.cpp are called in a dangerous context, with the dyld and ObjC runtime locks held. C++ allows programs to overload the global operator new/delete, and there's no guarantee that those overloads behave. Ideally, we'd avoid them entirely, but that's a bigger job. For now, avoid the worst trouble by avoiding STL and new/delete in these callbacks. That use came from ConcurrentReadableArray's free list, so we switch that from a std::vector to a linked list.

rdar://75036800

Author: mikeash

include/swift/Runtime/Concurrent.h

@@ -416,6 +416,35 @@ class ConcurrentMap
   }
 };
 
+/// A simple linked list representing pointers that need to be freed. This is
+/// not a concurrent data structure, just a bit of support used in the types
+/// below.
+struct ConcurrentFreeListNode {
+  ConcurrentFreeListNode *Next;
+  void *Ptr;
+
+  static void add(ConcurrentFreeListNode **head, void *ptr) {
+    auto *newNode = reinterpret_cast<ConcurrentFreeListNode *>(
+        malloc(sizeof(ConcurrentFreeListNode)));
+    newNode->Next = *head;
+    newNode->Ptr = ptr;
+    *head = newNode;
+  }
+
+  /// Free all nodes in the free list, resetting `head` to `NULL`. Calls
+  /// `FreeFn` on the Ptr field of every node.
+  template <typename FreeFn>
+  static void freeAll(ConcurrentFreeListNode **head, const FreeFn &freeFn) {
+    auto *node = *head;
+    while (node) {
+      auto *next = node->Next;
+      freeFn(node->Ptr);
+      free(node);
+      node = next;
+    }
+    *head = nullptr;
+  }
+};
 
 /// An append-only array that can be read without taking locks. Writes
 /// are still locked and serialized, but only with respect to other
@@ -454,8 +483,8 @@ template <class ElemTy> struct ConcurrentReadableArray {
   std::atomic<size_t> ReaderCount;
   std::atomic<Storage *> Elements;
   Mutex WriterLock;
-  std::vector<Storage *> FreeList;
-  
+  ConcurrentFreeListNode *FreeList{nullptr};
+
   void incrementReaders() {
     ReaderCount.fetch_add(1, std::memory_order_acquire);
   }
@@ -465,10 +494,9 @@ template <class ElemTy> struct ConcurrentReadableArray {
   }
 
   void deallocateFreeList() {
-    for (Storage *storage : FreeList)
-      storage->deallocate();
-    FreeList.clear();
-    FreeList.shrink_to_fit();
+    ConcurrentFreeListNode::freeAll(&FreeList, [](void *ptr) {
+      reinterpret_cast<Storage *>(ptr)->deallocate();
+    });
   }
 
 public:
@@ -522,7 +550,7 @@ template <class ElemTy> struct ConcurrentReadableArray {
       if (storage) {
         std::copy(storage->data(), storage->data() + count, newStorage->data());
         newStorage->Count.store(count, std::memory_order_release);
-        FreeList.push_back(storage);
+        ConcurrentFreeListNode::add(&FreeList, storage);
       }
 
       storage = newStorage;
@@ -797,28 +825,6 @@ struct ConcurrentReadableHashMap {
     ElemTy *data() { return &Elem; }
   };
 
-  /// A simple linked list representing pointers that need to be freed.
-  struct FreeListNode {
-    FreeListNode *Next;
-    void *Ptr;
-
-    static void add(FreeListNode **head, void *ptr) {
-      auto *newNode = new FreeListNode{*head, ptr};
-      *head = newNode;
-    }
-
-    static void freeAll(FreeListNode **head) {
-      auto *node = *head;
-      while (node) {
-        auto *next = node->Next;
-        free(node->Ptr);
-        delete node;
-        node = next;
-      }
-      *head = nullptr;
-    }
-  };
-
   /// The number of readers currently active, equal to the number of snapshot
   /// objects currently alive.
   std::atomic<uint32_t> ReaderCount{0};
@@ -840,7 +846,7 @@ struct ConcurrentReadableHashMap {
   MutexTy WriterLock;
 
   /// The list of pointers to be freed once no readers are active.
-  FreeListNode *FreeList{nullptr};
+  ConcurrentFreeListNode *FreeList{nullptr};
 
   void incrementReaders() {
     ReaderCount.fetch_add(1, std::memory_order_acquire);
@@ -854,7 +860,7 @@ struct ConcurrentReadableHashMap {
   /// there are active readers, do nothing.
   void deallocateFreeListIfSafe() {
     if (ReaderCount.load(std::memory_order_seq_cst) == 0)
-      FreeListNode::freeAll(&FreeList);
+      ConcurrentFreeListNode::freeAll(&FreeList, free);
   }
 
   /// Grow the elements array, adding the old array to the free list and
@@ -868,7 +874,7 @@ struct ConcurrentReadableHashMap {
     if (elements) {
       memcpy(newElements->data(), elements->data(),
              elementCount * sizeof(ElemTy));
-      FreeListNode::add(&FreeList, elements);
+      ConcurrentFreeListNode::add(&FreeList, elements);
     }
 
     // Use seq_cst here to ensure that the subsequent load of ReaderCount is
@@ -916,7 +922,7 @@ struct ConcurrentReadableHashMap {
     Indices.store(newIndices.Value, std::memory_order_seq_cst);
 
     if (auto *ptr = indices.pointer())
-      FreeListNode::add(&FreeList, ptr);
+      ConcurrentFreeListNode::add(&FreeList, ptr);
 
     return newIndices;
   }
@@ -1122,8 +1128,8 @@ struct ConcurrentReadableHashMap {
     Elements.store(nullptr, std::memory_order_relaxed);
 
     if (auto *ptr = indices.pointer())
-      FreeListNode::add(&FreeList, ptr);
-    FreeListNode::add(&FreeList, elements);
+      ConcurrentFreeListNode::add(&FreeList, ptr);
+    ConcurrentFreeListNode::add(&FreeList, elements);
 
     deallocateFreeListIfSafe();
   }

stdlib/public/runtime/ImageInspectionMachO.cpp

@@ -129,6 +129,13 @@ void addImageCallback2Sections(const mach_header *mh, intptr_t vmaddr_slide) {
 #define REGISTER_FUNC(...) _dyld_register_func_for_add_image(__VA_ARGS__)
 #endif
 
+// WARNING: the callbacks are called from unsafe contexts (with the dyld and
+// ObjC runtime locks held) and must be very careful in what they do. Locking
+// must be arranged to avoid deadlocks (other code must never call out to dyld
+// or ObjC holding a lock that gets taken in one of these callbacks) and the
+// new/delete operators must not be called, in case a program supplies an
+// overload which does not cooperate with these requirements.
+
 void swift::initializeProtocolLookup() {
   REGISTER_FUNC(addImageCallback<TextSegment, ProtocolsSection,
                                  addImageProtocolsBlockCallbackUnsafe>);

test/stdlib/symbol-visibility-linux.test-sh

@@ -23,17 +23,7 @@
 // RUN:             -e _ZN9__gnu_cxx12__to_xstringINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEcEET_PFiPT0_mPKS8_P13__va_list_tagEmSB_z \
 // RUN:             -e _ZZNSt19_Sp_make_shared_tag5_S_tiEvE5__tag \
 // RUN:             -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEDnEEEvv \
-// RUN:             -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA1_cEEEvv \
-// RUN:             -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA8_cEEEvv \
-// RUN:             -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA24_cEEEvv \
-// RUN:             -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA32_cEEEvv \
-// RUN:             -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA40_cEEEvv \
-// RUN:             -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA80_cEEEvv \
-// RUN:             -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA88_cEEEvv \
-// RUN:             -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA104_cEEEvv \
-// RUN:             -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA160_cEEEvv \
-// RUN:             -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA168_cEEEvv \
-// RUN:             -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA216_cEEEvv \
+// RUN:             -e '_ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA[0-9]\+_cEEEvv' \
 // RUN:             -e _ZN9__gnu_cxx12__to_xstringISscEET_PFiPT0_mPKS2_P13__va_list_tagEmS5_z \
 // RUN:             -e  _ZZNSt8__detail18__to_chars_10_implIjEEvPcjT_E8__digits \
 // RUN:             -e  _ZZNSt8__detail18__to_chars_10_implImEEvPcjT_E8__digits \
@@ -51,17 +41,7 @@
 // RUN:             -e _ZN9__gnu_cxx12__to_xstringINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEcEET_PFiPT0_mPKS8_P13__va_list_tagEmSB_z \
 // RUN:             -e _ZZNSt19_Sp_make_shared_tag5_S_tiEvE5__tag \
 // RUN:             -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEDnEEEvv \
-// RUN:             -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA1_cEEEvv \
-// RUN:             -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA8_cEEEvv \
-// RUN:             -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA24_cEEEvv \
-// RUN:             -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA32_cEEEvv \
-// RUN:             -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA40_cEEEvv \
-// RUN:             -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA80_cEEEvv \
-// RUN:             -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA88_cEEEvv \
-// RUN:             -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA104_cEEEvv \
-// RUN:             -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA160_cEEEvv \
-// RUN:             -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA168_cEEEvv \
-// RUN:             -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA216_cEEEvv \
+// RUN:             -e '_ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA[0-9]\+_cEEEvv' \
 // RUN:             -e _ZN9__gnu_cxx12__to_xstringISscEET_PFiPT0_mPKS2_P13__va_list_tagEmS5_z \
 // RUN:             -e  _ZZNSt8__detail18__to_chars_10_implIjEEvPcjT_E8__digits \
 // RUN:             -e  _ZZNSt8__detail18__to_chars_10_implImEEvPcjT_E8__digits \