[libcxx-commits] [libcxx] [libc++] Fix exception safety of `__hash_table::__copy_construct` (avoid memory leak) (PR #201452)
Nikita Taranov via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Jun 4 03:14:16 PDT 2026
https://github.com/nickitat updated https://github.com/llvm/llvm-project/pull/201452
>From 8f9e2b86a5ae797081890ebf53c2aa0b98748d27 Mon Sep 17 00:00:00 2001
From: Nikita Taranov <nikita.taranov at clickhouse.com>
Date: Wed, 3 Jun 2026 20:50:38 +0000
Subject: [PATCH 1/4] impl
---
libcxx/include/__hash_table | 9 ++
.../unord.map.cnstr/exceptions.pass.cpp | 91 +++++++++++++++++++
2 files changed, 100 insertions(+)
create mode 100644 libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/exceptions.pass.cpp
diff --git a/libcxx/include/__hash_table b/libcxx/include/__hash_table
index e1264703f6b18..21c7baeb055f5 100644
--- a/libcxx/include/__hash_table
+++ b/libcxx/include/__hash_table
@@ -41,6 +41,7 @@
#include <__type_traits/is_swappable.h>
#include <__type_traits/remove_const.h>
#include <__type_traits/remove_cvref.h>
+#include <__utility/exception_guard.h>
#include <__utility/exchange.h>
#include <__utility/forward.h>
#include <__utility/move.h>
@@ -697,6 +698,13 @@ private:
_LIBCPP_HIDE_FROM_ABI void __copy_construct(__next_pointer __other_iter) {
__next_pointer __own_iter = __first_node_.__ptr();
+ // If copying a node throws, the nodes that have already been constructed and linked into the
+ // list have to be deallocated. When this is called from the copy constructor the enclosing
+ // __hash_table is not fully constructed yet, so its destructor would not run to release them.
+ auto __guard = std::__make_exception_guard([&] {
+ __deallocate_node_list(__first_node_.__next_);
+ __first_node_.__next_ = nullptr;
+ });
{
__node_holder __new_node = __construct_node_hash(__other_iter->__hash(), __other_iter->__upcast()->__get_value());
__own_iter->__next_ = static_cast<__next_pointer>(__new_node.release());
@@ -707,6 +715,7 @@ private:
__other_iter = __other_iter->__next_;
__own_iter = __own_iter->__next_;
__copy_construct(__other_iter, __own_iter, __current_chash);
+ __guard.__complete();
}
public:
diff --git a/libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/exceptions.pass.cpp b/libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/exceptions.pass.cpp
new file mode 100644
index 0000000000000..27cfcf20a9266
--- /dev/null
+++ b/libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/exceptions.pass.cpp
@@ -0,0 +1,91 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: no-exceptions
+
+// <unordered_map>
+
+// Check that the unordered_map copy constructor does not leak nodes when an allocation
+// throws partway through copying the elements. The enclosing container is not fully
+// constructed yet, so its destructor does not run to release the already-copied nodes.
+
+#include <cassert>
+#include <cstddef>
+#include <functional>
+#include <memory>
+#include <new>
+#include <unordered_map>
+#include <utility>
+
+#include "count_new.h"
+#include "test_macros.h"
+
+// An allocator that throws std::bad_alloc once a shared countdown reaches zero. It forwards
+// to std::allocator (operator new) so that count_new.h can observe leaked nodes.
+template <class T>
+struct CountedThrowingAllocator {
+ using value_type = T;
+
+ int* countdown_;
+
+ explicit CountedThrowingAllocator(int& countdown) : countdown_(&countdown) {}
+
+ template <class U>
+ CountedThrowingAllocator(const CountedThrowingAllocator<U>& other) : countdown_(other.countdown_) {}
+
+ T* allocate(std::size_t n) {
+ if (*countdown_ == 0)
+ throw std::bad_alloc();
+ --*countdown_;
+ return std::allocator<T>().allocate(n);
+ }
+
+ void deallocate(T* p, std::size_t n) { std::allocator<T>().deallocate(p, n); }
+
+ template <class U>
+ friend bool operator==(const CountedThrowingAllocator&, const CountedThrowingAllocator<U>&) {
+ return true;
+ }
+
+ template <class U>
+ friend bool operator!=(const CountedThrowingAllocator&, const CountedThrowingAllocator<U>&) {
+ return false;
+ }
+};
+
+int main(int, char**) {
+ using Alloc = CountedThrowingAllocator<std::pair<const int, int> >;
+ using Map = std::unordered_map<int, int, std::hash<int>, std::equal_to<int>, Alloc>;
+
+ const int never_throw = 1 << 30;
+ int countdown = never_throw;
+
+ // Build a source map large enough that copying it performs several node allocations.
+ Map src((Alloc(countdown)));
+ for (int i = 0; i < 16; ++i)
+ src.emplace(i, i);
+
+ // For every point at which an allocation can fail during the copy, verify that the
+ // partially-constructed copy does not leak any nodes. The allocator is propagated by
+ // select_on_container_copy_construction, so it shares the same countdown.
+ for (int fail_at = 0; fail_at < 32; ++fail_at) {
+ const int outstanding_before = globalMemCounter.outstanding_new;
+
+ countdown = fail_at;
+ try {
+ Map copy(src);
+ (void)copy;
+ } catch (const std::bad_alloc&) {
+ }
+ countdown = never_throw;
+
+ assert(globalMemCounter.outstanding_new == outstanding_before);
+ }
+
+ return 0;
+}
>From a4c7221b3158f389134504477b3910aa81af182f Mon Sep 17 00:00:00 2001
From: Nikita Taranov <nikita.taranov at clickhouse.com>
Date: Wed, 3 Jun 2026 20:59:41 +0000
Subject: [PATCH 2/4] fix style
---
.../unord/unord.map/unord.map.cnstr/exceptions.pass.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/exceptions.pass.cpp b/libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/exceptions.pass.cpp
index 27cfcf20a9266..5c4d7f50c3b2f 100644
--- a/libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/exceptions.pass.cpp
+++ b/libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/exceptions.pass.cpp
@@ -76,7 +76,7 @@ int main(int, char**) {
for (int fail_at = 0; fail_at < 32; ++fail_at) {
const int outstanding_before = globalMemCounter.outstanding_new;
- countdown = fail_at;
+ countdown = fail_at;
try {
Map copy(src);
(void)copy;
>From 383a3b81288dc961118de62424acc01ead528488 Mon Sep 17 00:00:00 2001
From: Nikita Taranov <nikita.taranov at clickhouse.com>
Date: Wed, 3 Jun 2026 21:17:57 +0000
Subject: [PATCH 3/4] fix frozen C++03 headers
---
.../unord/unord.map/unord.map.cnstr/exceptions.pass.cpp | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/exceptions.pass.cpp b/libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/exceptions.pass.cpp
index 5c4d7f50c3b2f..0f881faccd191 100644
--- a/libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/exceptions.pass.cpp
+++ b/libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/exceptions.pass.cpp
@@ -8,6 +8,10 @@
// UNSUPPORTED: no-exceptions
+// The frozen C++03 headers use a separate __hash_table that is not affected by this fix, and
+// std::unordered_map::emplace is not available there.
+// UNSUPPORTED: c++03
+
// <unordered_map>
// Check that the unordered_map copy constructor does not leak nodes when an allocation
>From 2c8f96124f7d94326e91b6da1419deced36a0934 Mon Sep 17 00:00:00 2001
From: Nikita Taranov <nikita.taranov at clickhouse.com>
Date: Thu, 4 Jun 2026 10:14:04 +0000
Subject: [PATCH 4/4] run test for c++03
---
.../unord.map.cnstr/exceptions.pass.cpp | 28 +++++++++----------
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/exceptions.pass.cpp b/libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/exceptions.pass.cpp
index 0f881faccd191..4904582ef9c66 100644
--- a/libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/exceptions.pass.cpp
+++ b/libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/exceptions.pass.cpp
@@ -8,15 +8,11 @@
// UNSUPPORTED: no-exceptions
-// The frozen C++03 headers use a separate __hash_table that is not affected by this fix, and
-// std::unordered_map::emplace is not available there.
-// UNSUPPORTED: c++03
-
// <unordered_map>
-// Check that the unordered_map copy constructor does not leak nodes when an allocation
-// throws partway through copying the elements. The enclosing container is not fully
-// constructed yet, so its destructor does not run to release the already-copied nodes.
+// Check that the unordered_map copy constructor does not leak nodes when a node allocation
+// throws partway through copying the elements. The already-copied nodes must be released even
+// though the container itself never finishes constructing.
#include <cassert>
#include <cstddef>
@@ -33,7 +29,7 @@
// to std::allocator (operator new) so that count_new.h can observe leaked nodes.
template <class T>
struct CountedThrowingAllocator {
- using value_type = T;
+ typedef T value_type;
int* countdown_;
@@ -63,20 +59,22 @@ struct CountedThrowingAllocator {
};
int main(int, char**) {
- using Alloc = CountedThrowingAllocator<std::pair<const int, int> >;
- using Map = std::unordered_map<int, int, std::hash<int>, std::equal_to<int>, Alloc>;
+ typedef CountedThrowingAllocator<std::pair<const int, int> > Alloc;
+ typedef std::unordered_map<int, int, std::hash<int>, std::equal_to<int>, Alloc> Map;
const int never_throw = 1 << 30;
int countdown = never_throw;
- // Build a source map large enough that copying it performs several node allocations.
+ // Build a source map large enough that copying it performs several node allocations. The
+ // allocator is propagated by select_on_container_copy_construction, so the copy shares this
+ // same countdown.
Map src((Alloc(countdown)));
for (int i = 0; i < 16; ++i)
- src.emplace(i, i);
+ src.insert(std::make_pair(i, i));
- // For every point at which an allocation can fail during the copy, verify that the
- // partially-constructed copy does not leak any nodes. The allocator is propagated by
- // select_on_container_copy_construction, so it shares the same countdown.
+ // Make a node allocation fail at every position partway through the copy and verify that no
+ // nodes are leaked. `outstanding_before` captures the allocations held by the source so the
+ // check is unaffected by it.
for (int fail_at = 0; fail_at < 32; ++fail_at) {
const int outstanding_before = globalMemCounter.outstanding_new;
More information about the libcxx-commits
mailing list