C++ struct with reference counter causes memory leak

Question

I'm trying to implement my own struct with reference counter that is compiled under Visual Studio 2008's C++/Windows compiler. I came up with this:

struct STOP_FLAG
{
    STOP_FLAG() 
        : mRefCount(0)
        , pbStopFlag(NULL)
    {
        pbStopFlag = new (std::nothrow) volatile BOOL;
        ASSERT(pbStopFlag);
        this->grab();
    }
    ~STOP_FLAG()
    {
        this->release();
    }
    STOP_FLAG(const STOP_FLAG& s) 
        : pbStopFlag(s.pbStopFlag)
        , mRefCount(s.mRefCount)
    {
        //Copy constructor
        this->grab();
    }
    STOP_FLAG& operator = (const STOP_FLAG& s)
    {
        //Assignment operator
        if(pbStopFlag != s.pbStopFlag)
        {
            this->release();
            s.grab();

            pbStopFlag = s.pbStopFlag;
            mRefCount = s.mRefCount;
        }

        return *this;
    }

    //Helper methods
    volatile BOOL* operator->() const {return pbStopFlag;}      //x->member
    volatile BOOL& operator*() const {return *pbStopFlag;}      //*x, (*x).member
    operator volatile BOOL*() const {return pbStopFlag;}        //T* y = x;
    operator bool() const {return pbStopFlag != NULL;}          //if(x)

private:
    void grab() const 
    {
        //++mRefCount;
        ::InterlockedIncrement(&mRefCount);
    }

    void release() const
    {
        ASSERT(mRefCount > 0);
        //--mRefCount;
        LONG mCnt = ::InterlockedDecrement(&mRefCount);

        if(mCnt == 0)
        {
            ASSERT(pbStopFlag);
            if(pbStopFlag)
            {
                delete pbStopFlag;
                pbStopFlag = NULL;
            }
        }
    }
private:
    mutable volatile BOOL* pbStopFlag;
    mutable LONG mRefCount;
};

But when I test the following (running in a single thread) with a debugger:

{
    STOP_FLAG sf;

    {
        STOP_FLAG s2(sf);

        s2 = sf;

        STOP_FLAG s3;

        s3 = s2;

        STOP_FLAG s4[3];
        s4[0] = s3;
        s4[1] = s3;
        s4[2] = s3;

        STOP_FLAG s5;
        s3 = s5;
    }
}

I happen to have my new volatile BOOL operator called 6 times and delete only 5.

So where is my memory leak coming from?

EDIT: Here's an updated version after a suggestion below. It still produces the same result though:

struct _S_FLAG{
    volatile BOOL* pbStopFlag;
    LONG mRefCount;

    _S_FLAG(volatile BOOL* pb, LONG cntr)
    {
        pbStopFlag = pb;
        mRefCount = cntr;
    }
};

struct STOP_FLAG
{
    STOP_FLAG() 
        : _sf(NULL, 0)
    {
        _sf.pbStopFlag = new (std::nothrow) volatile BOOL;
        TRACE("new\n");
        ASSERT(_sf.pbStopFlag);
        this->grab();
    }
    ~STOP_FLAG()
    {
        this->release();
    }
    STOP_FLAG(const STOP_FLAG& s) 
        : _sf(s._sf.pbStopFlag, s._sf.mRefCount)
    {
        //Copy constructor
        this->grab();
    }
    STOP_FLAG& operator = (const STOP_FLAG& s)
    {
        //Assignment operator
        if(_sf.pbStopFlag != s._sf.pbStopFlag)
        {
            this->release();
            s.grab();

            _sf.pbStopFlag = s._sf.pbStopFlag;
            _sf.mRefCount = s._sf.mRefCount;
        }

        return *this;
    }

    //Helper methods
    volatile BOOL* operator->() const {return _sf.pbStopFlag;}      //x->member
    volatile BOOL& operator*() const {return *_sf.pbStopFlag;}      //*x, (*x).member
    operator volatile BOOL*() const {return _sf.pbStopFlag;}        //T* y = x;
    operator bool() const {return _sf.pbStopFlag != NULL;}          //if(x)

private:
    void grab() const 
    {
        //++mRefCount;
        ::InterlockedIncrement(&_sf.mRefCount);
    }

    void release() const
    {
        ASSERT(_sf.mRefCount > 0);
        //--mRefCount;
        LONG mCnt = ::InterlockedDecrement(&_sf.mRefCount);

        if(mCnt == 0)
        {
            ASSERT(_sf.pbStopFlag);
            if(_sf.pbStopFlag)
            {
                delete _sf.pbStopFlag;
                TRACE("delete\n");
                _sf.pbStopFlag = NULL;
            }
        }
    }
private:
    mutable _S_FLAG _sf;
};

Answer 1

A reference count needs to be shared among the logical references to the object.

Instead you're copying it.

That doesn't exactly explain the effect you observe, but it's more fundamental: you're worrying about some ungood sounds from the engine, I'm pointing out the car has square wheels so don't worry about the engine.

For example this:

STOP_FLAG s4[3];
s4[0] = s3;
s4[1] = s3;
s4[2] = s3;

Each of these properly destroys the prior shared block within each element of s4[] (which isn't actually shared with anything since each is a singular instance and is never assigned/copied anywhere). But think about what this code does:

//Assignment operator
if(pbStopFlag != s.pbStopFlag)
{
    this->release();
    s.grab();

    pbStopFlag = s.pbStopFlag;
    mRefCount = s.mRefCount; // <<== makes a one-time copy of the ref count
}

Note the comment added. Because the reference count is tied to the object and not the shared data, the tracked mRefCount per object is utterly useless. After this is done, s[0] through s[2] all point to the same shared data, but each has a different reference count. By this, a change in the reference count within s is not reflected with each object wired to the same shared data block in prior assignments or constructions.

These are the kinds of things std::shared_ptr<> and the like are built for. Further, not all "shared" pointers are alike. Reviewing the differences in how std::make_shared<> vs. actual construction of std::shared_ptr<> is worth some research, but suffice it to say one of them does something similar to what you seem to be trying here, the other manages a single allocation via what is often termed a compressed pair.

A modified version of your code that is obviously not thread-safe, but details one way you can accomplish what is explained in this answer appears below. It is by no means some silver bullet, and obviously needs work for thread-safe exchanges and such, but it documents how sharing the reference count and the data in a single shared block fixes the issue identified above:

---

Live Code Here

#include <iostream>
#include <algorithm>
#include <utility>

typedef long LONG;
typedef int BOOL;

struct STOP_FLAG
{
    STOP_FLAG()
        : shared(new Shared())
    {
    }

    ~STOP_FLAG()
    {
        shared->release();
    }

    // though not mandatory, note the use of the comma-operator to establish
    // the source object's shared data grab before constructing the new object's
    // shared member pointer. By the time the constructor body is entered the
    // grab is complete and the shared member is properly setup.
    STOP_FLAG(const STOP_FLAG& s)
        : shared((s.shared->grab(), s.shared))
    {
    }

    // assignment operator by copy/swap idiom
    STOP_FLAG& operator = (STOP_FLAG s)
    {
        std::swap(shared, s.shared);
        return *this;
    }

private:
    struct Shared
    {
        BOOL value;
        LONG refCount;

        Shared() : value(), refCount()
        {
            grab();
        }

        void grab()
        {
            std::cout << '\t' << __PRETTY_FUNCTION__ << ':' << refCount+1 << '\n';
            ++refCount;
        }

        void release()
        {
            std::cout << '\t' << __PRETTY_FUNCTION__ << ':' << refCount-1 << '\n';
            if (!--refCount)
                delete this;
        }

    } *shared;
};

int main()
{
    std::cout << "STOP_FLAG sf;" << '\n';
    STOP_FLAG sf; // one block
    {
        std::cout << "STOP_FLAG s2(sf);" << '\n';
        STOP_FLAG s2(sf);  // two objets, one shared block, refcount=2

        // still two objects, after settling, one shared block, refcount=2
        std::cout << "s2 = sf;" << '\n';
        s2 = sf; 

        // three objects, two shared (refcount=2), one stand-alone (refcount=1)
        std::cout << "STOP_FLAG s3;" << '\n';
        STOP_FLAG s3; 

        // three objects, one shared block, (refcount=3)
        std::cout << "s3 = s2;" << '\n';
        s3 = s2; 

        // three more objects, all singular, 
        std::cout << "STOP_FLAG s4[3];" << '\n';
        STOP_FLAG s4[3];

        // each assignment below attaches to sf, detaches prior content.
        std::cout << "s4[0] = s3;" << '\n';
        s4[0] = s3;
        std::cout << "s4[1] = s3;" << '\n';
        s4[1] = s3;
        std::cout << "s4[2] = s3;" << '\n';
        s4[2] = s3;

        // by this point every object so far all shared the same
        //  shared data, refcount=6.

        // another stand-alone object
        std::cout << "STOP_FLAG s5;" << '\n';
        STOP_FLAG s5;

        // s3 attaches to s5, detaches from prior shared data
        //  meaning we now have two shared data blocks with
        // reference counts of 2 and 5 respectively.
        std::cout << "s3 = s5;" << '\n';
        s3 = s5;

        std::cout << "scope exiting\n";

        // in leaving scope, *all* objects are detached except ONE
        //  (the one outside of this; sf), which leaves a single 
        // shared block with refcount of 1 only.
    }
    std::cout << "scope exited\n";

    // the last object, sf, is released here.
}

Output

STOP_FLAG sf;
    void STOP_FLAG::Shared::grab():1
STOP_FLAG s2(sf);
    void STOP_FLAG::Shared::grab():2
s2 = sf;
    void STOP_FLAG::Shared::grab():3
    void STOP_FLAG::Shared::release():2
STOP_FLAG s3;
    void STOP_FLAG::Shared::grab():1
s3 = s2;
    void STOP_FLAG::Shared::grab():3
    void STOP_FLAG::Shared::release():0
STOP_FLAG s4[3];
    void STOP_FLAG::Shared::grab():1
    void STOP_FLAG::Shared::grab():1
    void STOP_FLAG::Shared::grab():1
s4[0] = s3;
    void STOP_FLAG::Shared::grab():4
    void STOP_FLAG::Shared::release():0
s4[1] = s3;
    void STOP_FLAG::Shared::grab():5
    void STOP_FLAG::Shared::release():0
s4[2] = s3;
    void STOP_FLAG::Shared::grab():6
    void STOP_FLAG::Shared::release():0
STOP_FLAG s5;
    void STOP_FLAG::Shared::grab():1
s3 = s5;
    void STOP_FLAG::Shared::grab():2
    void STOP_FLAG::Shared::release():5
scope exiting
    void STOP_FLAG::Shared::release():1
    void STOP_FLAG::Shared::release():4
    void STOP_FLAG::Shared::release():3
    void STOP_FLAG::Shared::release():2
    void STOP_FLAG::Shared::release():0
    void STOP_FLAG::Shared::release():1
scope exited
    void STOP_FLAG::Shared::release():0

---

It's possible that you may find boost::intrusive_ptr useful.