Detected memory leaks in shared_ptr copy constructor

Question

All the code is included in this post. For some reason, it tells me that, a memory leak is happening at shared_ptr.hpp(60), assuming this must be line 60 here. But, why exactly? This happens is uncertain.

Anyone that can spot the issue, please let me know for certain why, and what the best fix is here. I been spending all day yesterday, with no results of whatsoever.

It is here, according to the leaked output:

explicit SharedPtr(T* other) : _ptr{ other }, _ctrl_block{new ControlBlock<T>}
        {
            __increment_reference();
            CHECK 
        }

LEAK:

Detected memory leaks!
Dumping objects ->
C:\Users\root\cpp\ptrs\ptrs\shared_ptr.hpp(60) : {731} normal block at 0x000001B15AE83130, 8 bytes long.
 Data: <        > 00 00 00 00 01 00 00 00 
C:\Users\root\cpp\ptrs\ptrs\shared_ptr.hpp(60) : {727} normal block at 0x000001B15AE830E0, 8 bytes long.
 Data: <        > 00 00 00 00 01 00 00 00 
C:\Users\root\cpp\ptrs\ptrs\shared_ptr.hpp(60) : {721} normal block at 0x000001B15AE83090, 8 bytes long.
 Data: <        > 00 00 00 00 01 00 00 00 
C:\Users\root\cpp\ptrs\ptrs\shared_ptr.hpp(60) : {717} normal block at 0x000001B15AE82E60, 8 bytes long.
 Data: <        > 00 00 00 00 01 00 00 00 
C:\Users\root\cpp\ptrs\ptrs\shared_ptr.hpp(60) : {688} normal block at 0x000001B15AE83040, 8 bytes long.
 Data: <        > 00 00 00 00 01 00 00 00 
C:\Users\root\cpp\ptrs\ptrs\shared_ptr.hpp(60) : {686} normal block at 0x000001B15AE82FF0, 8 bytes long.
 Data: <        > 00 00 00 00 01 00 00 00 
C:\Users\root\cpp\ptrs\ptrs\shared_ptr.hpp(60) : {684} normal block at 0x000001B15AE82F50, 8 bytes long.
 Data: <        > 00 00 00 00 01 00 00 00 
Object dump complete.
The program '[34840] ptrs.exe' has exited with code 0 (0x0).

controlblock.hpp

template<class T>
struct ControlBlock
{
    int _ref_count;
    int _weak_count;
    void inc_ref() noexcept { ++_ref_count; }
    void inc_wref() noexcept { ++_weak_count; }
    void dec_ref() noexcept { --_ref_count; }
    void dec_wref() noexcept { if (--_weak_count == 0) delete this; }
    int use_count() const noexcept { return _ref_count; }
    ControlBlock() : _ref_count{ 0 }, _weak_count{ 0 } {}
    ~ControlBlock() { _ref_count = 0; _weak_count = 0; }
};

shared_ptr.hpp

#pragma once

#include <algorithm>
#include <iterator>
#include <compare>
#include <cassert>
#include <memory>

#include "controlblock.hpp"
#include "weak_ptr.hpp"

template<class T>
class SharedPtr {
#define CHECK assert(Invariant());
    template<class Y> friend class WeakPtr;
private:
    T* _ptr;
    ControlBlock<T>* _ctrl_block;

    void __increment_reference()
    {
        if (_ptr != nullptr && _ctrl_block != nullptr)
                _ctrl_block->inc_ref();
    }

    void __remove_reference()
    {
        if (_ctrl_block && _ctrl_block->_ref_count > 0) {
            --_ctrl_block->_ref_count;
            if(_ctrl_block && _ctrl_block->_ref_count == 0){
                delete _ptr;
            }
            
            if(_ctrl_block->_ref_count + _ctrl_block->_weak_count == 0) {
                delete _ctrl_block;
            }
            
            _ptr = nullptr;
            _ctrl_block = nullptr;
        }
    }

public:
    constexpr SharedPtr() : _ptr{ nullptr }, _ctrl_block{ nullptr } { }
    explicit SharedPtr(T* other) : _ptr{ other }, _ctrl_block{new ControlBlock<T>}
    {
        __increment_reference();
        CHECK 
    }
    constexpr SharedPtr(const std::nullptr_t) noexcept 
        : _ptr{ nullptr }, _ctrl_block{ nullptr } { CHECK }
    
    explicit SharedPtr(WeakPtr<T>& other)
        : _ptr{other._ptr}, _ctrl_block{other._ctrl_block}
    {
        if (other.expired())
            throw std::bad_weak_ptr();
        
        _ctrl_block->inc_wref();
        CHECK
    }

    SharedPtr(const SharedPtr& other) noexcept 
        : _ptr{other._ptr}, _ctrl_block{other._ctrl_block}
    {
        __increment_reference();
        CHECK
    }

    SharedPtr(SharedPtr&& other) noexcept 
        : _ptr{ std::exchange(other._ptr, nullptr) }, 
          _ctrl_block{std::exchange(other._ctrl_block, nullptr)} 
    { 
        CHECK 
    }
    
    ~SharedPtr() { __remove_reference(); CHECK };

    SharedPtr& operator=(const SharedPtr& other) noexcept 
    {
        if (this == &other)
            return *this;

        __remove_reference();
        _ptr = other._ptr;
        _ctrl_block = other._ctrl_block;
        __increment_reference();

        CHECK
        return *this;
    }

    SharedPtr& operator=(SharedPtr&& other) noexcept
    {
        if (this == &other)
            return *this;

        __remove_reference();
        _ptr = std::exchange(other._ptr, nullptr);
        _ctrl_block = std::exchange(other._ctrl_block, nullptr);

        CHECK 
        return *this;
    }

    SharedPtr& operator=(std::nullptr_t)
    {
        if (_ptr == nullptr && _ctrl_block == nullptr)
            return *this;

        __remove_reference();
        return *this;
    }

    T* get() const noexcept { return _ptr; }
    T& operator*() const noexcept { return *_ptr; }
    T* operator->() const noexcept { return _ptr; }
    operator bool() const noexcept { return _ptr != nullptr && _ctrl_block != nullptr; }

    size_t use_count() const noexcept { return (_ctrl_block) ? _ctrl_block->use_count() : 0; }
    
    void reset() noexcept { this->__remove_reference(); }
    bool Invariant() const noexcept;

    template<class T> friend void swap(SharedPtr<T>& lhs, SharedPtr<T>& rhs) noexcept;

    friend auto operator<=>(const SharedPtr& lhs, const SharedPtr& rhs) = default;
    friend auto operator==(const SharedPtr& lhs, const SharedPtr& rhs) 
    {
        if (lhs.get() != rhs.get())
            return false;

        return (lhs.get() <=> rhs.get()) == 0;
    }
};

template<class T>
inline bool SharedPtr<T>::Invariant() const noexcept
{
    if (_ptr == nullptr && _ctrl_block == nullptr)
        return true;
    else if (_ptr != nullptr || _ctrl_block != nullptr && 
           _ctrl_block->_ref_count > 0 || _ctrl_block->_weak_count > 0)
        return true;
    
    return false;
}

template<class T> void swap(SharedPtr<T>& lhs, SharedPtr<T>& rhs) noexcept 
{
    std::swap(lhs, rhs);
}

template<class T, class ...Args>
SharedPtr<T> MakeShared(Args && ...args)
{
    return SharedPtr<T>(new T(std::forward<Args>(args)...));
}

weak_ptr.hpp

#pragma once

#include "controlblock.hpp"
#include "shared_ptr.hpp"

template<class T>
class WeakPtr {
#define CHECK assert(Invariant());
    template<class Y> friend class SharedPtr;
private:
    T* _ptr;
    ControlBlock<T>* _ctrl_block;
public:
    WeakPtr() { CHECK }
    WeakPtr(T* other) : _ptr{other}, _ctrl_block{new ControlBlock<T>()} { __increment_weakptr(); CHECK }
    
    WeakPtr(const WeakPtr& other) noexcept : _ptr{other._ptr}, _ctrl_block{other._ctrl_block}
    { 
        __increment_weakptr(); 
        CHECK 
    }

    WeakPtr(const SharedPtr<T>& other) noexcept 
        : _ptr{other._ptr}, _ctrl_block{other._ctrl_block}
    { 
        if(_ptr != nullptr && _ctrl_block != nullptr)
            _ctrl_block->inc_wref();
        CHECK 
    }

    ~WeakPtr() { __decrement_weakptr(); CHECK }

    void reset() noexcept
    {
        this->__decrement_weakptr();
    }
    
    bool expired() noexcept
    {
        if (_ctrl_block == nullptr)
            return true;

        if (_ctrl_block->_ref_count == 0)
            --_ctrl_block->_weak_count;

        if (_ctrl_block->_ref_count + _ctrl_block->_weak_count == 0) {
            delete _ctrl_block;
            _ptr = nullptr;
            _ctrl_block = nullptr;
        }

        return !_ctrl_block || _ctrl_block->_weak_count == 0;
    }

