Skip to content

Commit

Permalink
Fix some subtle UB found by MSan. (#4093)
Browse files Browse the repository at this point in the history
Technically, any small size buffer's lifetime has ended by the time we
get to the base's destructor. This means its no longer valid to access
table contents if stored there from the base destructor. We need to
handle destruction in the table class instead.

This ends up being a trivial change because the logic is already
factored out, we just need to call it from a different point.
  • Loading branch information
chandlerc authored Jun 29, 2024
1 parent 3f78e1d commit 6310293
Showing 1 changed file with 10 additions and 6 deletions.
16 changes: 10 additions & 6 deletions common/raw_hashtable.h
Original file line number Diff line number Diff line change
Expand Up @@ -442,7 +442,10 @@ class BaseImpl {
: view_impl_(alloc_size, nullptr),
growth_budget_(growth_budget),
small_alloc_size_(small_alloc_size) {}
~BaseImpl();

// Destruction must be handled by the table where it can destroy entries in
// any small buffer, so make the base destructor protected but defaulted here.
~BaseImpl() = default;

// NOLINTNEXTLINE(google-explicit-constructor): Designed to implicitly decay.
operator ViewImplT() const { return view_impl(); }
Expand Down Expand Up @@ -575,6 +578,7 @@ class TableImpl : public InputBaseT {
TableImpl(TableImpl&& arg) noexcept;
auto operator=(const TableImpl& arg) -> TableImpl&;
auto operator=(TableImpl&& arg) noexcept -> TableImpl&;
~TableImpl();

// Resets the hashtable to its initial state, clearing all entries and
// releasing all memory. If the hashtable had an SSO buffer, that is restored
Expand Down Expand Up @@ -841,11 +845,6 @@ auto ViewImpl<InputKeyT, InputValueT, InputKeyContextT>::ComputeMetricsImpl(
return metrics;
}

template <typename InputKeyT, typename InputValueT, typename InputKeyContextT>
BaseImpl<InputKeyT, InputValueT, InputKeyContextT>::~BaseImpl() {
Destroy();
}

// TODO: Evaluate whether it is worth forcing this out-of-line given the
// reasonable ABI boundary it forms and large volume of code necessary to
// implement it.
Expand Down Expand Up @@ -1596,6 +1595,11 @@ auto TableImpl<InputBaseT, SmallSize>::operator=(TableImpl&& arg) noexcept
return *this;
}

template <typename InputBaseT, ssize_t SmallSize>
TableImpl<InputBaseT, SmallSize>::~TableImpl() {
this->Destroy();
}

// Reset a table to its original state, including releasing any allocated
// memory.
template <typename InputBaseT, ssize_t SmallSize>
Expand Down

0 comments on commit 6310293

Please sign in to comment.