Skip to content

Memory Leak in VariableLengthArrayBase Move Constructor with Allocator When is_pocma_or_is_always_equal<Alloc> is false_type #187

Description

@JLReitz

See relevant test case with failing results below:

using VLAWithNoPropagateOnMoveAllocator =
    cetl::VariableLengthArray<
            InstrumentedType,
            cetlvast::InstrumentedNewDeleteAllocator<
                InstrumentedType,
                std::false_type,  // Is always equal
                std::false_type,  // Is equal
                std::false_type,  // Propagate on container move assignment
                std::true_type    // Propagate on container copy assignment
            >
        >;
using VLAWithNoPropagateOnMoveAllocatorTests = VLADetailedAllocationTests<VLAWithNoPropagateOnMoveAllocator>;
TEST_F(VLAWithNoPropagateOnMoveAllocatorTests, MoveConstructWithNewAllocator)
{
    using TestFixture = VLAWithNoPropagateOnMoveAllocatorTests;
    using VlaType = VLAWithNoPropagateOnMoveAllocator;
    using AllocatorType = VlaType::allocator_type;

    VlaType test_source{{1, 2, 3, 4}, AllocatorType{}};
    VlaType test_subject{std::move(test_source), AllocatorType{}};

    EXPECT_EQ(12, TestFixture::ItemT::total_instances_constructed);
    EXPECT_EQ(4, TestFixture::ItemT::total_instances_implicit_int_constructed); // Test source init list instances
    EXPECT_EQ(4, TestFixture::ItemT::total_instances_copy_constructed); // Test source instances
    EXPECT_EQ(4, TestFixture::ItemT::total_instances_move_constructed); // Test subject instances
    EXPECT_EQ(2, cetlvast::InstrumentedAllocatorStatistics::get().allocations);
    EXPECT_EQ(2, cetlvast::InstrumentedAllocatorStatistics::get().deallocations);
}
[----------] 1 test from VLAWithNonMovePropagationAllocatorTests
[ RUN      ] VLAWithNonMovePropagationAllocatorTests.MoveConstructWithNewAllocator
/repo/cetlvast/suites/unittest/test_variable_length_array_detailed_allocation.cpp:754: Failure
Expected equality of these values:
  2
  cetlvast::InstrumentedAllocatorStatistics::get().deallocations
    Which is: 1

/repo/cetlvast/suites/unittest/test_variable_length_array_detailed_allocation.cpp:152: Failure
Expected equality of these values:
  0
  InstrumentedType::instance_counter
    Which is: 4

[  FAILED  ] VLAWithNonMovePropagationAllocatorTests.MoveConstructWithNewAllocator (0 ms)
[----------] 1 test from VLAWithNonMovePropagationAllocatorTests (0 ms total)

The move constructor with allocator of VariableLengthArrayBase, invoked when the allocator does not propagate on container move assignment and is not equivalent to the right-hand side's allocator, sets the right hand side's data pointer to nullptr before its data is deallocated. When the right hand side is eventually destructed, at the end of the test case, deallocation is skipped because the data pointer is nullptr.

template <typename UAlloc>
constexpr VariableLengthArrayBase(
    VariableLengthArrayBase&& rhs,
    const UAlloc&             rhs_alloc,
    typename std::enable_if_t<!is_pocma_or_is_always_equal<UAlloc>::value>* = nullptr) noexcept
    : alloc_(std::allocator_traits<UAlloc>::select_on_container_copy_construction(rhs_alloc))
    , data_{nullptr}
    , capacity_(0)
    , size_(0)
    , max_size_max_(rhs.max_size_max_)
{
    static_assert(std::is_nothrow_copy_constructible<UAlloc>::value,
                  "Allocator must be nothrow copy constructible.");
    if (alloc_ == rhs.alloc_)
    {
        // The allocators may not always be equal, but they are this time.
        data_     = std::move(rhs.data_);
        capacity_ = rhs.capacity_;
        size_     = rhs.size_;
    }
    else
    {
        // The allocators are not equal, so we need to move the data over
        // manually.
        if (rhs.size_ > 0)
        {
            data_ = std::allocator_traits<allocator_type>::allocate(alloc_, rhs.size_);
            fast_forward_construct(data_, rhs.size_, rhs.data_, rhs.size_, alloc_);
        }
        capacity_ = rhs.capacity_;
        size_     = rhs.size_;

        // <- Need to deallocate rhs.data_ here

    }
    rhs.size_     = 0;
    rhs.capacity_ = 0;
    rhs.data_     = nullptr;
}

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions