Skip to content

Fix issue #274 UBSan Error constructor call with insufficient space for an object of type 'node_t' #323

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
arximboldi merged 3 commits into from
Jan 29, 2026
Merged

Fix issue #274 UBSan Error constructor call with insufficient space for an object of type 'node_t' #323

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6ddf630
Fix UB with aligned_storage_for
atesteve Jan 26, 2026
30322a8
Revert formatting change
atesteve Jan 26, 2026
bf8bf2b
Address review comments
atesteve Jan 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions immer/detail/arrays/node.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,18 +31,18 @@ struct node
using node_t = node;
using edit_t = typename transience::edit;

struct data_t
{
aligned_storage_for<T> buffer;
};
struct data_t : public has_trailing_storage<data_t, T, true>
{};

using impl_t = combine_standard_layout_t<data_t, refs_t, ownee_t>;

impl_t impl;

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));
}

Expand All @@ -51,8 +51,8 @@ struct node
const ownee_t& ownee() const { return get<ownee_t>(impl); }
ownee_t& ownee() { return get<ownee_t>(impl); }

const T* data() const { return reinterpret_cast<const T*>(&impl.d.buffer); }
T* data() { return reinterpret_cast<T*>(&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
{
Expand Down
44 changes: 22 additions & 22 deletions immer/detail/hamts/node.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,25 +65,21 @@ struct node
inner
};

struct collision_t
struct collision_t : public has_trailing_storage<collision_t, T>
{
count_t count;
aligned_storage_for<T> buffer;
};

struct values_data_t
{
aligned_storage_for<T> buffer;
};
struct values_data_t : public has_trailing_storage<values_data_t, T, true>
{};

using values_t = combine_standard_layout_t<values_data_t, refs_t, ownee_t>;

struct inner_t
struct inner_t : public has_trailing_storage<inner_t, node_t*>
{
bitmap_t nodemap;
bitmap_t datamap;
values_t* values;
aligned_storage_for<node_t*> buffer;
};

union data_t
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
}

Expand Down
23 changes: 11 additions & 12 deletions immer/detail/rbts/node.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,15 +72,12 @@ struct node
relaxed_data_no_meta_t,
relaxed_data_with_meta_t>;

struct leaf_t
{
aligned_storage_for<T> buffer;
};
struct leaf_t : public has_trailing_storage<leaf_t, T, true>
{};

struct inner_t
struct inner_t : public has_trailing_storage<inner_t, node_t*>
{
relaxed_t* relaxed;
aligned_storage_for<node_t*> buffer;
};

union data_t
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -184,13 +183,13 @@ struct node
node_t** inner()
{
IMMER_ASSERT_TAGGED(kind() == kind_t::inner);
return reinterpret_cast<node_t**>(&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<T*>(&impl.d.data.leaf.buffer);
return impl.d.data.leaf.get_storage_ptr();
}

static refs_t& refs(const relaxed_t* x)
Expand Down
70 changes: 70 additions & 0 deletions immer/detail/util.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,76 @@ template <typename T>
using aligned_storage_for =
typename aligned_storage<sizeof(T), alignof(T)>::type;

template <typename Base, typename T, bool EmptyBase = false>
struct has_trailing_storage;

@arximboldi arximboldi Jan 26, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: the has_ verb makes it looks like this is a meta-predicate (a boolean returning meta-function). For "mix-ins" of this form I prefer the with_ prefix, as in:

struct with_trailing_storage;

@arximboldi arximboldi Jan 26, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also a comment explaining why and how this is needed would be nice to have.

@atesteve atesteve Jan 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, with_trailing_storage is a much better name. I added some comments, let me know if you are OK with them or if you wanted to explain why this is needed instead of the original aligned_storage solution.


template <typename Base, typename T>

@arximboldi arximboldi Jan 26, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Base should be rename the Deriv, as it is not the base clase but the class that derives from this (technically, with_trailing_storage is the base class of Deriv as usual in CRTP)

@atesteve atesteve Jan 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha true, I did it completely backwards. Fixed.

struct alignas(alignof(T)) has_trailing_storage<Base, T, true>
{
using storage_type = T;

T* get_storage_ptr()
{
check_base();
return reinterpret_cast<T*>(this);
}

const T* get_storage_ptr() const
{
check_base();
return reinterpret_cast<const T*>(this);
}

static constexpr size_t get_storage_offset()
{
check_base();
return 0;
}

private:
static constexpr void check_base()
{
static_assert(std::is_standard_layout<Base>::value,
"Please remove 'true' if the base class is non emtpy");

@arximboldi arximboldi Jan 26, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty neat trick to discover when this still holds :)

}

// Dummy field to make the base class non-empty.
char _;
};

template <typename Base, typename T>
struct alignas(alignof(T)) has_trailing_storage<Base, T, false>
{
using storage_type = T;

T* get_storage_ptr()
{
check_base();
auto* base = static_cast<Base*>(this);
return reinterpret_cast<T*>(base + 1);
}

const T* get_storage_ptr() const
{
check_base();
auto* base = static_cast<const Base*>(this);
return reinterpret_cast<const T*>(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<Base>::value && !std::is_empty<Base>::value,
"Please add 'true' if the base class is emtpy");
}
};

template <typename T>
T& auto_const_cast(const T& x)
{
Expand Down
Loading