Skip to content

make_uninitialized_space_new() leaks the already relocated elements when a move throws #74

Description

@martinus

Found while fixing #68, which it is adjacent to but not covered by.

When an insert has to grow, make_uninitialized_space_new() relocates into a fresh vector in two steps:

auto target = svector();
target.reserve(s + count);

auto* target_pos = std::uninitialized_move(data<D>(), p, target.data<indirect>());   // [begin, pos)
std::uninitialized_move(p, data<D>() + s, target_pos + count);                       // [pos, end)

target.set_size<indirect>(s + count);   // only reached on success
*this = std::move(target);

If the second std::uninitialized_move throws, it destroys what it built itself, but the elements the first one already moved into target are never destroyed. target's size is still 0, so its destructor frees the allocation without touching them.

auto v = ankerl::svector<ThrowOnMove, 4>();
for (int i = 0; i < 4; ++i) { v.emplace_back("aaaa...40 chars..."); }   // now full

ThrowOnMove::budget = 2;                    // the 3rd move throws
v.insert(v.cbegin() + 2, ThrowOnMove("bbbb..."));
size=4 capacity=4 (full, next insert reallocates)
caught: move
size after = 4
Direct leak of 82 byte(s) in 2 object(s)
SUMMARY: AddressSanitizer: 82 byte(s) leaked in 2 allocation(s).

Exactly the two elements moved before the throw. The source vector keeps its 4 elements, so the container itself stays valid — this is a leak, not corruption.

Only reachable with a throwing move constructor, which is why it survived: is_nothrow_move_constructible_v<T> is true for almost everything real, and until #63 svector claimed a noexcept move regardless.

Fix

The two moves need to be wrapped so that a failure destroys whatever landed in target before rethrowing. Setting target's size incrementally would do it too, since then its own destructor cleans up.

Related, larger

std::vector avoids this whole family of problems by never creating a raw gap: it assigns into the region that already holds live objects and only constructs into genuinely new slots, so a throw always leaves valid objects everywhere and no unwinding cleanup is needed.

Doing the same here means reworking shift_right() so the new value is threaded through the shift instead of applied to a hole afterwards, which also lets assignable types like std::string reuse their existing buffers instead of destroy-then-construct. That would replace both #68's rollback and this leak with a design where neither can happen. It is a much larger change and belongs on its own.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions