Fix race condition in iterator debug machinery (#2060)

miscco · AdamBucior · CaseyCarter · web-flow · commit 37b0ce0ce385 · 2021-07-30T00:25:40.000-07:00
Co-authored-by: Adam Bucior &lt;35536269+AdamBucior@users.noreply.github.com&gt;
Co-authored-by: Casey Carter &lt;cacarter@microsoft.com&gt;
Co-authored-by: Stephan T. Lavavej &lt;stl@microsoft.com&gt;
diff --git a/stl/inc/regex b/stl/inc/regex
@@ -2516,8 +2516,8 @@ public:
                 _MyRe = nullptr;
 
 #if _ITERATOR_DEBUG_LEVEL == 2
-                this->_Orphan_me_v2();
-#endif // _ITERATOR_DEBUG_LEVEL
+                this->_Adopt(nullptr);
+#endif // _ITERATOR_DEBUG_LEVEL == 2
 
                 return *this;
             }
diff --git a/stl/inc/xmemory b/stl/inc/xmemory
@@ -1120,12 +1120,12 @@ public:
     _Container_proxy* _Myproxy = nullptr;
 
 private:
-    _CONSTEXPR20 void _Orphan_all_unlocked() noexcept;
+    _CONSTEXPR20 void _Orphan_all_unlocked_v3() noexcept;
     _CONSTEXPR20 void _Swap_proxy_and_iterators_unlocked(_Container_base12&) noexcept;
 
