TEMP_BUFFER Copy/Move Problem

[Larry Watterson 485]

In C++, if you write a user-declared destructor (which you usually do in resource-managing classes), the compiler implicitly declares the copy constructor and copy assignment operator.

`TEMP_BUFFER` manages a resource (has-a `LPBUFFER`), and returns that resource in its destructor (the idiom known as RAII, by Bjarne).

But if you have a resource-managing class and you use the compiler-generated copy special member functions, different things can happen to you depending on the scenario.

For our case, let's assume we wrote code like this:

 

TEMP_BUFFER b1, b2;
b1 = b2;

The compiler-generated copy-assign does a shallow copy: `b1`'s old buffer gets overwritten with `b2.buf` without its destructor ever running (i.e. without `buffer_delete` being called). Since `b1.buf == b2.buf`, the same corruption that would occur in the copy-ctor happens here too. So assign = leak + corruption.

In the copy-ctor, as far as I can see, there's only corruption. Two objects share the same `buf`, and in the destructor that `buf` goes to `buffer_delete` twice. If the size fits in the pool, `buffer_delete` doesn't call `free`; it puts the buffer back on the free list. When the same `buf` is pushed onto the list twice, logically `buf->next = buf`, meaning the next node points to itself. Then `buffer_new` hands out the same `buf` to two separate calls, and you can potentially run into something like a heap overflow or an infinite loop. Classic corruption, blowing up in some unrelated place, is my guess.

The simplest fix is to explicitly delete the copy-ctor and copy-assign operator as a preventive measure; I doubt anyone in their right mind would copy a `TEMP_BUFFER` anyway.

 

TEMP_BUFFER(const TEMP_BUFFER&) = delete;
TEMP_BUFFER& operator=(const TEMP_BUFFER&) = delete;

But if you want following the rule of five:

TEMP_BUFFER::TEMP_BUFFER(const TEMP_BUFFER& other)
    : buf{(other.buf != nullptr) ? buffer_new(other.buf->mem_size) : nullptr}
    , forceDelete{other.forceDelete}
{
    if (other.buf != nullptr)
    {
        buffer_write(buf, other.buf->mem_data, other.buf->write_point_pos);
    }
}

TEMP_BUFFER& TEMP_BUFFER::operator=(const TEMP_BUFFER& other)
{
    if (this != &other)
    {
        if (buf != nullptr)
        {
            buffer_delete(buf);
        }
        forceDelete = other.forceDelete;
        buf = (other.buf != nullptr) ? buffer_new(other.buf->mem_size) : nullptr;
        if (other.buf != nullptr)
        {
            buffer_write(buf, other.buf->mem_data, other.buf->write_point_pos);
        }
    }
    return *this;
}

TEMP_BUFFER::TEMP_BUFFER(TEMP_BUFFER&& other) noexcept
    : buf{std::exchange(other.buf, nullptr)}
    , forceDelete{std::exchange(other.forceDelete, false)}
{
}

TEMP_BUFFER& TEMP_BUFFER::operator=(TEMP_BUFFER&& other) noexcept
{
    if (this != &other)
    {
        if (buf != nullptr)
        {
            buffer_delete(buf);
        }

        buf = std::exchange(other.buf, nullptr);
        forceDelete = std::exchange(other.forceDelete, false);
    }
    return *this;
}

Note: this deep copy only copy over the written data; `read_point` and the flag are not copied, so it doesn't preserve the full state of a half-read buffer. Since it's used for writing when building packets it's not a problem, but use it knowing this.

Note 2: as I said, it's not a critical thing, you've most likely never copied the buffer at all. But keep similar situations in mind for other resource-managing classes as well.

One warning: when there's a user-declared destructor, the move ctor and move assign operator are not implicitly declared by the compiler, meaning once you delete the copies the type becomes entirely non-movable. If you use it somewhere that `std::move`s the buffer (like a fluent API), you'll need to write the move ctor/assign yourself, I've written an example above, but take a look before using it, there might be spots in the buffer logic I missed; if there are, mention them in the comments and i'll fix it. Deleted copy move and assign operator alone is enough for pure prevention, but if it were me I'd write a more reasonable, modern buffer mechanism.

 

Btw, in the copy assignment operator, you can prefer the copy-and-swap idiom. I wrote the special member functions simply, just as examples, so don't focus on them too much. If you are going to use them, the implementation is up to you. Don't act without checking the APIs like buffer_new, etc. The responsibility is yours.

 

mock: https://godbolt.org/z/YaM9EreeW

Enjoy.

Edited July 1 by Larry Watterson

[CONTROL 917]

Hi, thx for pointing that out.

I was wondering why not simply delete the copy constructor and copy assignment operator if copying isn't actually needed or used (at least from what I've seen), and make TEMP_BUFFER a move-only type instead?