    auto lock()
    {
        return expired() ? SharedPtr<T>() : SharedPtr<T>(*this);
    }

    bool Invariant() const noexcept 
    {
        if (_ptr == nullptr)
            return true;
        return _ctrl_block->_weak_count > 0;
    }
    
    template<class T> 
    friend void swap(WeakPtr<T>& lhs, WeakPtr <T>& rhs) noexcept;

    WeakPtr& operator=(const WeakPtr& other) 
    {
        if (this != &other) {
            __decrement_weakptr();
            _ptr = other._ptr;
            _ctrl_block = other._ctrl_block;
            __increment_weakptr();
        }

        CHECK
        return *this; 
    }

    WeakPtr& operator=(const SharedPtr<T>& other) {
        _ptr = other._ptr;
        _ctrl_block = other._ctrl_block;

        if(_ptr != nullptr && _ctrl_block != nullptr)
            _ctrl_block->inc_wref();

        CHECK
        return *this;
    }

private:
    void __increment_weakptr()
    {
        if (_ctrl_block != nullptr) {
            _ctrl_block->inc_wref();
        }
    }

    void __decrement_weakptr()
    {
        if (_ctrl_block != nullptr && _ptr != nullptr) {
            if (--_ctrl_block->_weak_count == 0 && _ctrl_block->_ref_count == 0) {
                delete _ctrl_block;
                delete _ptr;
            }
        }
        _ptr = nullptr;
        _ctrl_block = nullptr;
    }
};

template<class T>
inline void swap(WeakPtr<T>& lhs, WeakPtr<T>& rhs) noexcept
{
    std::swap(lhs, rhs);
}

Reproduction with one Leak:

#include <iostream>
using std::cout;

#include <memory>
#include <cassert>

#include "shared_ptr.hpp"

void Errorx(const char* msg) {
    cout << msg << std::endl;
    __debugbreak();
}

struct Char {
    int _state = 1;
    char _c;
    ~Char() {
        if (_state != 1)
            if (_state == -1)
                Errorx("Probably trying to delete an item for the second time!");
            else if (_state == 0)
                Errorx("You are probably trying to delete an object that you have not designed!");
            else
                Errorx("Maybe you are trying to delete an object that you have not designed or something else is wrong");
        _state = -1;
    }
    Char(char c = 'x') {
        _c = c;
    }
    Char& operator=(const Char&) = delete;
};

#define TestShared SharedPtr<Char>
#define TestWeak WeakPtr<Char>

int main() {
    _CrtSetDbgFlag(_CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF);

    TestShared shrd(new Char('b'));
    TestWeak weak(shrd);
    assert(!weak.expired());
    assert(weak.Invariant());
    TestShared shrd2 = weak.lock();
    assert(shrd == shrd2);
    TestWeak weak0;
    TestShared shrd3 = weak0.lock();
    assert(weak0.expired());
    
    std::cin.get();
    return 0;
}

Answer 1

    void __decrement_weakptr()
    {
        if (_ctrl_block != nullptr && _ptr != nullptr) {
            if (--_ctrl_block->_weak_count == 0 && _ctrl_block->_ref_count == 0) {
                delete _ctrl_block;
                delete _ptr;
            }
        }
        _ptr = nullptr;
        _ctrl_block = nullptr;
    }

Weak pointers should not destroy _ptr - the last shared pointer should have done that when _ref_count became zero.

    explicit SharedPtr(WeakPtr<T>& other)
        : _ptr{other._ptr}, _ctrl_block{other._ctrl_block}
    {
        if (other.expired())
            throw std::bad_weak_ptr();
        
        _ctrl_block->inc_wref();
        CHECK
    }

Constructing a SharedPtr should not increment the weak refcount! Why aren't you using the same __increment_reference() as the other constructors?

Note also that ControlBlock::dec_wref is wrong: a zero weak refcount shouldn't immediately delete the control block. What if the strong refcount is still non-zero? You don't seem to use this method anyway, but you should either fix it or remove it.

All of your problems can be diagnosed by printing out the control block state before & after every operation. It'll be verbose but informative. This will be much easier to implement if you start using the inc/dec_(w)ref methods consistently. Just make the data members private, add an accessor for weak_count, and fix the build.

Answer 2

I've been trying to point to every bug in the code for 20 minutes.
I wasn't able to do that without rewriting ALL of your code… So I did:

// file: 'shared_ptr.hpp'
#include <memory>  // keep this
template <class T> using SharedPtr = std::shared_ptr<T>; // Add this.
// remove anything else.

The rest of the code is either redundant, or (for the most part) ill-formed.