-    void _Orphan_all_locked() noexcept {
+    void _Orphan_all_locked_v3() noexcept {
         _Lockit _Lock(_LOCK_DEBUG);
-        _Orphan_all_unlocked();
+        _Orphan_all_unlocked_v3();
     }
 
     void _Swap_proxy_and_iterators_locked(_Container_base12& _Right) noexcept {
@@ -1143,56 +1143,43 @@ public:
     }
 
     _CONSTEXPR20 _Iterator_base12& operator=(const _Iterator_base12& _Right) noexcept {
-        if (_Myproxy != _Right._Myproxy) {
-            if (_Right._Myproxy) {
-                _Adopt(_Right._Myproxy->_Mycont);
-            } else { // becoming invalid, disown current parent
 #if _ITERATOR_DEBUG_LEVEL == 2
-                _Orphan_me_v2();
-#else // _ITERATOR_DEBUG_LEVEL == 2
-                _Myproxy = nullptr;
-#endif // _ITERATOR_DEBUG_LEVEL == 2
-            }
+#if _HAS_CXX20
+        if (_STD is_constant_evaluated()) {
+            _Assign_unlocked(_Right);
+        } else
+#endif // _HAS_CXX20
+        {
+            _Assign_locked(_Right);
         }
+#else // ^^^ _ITERATOR_DEBUG_LEVEL == 2 ^^^ / vvv _ITERATOR_DEBUG_LEVEL != 2 vvv
+        _Myproxy = _Right._Myproxy;
+#endif // _ITERATOR_DEBUG_LEVEL != 2
         return *this;
     }
 
 #if _ITERATOR_DEBUG_LEVEL == 2
     _CONSTEXPR20 ~_Iterator_base12() noexcept {
-        _Orphan_me_v2();
-    }
-
-    _CONSTEXPR20 void _Adopt(const _Container_base12* _Parent) noexcept {
-        if (_Parent) { // have a parent, do adoption
-            _Container_proxy* _Parent_proxy = _Parent->_Myproxy;
-            if (_Myproxy != _Parent_proxy) { // change parentage
 #if _HAS_CXX20
-                if (_STD is_constant_evaluated()) {
-                    _Adopt_unlocked(_Parent_proxy);
-                } else
+        if (_STD is_constant_evaluated()) {
+            _Orphan_me_unlocked_v3();
+        } else
 #endif // _HAS_CXX20
-                {
-                    _Adopt_locked(_Parent_proxy);
-                }
-            }
-        } else { // no future parent, just disown current parent
-            _Orphan_me_v2();
+        {
+            _Orphan_me_locked_v3();
         }
     }
 
-    _CONSTEXPR20 void _Orphan_me_v2() noexcept {
-        if (_Myproxy) { // adopted, remove self from list
+    _CONSTEXPR20 void _Adopt(const _Container_base12* _Parent) noexcept {
 #if _HAS_CXX20
-            if (_STD is_constant_evaluated()) {
-                _Orphan_me_unlocked();
-            } else
+        if (_STD is_constant_evaluated()) {
+            _Adopt_unlocked(_Parent);
+        } else
 #endif // _HAS_CXX20
-            {
-                _Orphan_me_locked();
-            }
+        {
+            _Adopt_locked(_Parent);
         }
     }
-
 #else // ^^^ _ITERATOR_DEBUG_LEVEL == 2 ^^^ / vvv _ITERATOR_DEBUG_LEVEL != 2 vvv
     _CONSTEXPR20 void _Adopt(const _Container_base12* _Parent) noexcept {
         if (_Parent) { // have a parent, do adoption
@@ -1214,21 +1201,51 @@ public:
 
 #if _ITERATOR_DEBUG_LEVEL == 2
 private:
-    _CONSTEXPR20 void _Adopt_unlocked(_Container_proxy* _Parent_proxy) noexcept {
-        if (_Myproxy) { // adopted, remove self from list
-            _Orphan_me_unlocked();
+    _CONSTEXPR20 void _Assign_unlocked(const _Iterator_base12& _Right) noexcept {
+        if (_Myproxy == _Right._Myproxy) {
+            return;
+        }
+
+        if (_Right._Myproxy) {
+            _Adopt_unlocked(_Right._Myproxy->_Mycont);
+        } else { // becoming invalid, disown current parent
+            _Orphan_me_unlocked_v3();
+        }
+    }
+
+    void _Assign_locked(const _Iterator_base12& _Right) noexcept {
+        _Lockit _Lock(_LOCK_DEBUG);
+        _Assign_unlocked(_Right);
+    }
+
+    _CONSTEXPR20 void _Adopt_unlocked(const _Container_base12* _Parent) noexcept {
+        if (!_Parent) {
+            _Orphan_me_unlocked_v3();
+            return;
+        }
+
+        _Container_proxy* _Parent_proxy = _Parent->_Myproxy;
+        if (_Myproxy != _Parent_proxy) { // change parentage
+            if (_Myproxy) { // adopted, remove self from list
+                _Orphan_me_unlocked_v3();
+            }
+            _Mynextiter                 = _Parent_proxy->_Myfirstiter;
+            _Parent_proxy->_Myfirstiter = this;
+            _Myproxy                    = _Parent_proxy;
         }
-        _Mynextiter                 = _Parent_proxy->_Myfirstiter;
-        _Parent_proxy->_Myfirstiter = this;
-        _Myproxy                    = _Parent_proxy;
     }
 
-    void _Adopt_locked(_Container_proxy* _Parent_proxy) noexcept {
+    void _Adopt_locked(const _Container_base12* _Parent) noexcept {
         _Lockit _Lock(_LOCK_DEBUG);
-        _Adopt_unlocked(_Parent_proxy);
+        _Adopt_unlocked(_Parent);
     }
 
-    _CONSTEXPR20 void _Orphan_me_unlocked() noexcept {
+    _CONSTEXPR20 void _Orphan_me_unlocked_v3() noexcept {
+        if (!_Myproxy) { // already orphaned
+            return;
+        }
+
+        // adopted, remove self from list
         _Iterator_base12** _Pnext = &_Myproxy->_Myfirstiter;
         while (*_Pnext && *_Pnext != this) {
             const auto _Temp = *_Pnext; // TRANSITION, VSO-1269037
@@ -1240,15 +1257,20 @@ private:
         _Myproxy = nullptr;
     }
 
-    void _Orphan_me_locked() noexcept {
+    void _Orphan_me_locked_v3() noexcept {
         _Lockit _Lock(_LOCK_DEBUG);
-        _Orphan_me_unlocked();
+        _Orphan_me_unlocked_v3();
     }
 #endif // _ITERATOR_DEBUG_LEVEL == 2
 };
 
 // MEMBER FUNCTIONS FOR _Container_base12
-_CONSTEXPR20 void _Container_base12::_Orphan_all_unlocked() noexcept {
+_CONSTEXPR20 void _Container_base12::_Orphan_all_unlocked_v3() noexcept {
+    if (!_Myproxy) { // no proxy, already done
+        return;
+    }
+
+    // proxy allocated, drain it
     for (auto& _Pnext = _Myproxy->_Myfirstiter; _Pnext; _Pnext = _Pnext->_Mynextiter) { // TRANSITION, VSO-1269037
         _Pnext->_Myproxy = nullptr;
     }
@@ -1257,15 +1279,13 @@ _CONSTEXPR20 void _Container_base12::_Orphan_all_unlocked() noexcept {
 
 _CONSTEXPR20 void _Container_base12::_Orphan_all() noexcept {
 #if _ITERATOR_DEBUG_LEVEL == 2
-    if (_Myproxy) { // proxy allocated, drain it
 #if _HAS_CXX20
-        if (_STD is_constant_evaluated()) {
-            _Orphan_all_unlocked();
-        } else
+    if (_STD is_constant_evaluated()) {
+        _Orphan_all_unlocked_v3();
+    } else
 #endif // _HAS_CXX20
-        {
-            _Orphan_all_locked();
-        }
+    {
+        _Orphan_all_locked_v3();
     }
 #endif // _ITERATOR_DEBUG_LEVEL == 2
 }
diff --git a/tests/std/test.lst b/tests/std/test.lst
@@ -177,6 +177,7 @@ tests\GH_001541_case_sensitive_boolalpha
 tests\GH_001638_dllexport_derived_classes
 tests\GH_001850_clog_tied_to_cout
 tests\GH_002039_byte_is_not_trivially_swappable
+tests\GH_002058_debug_iterator_race
 tests\LWG2597_complex_branch_cut
 tests\LWG3018_shared_ptr_function
 tests\P0019R8_atomic_ref
diff --git a/tests/std/tests/GH_002058_debug_iterator_race/env.lst b/tests/std/tests/GH_002058_debug_iterator_race/env.lst
@@ -0,0 +1,4 @@
+# Copyright (c) Microsoft Corporation.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+RUNALL_INCLUDE ..\native_matrix.lst
diff --git a/tests/std/tests/GH_002058_debug_iterator_race/test.cpp b/tests/std/tests/GH_002058_debug_iterator_race/test.cpp
@@ -0,0 +1,54 @@
+// Copyright (c) Microsoft Corporation.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#include <future>
+#include <list>
+#include <set>
+#include <vector>
+
+using namespace std;
+
+// Concurrently destroy and invalidate iterators
+template <class Container>
+void test_concurrent_destruction() {
+    Container c;
+    c.insert(c.begin(), 0);
+
+    vector<typename Container::iterator> iters(1000, c.begin());
+    {
+        auto destroyIters = async(launch::async, [&]() { iters.clear(); });
+
+        auto invalidateIters = async(launch::async, [&]() { c.clear(); });
+    }
+}
+
+// Concurrently create and invalidate iterators
+template <class Container>
+void test_concurrent_creation() {
+    Container c;
+    c.insert(c.begin(), 0);
+
+    vector<typename Container::iterator> iters;
+    iters.reserve(1000);
+
+    const auto iter = c.begin();
+    {
+        auto copyIters = async(launch::async, [&]() {
+            for (int i = 0; i < 1000; ++i) {
+                iters.push_back(iter);
+            };
+        });
+
+        auto invalidateIters = async(launch::async, [&]() { c.clear(); });
+    }
+}
+
+int main() {
+    test_concurrent_destruction<list<int>>();
+    test_concurrent_destruction<set<int>>();
+    test_concurrent_destruction<vector<int>>();
+
+    test_concurrent_creation<list<int>>();
+    test_concurrent_creation<set<int>>();
+    test_concurrent_creation<vector<int>>();
+}

Original file line number	Diff line number	Diff line change
`@@ -2516,8 +2516,8 @@ public:`
`2516`	`2516`	`_MyRe = nullptr;`
`2517`	`2517`
`2518`	`2518`	`#if _ITERATOR_DEBUG_LEVEL == 2`
`2519`		`- this->_Orphan_me_v2();`
`2520`		`-#endif // _ITERATOR_DEBUG_LEVEL`
	`2519`	`+ this->_Adopt(nullptr);`
	`2520`	`+#endif // _ITERATOR_DEBUG_LEVEL == 2`
`2521`	`2521`
`2522`	`2522`	`return *this;`
`2523`	`2523`	`}`