Since it owns a unique buffer, move-only feels like a more natural fit to me.

Is there any place where copy semantics are actually required ?

[Syreldar 2736]

On 7/1/2026 at 4:59 PM, Larry Watterson said:

In C++, if you write a user-declared destructor (which you usually do in resource-managing classes), the compiler implicitly declares the copy constructor and copy assignment operator.

`TEMP_BUFFER` manages a resource (has-a `LPBUFFER`), and returns that resource in its destructor (the idiom known as RAII, by Bjarne).

But if you have a resource-managing class and you use the compiler-generated copy special member functions, different things can happen to you depending on the scenario.

For our case, let's assume we wrote code like this:

 

TEMP_BUFFER b1, b2;
b1 = b2;

The compiler-generated copy-assign does a shallow copy: `b1`'s old buffer gets overwritten with `b2.buf` without its destructor ever running (i.e. without `buffer_delete` being called). Since `b1.buf == b2.buf`, the same corruption that would occur in the copy-ctor happens here too. So assign = leak + corruption.

In the copy-ctor, as far as I can see, there's only corruption. Two objects share the same `buf`, and in the destructor that `buf` goes to `buffer_delete` twice. If the size fits in the pool, `buffer_delete` doesn't call `free`; it puts the buffer back on the free list. When the same `buf` is pushed onto the list twice, logically `buf->next = buf`, meaning the next node points to itself. Then `buffer_new` hands out the same `buf` to two separate calls, and you can potentially run into something like a heap overflow or an infinite loop. Classic corruption, blowing up in some unrelated place, is my guess.

The simplest fix is to explicitly delete the copy-ctor and copy-assign operator as a preventive measure; I doubt anyone in their right mind would copy a `TEMP_BUFFER` anyway.

 

TEMP_BUFFER(const TEMP_BUFFER&) = delete;
TEMP_BUFFER& operator=(const TEMP_BUFFER&) = delete;

But if you want following the rule of five:

TEMP_BUFFER::TEMP_BUFFER(const TEMP_BUFFER& other)
    : buf{(other.buf != nullptr) ? buffer_new(other.buf->mem_size) : nullptr}
    , forceDelete{other.forceDelete}
{
    if (other.buf != nullptr)
    {
        buffer_write(buf, other.buf->mem_data, other.buf->write_point_pos);
    }
}

TEMP_BUFFER& TEMP_BUFFER::operator=(const TEMP_BUFFER& other)
{
    if (this != &other)
    {
        if (buf != nullptr)
        {
            buffer_delete(buf);
        }
        forceDelete = other.forceDelete;
        buf = (other.buf != nullptr) ? buffer_new(other.buf->mem_size) : nullptr;
        if (other.buf != nullptr)
        {
            buffer_write(buf, other.buf->mem_data, other.buf->write_point_pos);
        }
    }
    return *this;
}

TEMP_BUFFER::TEMP_BUFFER(TEMP_BUFFER&& other) noexcept
    : buf{std::exchange(other.buf, nullptr)}
    , forceDelete{std::exchange(other.forceDelete, false)}
{
}

TEMP_BUFFER& TEMP_BUFFER::operator=(TEMP_BUFFER&& other) noexcept
{
    if (this != &other)
    {
        if (buf != nullptr)
        {
            buffer_delete(buf);
        }

        buf = std::exchange(other.buf, nullptr);
        forceDelete = std::exchange(other.forceDelete, false);
    }
    return *this;
}

Note: this deep copy only copy over the written data; `read_point` and the flag are not copied, so it doesn't preserve the full state of a half-read buffer. Since it's used for writing when building packets it's not a problem, but use it knowing this.

Note 2: as I said, it's not a critical thing, you've most likely never copied the buffer at all. But keep similar situations in mind for other resource-managing classes as well.

One warning: when there's a user-declared destructor, the move ctor and move assign operator are not implicitly declared by the compiler, meaning once you delete the copies the type becomes entirely non-movable. If you use it somewhere that `std::move`s the buffer (like a fluent API), you'll need to write the move ctor/assign yourself, I've written an example above, but take a look before using it, there might be spots in the buffer logic I missed; if there are, mention them in the comments and i'll fix it. Deleted copy move and assign operator alone is enough for pure prevention, but if it were me I'd write a more reasonable, modern buffer mechanism.

 

Btw, in the copy assignment operator, you can prefer the copy-and-swap idiom. I wrote the special member functions simply, just as examples, so don't focus on them too much. If you are going to use them, the implementation is up to you. Don't act without checking the APIs like buffer_new, etc. The responsibility is yours.

 

mock: https://godbolt.org/z/YaM9EreeW

