From 6ddf630bdecb4dca709129212524096b8b53b63f Mon Sep 17 00:00:00 2001 From: Aitor Esteve Alvarado Date: Mon, 26 Jan 2026 09:45:18 +0000 Subject: [PATCH 1/3] Fix UB with aligned_storage_for --- immer/detail/arrays/node.hpp | 14 +++--- immer/detail/hamts/node.hpp | 44 +++++++++--------- immer/detail/rbts/node.hpp | 23 +++++----- immer/detail/util.hpp | 86 ++++++++++++++++++++++++++++++++---- 4 files changed, 118 insertions(+), 49 deletions(-) diff --git a/immer/detail/arrays/node.hpp b/immer/detail/arrays/node.hpp index ac521e03..1aee01ab 100644 --- a/immer/detail/arrays/node.hpp +++ b/immer/detail/arrays/node.hpp @@ -31,10 +31,8 @@ struct node using node_t = node; using edit_t = typename transience::edit; - struct data_t - { - aligned_storage_for buffer; - }; + struct data_t : public has_trailing_storage + {}; using impl_t = combine_standard_layout_t; @@ -42,7 +40,9 @@ struct node constexpr static std::size_t sizeof_n(size_t count) { - return std::max(immer_offsetof(impl_t, d.buffer) + sizeof(T) * count, + return std::max(immer_offsetof(impl_t, d) + + decltype(impl_t::d)::get_storage_offset() + + sizeof(T) * count, sizeof(node)); } @@ -51,8 +51,8 @@ struct node const ownee_t& ownee() const { return get(impl); } ownee_t& ownee() { return get(impl); } - const T* data() const { return reinterpret_cast(&impl.d.buffer); } - T* data() { return reinterpret_cast(&impl.d.buffer); } + const T* data() const { return impl.d.get_storage_ptr(); } + T* data() { return impl.d.get_storage_ptr(); } bool can_mutate(edit_t e) const { diff --git a/immer/detail/hamts/node.hpp b/immer/detail/hamts/node.hpp index a59954da..a133f3ee 100644 --- a/immer/detail/hamts/node.hpp +++ b/immer/detail/hamts/node.hpp @@ -65,25 +65,21 @@ struct node inner }; - struct collision_t + struct collision_t : public has_trailing_storage { count_t count; - aligned_storage_for buffer; }; - struct values_data_t - { - aligned_storage_for buffer; - }; + struct values_data_t : public has_trailing_storage + {}; using values_t = combine_standard_layout_t; - struct inner_t + struct inner_t : public has_trailing_storage { bitmap_t nodemap; bitmap_t datamap; values_t* values; - aligned_storage_for buffer; }; union data_t @@ -107,20 +103,24 @@ struct node constexpr static std::size_t sizeof_values_n(count_t count) { return std::max(sizeof(values_t), - immer_offsetof(values_t, d.buffer) + - sizeof(values_data_t::buffer) * count); + immer_offsetof(values_t, d) + + values_data_t::get_storage_offset() + + sizeof(typename values_data_t::storage_type) * + count); } constexpr static std::size_t sizeof_collision_n(count_t count) { - return immer_offsetof(impl_t, d.data.collision.buffer) + - sizeof(collision_t::buffer) * count; + return immer_offsetof(impl_t, d.data.collision) + + collision_t::get_storage_offset() + + sizeof(typename collision_t::storage_type) * count; } constexpr static std::size_t sizeof_inner_n(count_t count) { - return immer_offsetof(impl_t, d.data.inner.buffer) + - sizeof(inner_t::buffer) * count; + return immer_offsetof(impl_t, d.data.inner) + + inner_t::get_storage_offset() + + sizeof(typename inner_t::storage_type) * count; } #if IMMER_TAGGED_NODE @@ -131,26 +131,26 @@ struct node { IMMER_ASSERT_TAGGED(kind() == kind_t::inner); assert(impl.d.data.inner.values); - return (T*) &impl.d.data.inner.values->d.buffer; + return impl.d.data.inner.values->d.get_storage_ptr(); } auto values() const { IMMER_ASSERT_TAGGED(kind() == kind_t::inner); assert(impl.d.data.inner.values); - return (const T*) &impl.d.data.inner.values->d.buffer; + return (const T*) impl.d.data.inner.values->d.get_storage_ptr(); } auto children() { IMMER_ASSERT_TAGGED(kind() == kind_t::inner); - return (node_t**) &impl.d.data.inner.buffer; + return impl.d.data.inner.get_storage_ptr(); } auto children() const { IMMER_ASSERT_TAGGED(kind() == kind_t::inner); - return (const node_t* const*) &impl.d.data.inner.buffer; + return (const node_t* const*) impl.d.data.inner.get_storage_ptr(); } auto datamap() const @@ -198,13 +198,13 @@ struct node T* collisions() { IMMER_ASSERT_TAGGED(kind() == kind_t::collision); - return (T*) &impl.d.data.collision.buffer; + return impl.d.data.collision.get_storage_ptr(); } const T* collisions() const { IMMER_ASSERT_TAGGED(kind() == kind_t::collision); - return (const T*) &impl.d.data.collision.buffer; + return impl.d.data.collision.get_storage_ptr(); } static refs_t& refs(const values_t* x) @@ -386,7 +386,7 @@ struct node else { auto nv = data_count(); auto nxt = new (heap::allocate(sizeof_values_n(nv))) values_t{}; - auto dst = (T*) &nxt->d.buffer; + auto dst = nxt->d.get_storage_ptr(); auto src = values(); ownee(nxt) = e; IMMER_TRY { @@ -1084,7 +1084,7 @@ struct node static void delete_values(values_t* p, count_t n) { assert(p); - detail::destroy_n((T*) &p->d.buffer, n); + detail::destroy_n(p->d.get_storage_ptr(), n); deallocate_values(p, n); } diff --git a/immer/detail/rbts/node.hpp b/immer/detail/rbts/node.hpp index df41ac18..bc1c4065 100644 --- a/immer/detail/rbts/node.hpp +++ b/immer/detail/rbts/node.hpp @@ -72,15 +72,12 @@ struct node relaxed_data_no_meta_t, relaxed_data_with_meta_t>; - struct leaf_t - { - aligned_storage_for buffer; - }; + struct leaf_t : public has_trailing_storage + {}; - struct inner_t + struct inner_t : public has_trailing_storage { relaxed_t* relaxed; - aligned_storage_for buffer; }; union data_t @@ -108,14 +105,16 @@ struct node constexpr static std::size_t sizeof_packed_leaf_n(count_t count) { - return immer_offsetof(impl_t, d.data.leaf.buffer) + - sizeof(leaf_t::buffer) * count; + return immer_offsetof(impl_t, d.data.leaf) + + leaf_t::get_storage_offset() + + sizeof(typename leaf_t::storage_type) * count; } constexpr static std::size_t sizeof_packed_inner_n(count_t count) { - return immer_offsetof(impl_t, d.data.inner.buffer) + - sizeof(inner_t::buffer) * count; + return immer_offsetof(impl_t, d.data.inner) + + inner_t::get_storage_offset() + + sizeof(typename inner_t::storage_type) * count; } constexpr static std::size_t sizeof_packed_relaxed_n(count_t count) @@ -184,13 +183,13 @@ struct node node_t** inner() { IMMER_ASSERT_TAGGED(kind() == kind_t::inner); - return reinterpret_cast(&impl.d.data.inner.buffer); + return impl.d.data.inner.get_storage_ptr(); } T* leaf() { IMMER_ASSERT_TAGGED(kind() == kind_t::leaf); - return reinterpret_cast(&impl.d.data.leaf.buffer); + return impl.d.data.leaf.get_storage_ptr(); } static refs_t& refs(const relaxed_t* x) diff --git a/immer/detail/util.hpp b/immer/detail/util.hpp index 3c8c0070..423db012 100644 --- a/immer/detail/util.hpp +++ b/immer/detail/util.hpp @@ -52,6 +52,76 @@ template using aligned_storage_for = typename aligned_storage::type; +template +struct has_trailing_storage; + +template +struct alignas(alignof(T)) has_trailing_storage +{ + using storage_type = T; + + T* get_storage_ptr() + { + check_base(); + return reinterpret_cast(this); + } + + const T* get_storage_ptr() const + { + check_base(); + return reinterpret_cast(this); + } + + static constexpr size_t get_storage_offset() + { + check_base(); + return 0; + } + +private: + static constexpr void check_base() + { + static_assert(std::is_standard_layout::value, + "Please remove 'true' if the base class is non emtpy"); + } + + // Dummy field to make the base class non-empty. + char _; +}; + +template +struct alignas(alignof(T)) has_trailing_storage +{ + using storage_type = T; + + T* get_storage_ptr() + { + check_base(); + auto* base = static_cast(this); + return reinterpret_cast(base + 1); + } + + const T* get_storage_ptr() const + { + check_base(); + auto* base = static_cast(this); + return reinterpret_cast(base + 1); + } + + static constexpr size_t get_storage_offset() + { + check_base(); + return sizeof(Base); + } + +private: + static constexpr void check_base() + { + static_assert(std::is_standard_layout::value && !std::is_empty::value, + "Please add 'true' if the base class is emtpy"); + } +}; + template T& auto_const_cast(const T& x) { @@ -287,13 +357,13 @@ distance(Iterator first, Sentinel last) * forward range @f$ [first, last) @f$ */ template ) &&detail:: is_forward_iterator_v && - detail::compatible_sentinel_v && - (!detail::is_subtractable_v), - bool> = true> + detail::compatible_sentinel_v && + (!detail::is_subtractable_v), + bool> = true> typename std::iterator_traits::difference_type distance(Iterator first, Sentinel last) { @@ -310,13 +380,13 @@ distance(Iterator first, Sentinel last) * random access range @f$ [first, last) @f$ */ template ) &&detail:: is_forward_iterator_v && - detail::compatible_sentinel_v && - detail::is_subtractable_v, - bool> = true> + detail::compatible_sentinel_v && + detail::is_subtractable_v, + bool> = true> typename std::iterator_traits::difference_type distance(Iterator first, Sentinel last) { From 30322a857377f311fc0cdd046dda392f9378bbc4 Mon Sep 17 00:00:00 2001 From: Aitor Esteve Alvarado Date: Mon, 26 Jan 2026 10:12:19 +0000 Subject: [PATCH 2/3] Revert formatting change --- immer/detail/util.hpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/immer/detail/util.hpp b/immer/detail/util.hpp index 423db012..1c185a48 100644 --- a/immer/detail/util.hpp +++ b/immer/detail/util.hpp @@ -357,13 +357,13 @@ distance(Iterator first, Sentinel last) * forward range @f$ [first, last) @f$ */ template ) &&detail:: is_forward_iterator_v && - detail::compatible_sentinel_v && - (!detail::is_subtractable_v), - bool> = true> + detail::compatible_sentinel_v && + (!detail::is_subtractable_v), + bool> = true> typename std::iterator_traits::difference_type distance(Iterator first, Sentinel last) { @@ -380,13 +380,13 @@ distance(Iterator first, Sentinel last) * random access range @f$ [first, last) @f$ */ template ) &&detail:: is_forward_iterator_v && - detail::compatible_sentinel_v && - detail::is_subtractable_v, - bool> = true> + detail::compatible_sentinel_v && + detail::is_subtractable_v, + bool> = true> typename std::iterator_traits::difference_type distance(Iterator first, Sentinel last) { From bf8bf2bce6b653e18c57ea3387a057a8e817dd1a Mon Sep 17 00:00:00 2001 From: Aitor Esteve Alvarado Date: Tue, 27 Jan 2026 09:40:39 +0000 Subject: [PATCH 3/3] Address review comments --- immer/detail/arrays/node.hpp | 2 +- immer/detail/hamts/node.hpp | 6 ++-- immer/detail/rbts/node.hpp | 4 +-- immer/detail/util.hpp | 58 +++++++++++++++++++++++------------- 4 files changed, 43 insertions(+), 27 deletions(-) diff --git a/immer/detail/arrays/node.hpp b/immer/detail/arrays/node.hpp index 1aee01ab..9aa33aee 100644 --- a/immer/detail/arrays/node.hpp +++ b/immer/detail/arrays/node.hpp @@ -31,7 +31,7 @@ struct node using node_t = node; using edit_t = typename transience::edit; - struct data_t : public has_trailing_storage + struct data_t : public with_trailing_storage {}; using impl_t = combine_standard_layout_t; diff --git a/immer/detail/hamts/node.hpp b/immer/detail/hamts/node.hpp index a133f3ee..b9b71d6e 100644 --- a/immer/detail/hamts/node.hpp +++ b/immer/detail/hamts/node.hpp @@ -65,17 +65,17 @@ struct node inner }; - struct collision_t : public has_trailing_storage + struct collision_t : public with_trailing_storage { count_t count; }; - struct values_data_t : public has_trailing_storage + struct values_data_t : public with_trailing_storage {}; using values_t = combine_standard_layout_t; - struct inner_t : public has_trailing_storage + struct inner_t : public with_trailing_storage { bitmap_t nodemap; bitmap_t datamap; diff --git a/immer/detail/rbts/node.hpp b/immer/detail/rbts/node.hpp index bc1c4065..a2181ffd 100644 --- a/immer/detail/rbts/node.hpp +++ b/immer/detail/rbts/node.hpp @@ -72,10 +72,10 @@ struct node relaxed_data_no_meta_t, relaxed_data_with_meta_t>; - struct leaf_t : public has_trailing_storage + struct leaf_t : public with_trailing_storage {}; - struct inner_t : public has_trailing_storage + struct inner_t : public with_trailing_storage { relaxed_t* relaxed; }; diff --git a/immer/detail/util.hpp b/immer/detail/util.hpp index 1c185a48..2d45f4bc 100644 --- a/immer/detail/util.hpp +++ b/immer/detail/util.hpp @@ -52,11 +52,22 @@ template using aligned_storage_for = typename aligned_storage::type; -template -struct has_trailing_storage; +/*! + * CRTP class that allows using the storage immediately following an instance of + * the derived class to store objects of type `T` (i.e. "trailing storage"). + * + * The class is guaranteed to be standard layout if the derived class is also + * standard_layout. + * + * `EmptyStruct` should be set to `true` if the derived class is empty, this + * allows the optimization of using the storage of the derived class as trailing + * storage. + */ +template +struct with_trailing_storage; -template -struct alignas(alignof(T)) has_trailing_storage +template +struct alignas(alignof(T)) with_trailing_storage { using storage_type = T; @@ -81,44 +92,49 @@ struct alignas(alignof(T)) has_trailing_storage private: static constexpr void check_base() { - static_assert(std::is_standard_layout::value, - "Please remove 'true' if the base class is non emtpy"); + // is_standard_layout requires that only one class in the hierarchy + // contains non-static data members. Since this class contains one + // member, the static_assert will only hold when the derived class is + // empty. + static_assert(std::is_standard_layout::value, + "Please remove 'true' if the derived class is non emtpy"); } // Dummy field to make the base class non-empty. char _; }; -template -struct alignas(alignof(T)) has_trailing_storage +template +struct alignas(alignof(T)) with_trailing_storage { using storage_type = T; T* get_storage_ptr() { check_base(); - auto* base = static_cast(this); + auto* base = static_cast(this); return reinterpret_cast(base + 1); } const T* get_storage_ptr() const { check_base(); - auto* base = static_cast(this); + auto* base = static_cast(this); return reinterpret_cast(base + 1); } static constexpr size_t get_storage_offset() { check_base(); - return sizeof(Base); + return sizeof(Derived); } private: static constexpr void check_base() { - static_assert(std::is_standard_layout::value && !std::is_empty::value, - "Please add 'true' if the base class is emtpy"); + static_assert(std::is_standard_layout::value && + !std::is_empty::value, + "Please add 'true' if the derived class is emtpy"); } }; @@ -357,13 +373,13 @@ distance(Iterator first, Sentinel last) * forward range @f$ [first, last) @f$ */ template ) &&detail:: is_forward_iterator_v && - detail::compatible_sentinel_v && - (!detail::is_subtractable_v), - bool> = true> + detail::compatible_sentinel_v && + (!detail::is_subtractable_v), + bool> = true> typename std::iterator_traits::difference_type distance(Iterator first, Sentinel last) { @@ -380,13 +396,13 @@ distance(Iterator first, Sentinel last) * random access range @f$ [first, last) @f$ */ template ) &&detail:: is_forward_iterator_v && - detail::compatible_sentinel_v && - detail::is_subtractable_v, - bool> = true> + detail::compatible_sentinel_v && + detail::is_subtractable_v, + bool> = true> typename std::iterator_traits::difference_type distance(Iterator first, Sentinel last) {