abseil/abseil-cpp

[Bug]: inlined_vector implicitly assumes Pointer<A> is a native pointer type

Opened this issue · 0 comments

Describe the issue

https://godbolt.org/z/9zYjbax8v

While working on #1615, I (thought I) noticed some places in absl::inlined_vector where it was more-or-less memcpying the union of (data, pointer-to-heap-allocation) guarded only under a check that the value type was trivially relocatable, i.e., it was failing to check whether the allocator's pointer type was also trivially relocatable. An example of a non-trivial pointer type is Boost's offset_ptr.

I tried to make a reproducer for that issue (expecting that I'd be able to show inlined_vector actually misbehaving at runtime, because memcpying an offset_ptr would trash its value) — but then I ran into a bigger issue: inlined_vector calls allocator_traits<A>::construct with a Pointer<A> argument instead of a native pointer. This flat-out doesn't compile, when Pointer<A> isn't a native pointer. This effectively prevents people from using inlined_vector with Boost.Interprocess... but that's a good thing, because if the code actually compiled (and if I'm not mistaken), it would Do The Wrong Thing at runtime by memcpying offset_ptrs!

The simple fix would be to add static_assert(std::is_trivially_copyable_v<Pointer<A>>); to inlined_vector's body, just to make the compiler error more user-friendly and "intentional."
The complicated/difficult fix would be to audit the whole inlined_vector codebase and fix any place that isn't offset_ptr-safe. I don't know how many places this would be.

Steps to reproduce the problem

https://godbolt.org/z/9zYjbax8v

#include <boost/interprocess/offset_ptr.hpp>
#include <boost/interprocess/segment_manager.hpp>
#include <boost/interprocess/managed_shared_memory.hpp>
#include <absl/container/inlined_vector.h>
#include <vector>

namespace bip = boost::interprocess;
template<class T> using shm_allocator = bip::allocator<T, bip::managed_shared_memory::segment_manager>;
template<class T> using shm_std_vector = std::vector<T, shm_allocator<T>>;
template<class T> using shm_absl_vector = absl::InlinedVector<T, 10, shm_allocator<T>>;

static_assert(!absl::is_trivially_relocatable<shm_std_vector<int>::pointer>::value);
static_assert(!absl::is_trivially_relocatable<shm_absl_vector<int>::pointer>::value);
  // This is the key problem: absl::InlinedVector implicitly assumes its pointer
  // type is always trivially relocatable, but in fact it might not be.
  // This should at least be static_asserted/mandated.
  // Ideally, you'd do a code audit of InlinedVector and fix all the places that
  // need fixing (i.e. any place that assumes Pointer<A> is memcpyable), so that
  // the following code would Just Work. I do *NOT* recommend just sprinkling in
  // calls to `std::to_address(ptr)` until the `reserve` call compiles; I think
  // it's a good thing that it fails noisily at compile time, instead of
  // *for all we know* misbehaving at runtime by trying to memcpy an offset_ptr.

void reserve_some(shm_std_vector<int>& v) {
    v.reserve(100); // OK
}

void reserve_some(shm_absl_vector<int>& v) {
    v.reserve(100);
      // Error in call to allocator_traits::construct:
      // can't match 'offset_ptr<int>' against 'T*'
}

What version of Abseil are you using?

trunk, i.e. 4358cb2 as of this writing

What operating system and version are you using?

Any

What compiler and version are you using?

Any

What build system are you using?

Any

Additional context

No response