Enjoy.

Very useful, many thanks for the heads-up.

Something to point out is that the problem is not the destructor by itself as much as it is the combination of an owning raw pointer plus compiler-generated memberwise copying. A destructor still allows implicit copy operations, but prevents implicit move operation.

Also, In copy assignment, saying the destructor does not run is.. technically true, but slightly imprecise: assignment never invokes the destination object's destructor. The bug is that its currently owned buffer is overwritten without an explicit buffer_delete, creating a leak before the eventual double-return/corruption.

The analysis is accurate. After the same pooled buffer is deleted twice:

pool[pi] = b;
b->next = b;

The first later buffer_new() returns b while leaving pool[pi] still pointing at it, meaning it resets b->next to nullptr and the second buffer_new() then returns that exact same b again. So two apparently separate owners receive one allocation.

The LS result in the mock is expected, but it is not directly detecting the double-return. x and y are raw pointers that are never passed to buffer_delete; because both refer to the same allocation, LS reports one buffer object plus its 8192-byte payload as leaked. Simply adding assert(x != y) or something similiar makes the allocator corruption visible immediately.

Mock: https://godbolt.org/z/cTbYj6vPP

For a prevention-only fix, deleting copies and implementing moves is the right choice:

TEMP_BUFFER(const TEMP_BUFFER&) = delete;
TEMP_BUFFER& operator=(const TEMP_BUFFER&) = delete;

TEMP_BUFFER(TEMP_BUFFER&& other) noexcept
    : buf(std::exchange(other.buf, nullptr))
    , forceDelete(std::exchange(other.forceDelete, false))
{
}

TEMP_BUFFER& operator=(TEMP_BUFFER&& other) noexcept
{
    if (this != &other)
    {
        buffer_delete(buf);

        buf = std::exchange(other.buf, nullptr);
        forceDelete = std::exchange(other.forceDelete, false);
    }

    return *this;
}

but the deep-copy assignment example you proposed has one meaningful weakness: it deletes the current buffer before allocating and filling the replacement. If allocation or buffer_write can fail, buf may remain dangling or the object may be left partially modified.

A copy-and-swap approach is preferable:

void swap(TEMP_BUFFER& other) noexcept
{
    using std::swap;

    swap(buf, other.buf);
    swap(forceDelete, other.forceDelete);
}

TEMP_BUFFER& operator=(const TEMP_BUFFER& other)
{
    if (this != &other)
    {
        TEMP_BUFFER copy(other);
        swap(copy);
    }

    return *this;
}

That is only safe if the copy constructor itself correctly handles allocation/write failure according to the real buffer_new and buffer_write contracts, and the deep-copy constructor must preserve every semantically relevant buffer field.
Copying only write_point_pos is fine only if TEMP_BUFFER intentionally represents a fresh writable snapshot. If it can wrap a partly-read packet, it should also preserve read position and any other state that affects interpretation.

21 hours ago, CONTROL said:

Hi, thx for pointing that out.

I was wondering why not simply delete the copy constructor and copy assignment operator if copying isn't actually needed or used (at least from what I've seen), and make TEMP_BUFFER a move-only type instead?

Since it owns a unique buffer, move-only feels like a more natural fit to me.

Is there any place where copy semantics are actually required ?

If TEMP_BUFFER has unique ownership and no meaningful use case where you have to duplicate a buffer, making it move-only is the better design imo.

struct TEMP_BUFFER
{
    LPBUFFER buf{nullptr};
    bool forceDelete{false};

    TEMP_BUFFER(const TEMP_BUFFER&) = delete;
    TEMP_BUFFER& operator=(const TEMP_BUFFER&) = delete;

    TEMP_BUFFER(TEMP_BUFFER&& other) noexcept
        : buf(std::exchange(other.buf, nullptr))
        , forceDelete(std::exchange(other.forceDelete, false))
    {
    }

    TEMP_BUFFER& operator=(TEMP_BUFFER&& other) noexcept
    {
        if (this != &other)
        {
            buffer_delete(buf);

            buf = std::exchange(other.buf, nullptr);
            forceDelete = std::exchange(other.forceDelete, false);
        }

        return *this;
    }

    ~TEMP_BUFFER()
    {
        buffer_delete(buf);
    }
};

Copy semantics are only required where code genuinely needs an independent duplicate, for example:

• TEMP_BUFFER b = a;
• Passing an lvalue TEMP_BUFFER by value.
• Storing it in an API or container operation that copies elements.
• Returning or assigning from an lvalue where a move is not requested.

Normal return-by-value code does not usually require copying because NRVO or move construction handles it. Standard containers can also hold move-only types, provided the element is movable and preferably noexcept movable.

Edited July 3 by Syreldar