Is there a potential for resource leak/double free here?

Question

The following sample (not compiled so I won't vouch for syntax) pulls two resources from resource pools (not allocated with new), then "binds" them together with MyClass for the duration of a certain transaction.

The transaction, implemented here by myFunc, attempts to protect against leakage of these resources by tracking their "ownership". The local resource pointers are cleared when its obvious that instantiation of MyClass was successful. The local catch, as well as the destructor ~MyClass return the resources to their pool (double-frees are protected by teh above mentioned clearing of the local pointers).

Instantiation of MyClass can fail and result in an exception at two steps (1) actual memory allocation, or (2) at the constructor body itself. I do not have a problem with #1, but in the case of #2, if the exception is thrown AFTER m_resA & m_resB were set. Causing both the ~MyClass and the cleanup code of myFunc to assume responsibility for returning these resources to their pools.

Is this a reasonable concern?

Options I have considered, but didn't like:

• Smart pointers (like boost's shared_ptr). I didn't see how to apply to a resource pool (aside for wrapping in yet another instance).
• Allowing double-free to occur at this level but protecting at the resource pools.
• Trying to use the exception type - trying to deduce that if bad_alloc was caught that MyClass did not take ownership. This will require a try-catch in the constructor to make sure that any allocation failures in ABC() ...more code here... wont be confused with failures to allocate MyClass.

Is there a clean, simple solution that I have overlooked?

class SomeExtResourceA;
class SomeExtResourceB;

class MyClass {
private:
  // These resources come out of a resource pool not allocated with "new" for each use by MyClass
  SomeResourceA* m_resA;
  SomeResourceB* m_resB;

public:
  MyClass(SomeResourceA* resA, SomeResourceB* resB):
    m_resA(resA), m_resB(resB)
    {
       ABC(); // ... more code here, could throw exceptions
    }

  ~MyClass(){
    if(m_resA){
      m_resA->Release();
    }
    if(m_resB){
      m_resB->Release();
    }
  }
};

void myFunc(void)
{
  SomeResourceA* resA    = NULL;
  SomeResourceB* resB    = NULL;
  MyClass*       pMyInst = NULL;

  try {
    resA = g_pPoolA->Allocate();
    resB = g_pPoolB->Allocate();
    pMyInst = new MyClass(resA,resB);
    resA=NULL; // ''ownership succesfully transfered to pMyInst
    resB=NULL; // ''ownership succesfully transfered to pMyInst

    // Do some work with pMyInst;
    ...;

    delete pMyInst;

  } catch (...) {
    // cleanup
    // need to check if resA, or resB were allocated prior 
    // to construction of pMyInst.
    if(resA) resA->Release();
    if(resB) resB->Release();
    delete pMyInst;
    throw; // rethrow caught exception
  }
}

Answer 1

Here is your chance for a double call to release:

void func()
{
   MyClass   a(resourceA, resourceB);
   MyClass   b(a);
}

Whoops.

If you use an RIAA wrapper fro your resources you will be much less likely to make mistakes. Doing it this way is error prone. You are currently missing the copy constructor and assignment operator on MyClass that could potentially lead to a double call to Release() as shown above.

Because of the complexity of handling resource a class should only own one resource. If you have multiple resource delegate their ownership to a class that it dedicated to their ownership and use multiple of these objects in your class.

Edit 1

Lut us make some assumptions:

Resources are shared and counted. You increment the count with Acquire() and decrement the count with Release(). When count reaches zero they are automatically destroyed.

class ReferenceRapper
{ 
    ReferenceBase*   ref;
    public:
        ReferenceWrapper(ReferenceBase* r) : ref (r)  {/* Pool set the initial count to 1 */ }
       ~ReferenceWrapper()                            { if (ref) { ref->Release();} }

        /*
         * Copy constructor provides strong exception guarantee (aka transactional guarantee)
         * Either the copy works or both objects remain unchanged.
         *
         * As the assignment operator is implemented using copy/swap it also provides
         * the strong exception guarantee.
         */
        ReferenceWrapper(ReferenceWrapper& copy)
        {
            if (copy.ref) {copy.ref->Acquire();}
            try
            {
                if (ref) {ref->Release();}
            }
            catch(...)
            {
                if (copy.ref)
                {  copy.ref->Release(); // old->Release() threw an exception. 
                                        // Must reset copy back to its original state.
                }
                throw;
            }
            ref = copy.ref;
        }
        /* 
         * Note using the copy and swap idium.
         * Note: To enable NRVO optimization we pass by value to make a copy of the RHS.
         *       rather than doing a manual copy inside the method.
         */
        ReferenceWrapper& operator(ReferenceWrapper rhsCopy)
        {
            this->swap(rhsCopy);
        }
        void swap(ReferenceWrapper& rhs) throws ()
        {
            std::swap(ref, rhs.ref);
        }
        // Add appropriate access methods like operator->()
};

Now that the hard work has been done (managing resources). The real code becomes trivial to write.

class MyClass
{
        ReferenceWrapper<SomeResourceA>  m_resA;
        ReferenceWrapper<SomeResourceB>  m_resB;
    public:
        MyClass(ReferenceWrapper<SomeResourceA>& a, ReferenceWrapper<SomeResourceB>& b)
            : m_resA(a)
            , m_resB(b)
        {
           ABC();
        }
};

void myFunc(void)
{
  ReferenceWrapper<SomeResourceA> resA(g_pPoolA->Allocate());
  ReferenceWrapper<SomeResourceB> resB(g_pPoolB->Allocate());

  std::auto_ptr<MyClass>         pMyInst = new MyClass(resA, resB);


  // Do some work with pMyInst;
}

Edit 2 Based on comment below that resources only have one owner:

If we assume a resource has only one owner and is not shared then it becomes trivial:

1. Drop the Release() method and do all the work in the destructor.
2. Change the Pool methods so that the construct the pointer into a std::auto_ptr and return the std::auto_ptr.

Code:

class MyClass
{
        std::auto_ptr<SomeResourceA>  m_resA;
        std::auto_ptr<SomeResourceB>  m_resB;
    public:
        MyClass(std::auto_ptr<SomeResourceA>& a, std::auto_ptr<SomeResourceB>& b)
            : m_resA(a)
            , m_resB(b)
        {
           ABC();
        }
};

void myFunc(void)
{
  std::auto_ptr<SomeResourceA> resA(g_pPoolA->Allocate());
  std::auto_ptr<SomeResourceB> resB(g_pPoolB->Allocate());

  std::auto_ptr<MyClass>       pMyInst = new MyClass(resA, resB);


  // Do some work with pMyInst;
}

Answer 2

Your code is fine. But to make it even better, use some kind of smart-pointer!

Edit: for example you can use shared_ptr:

class SomeExtResourceA;
class SomeExtResourceB;

class MyClass {
private:
  // These resources come out of a resource pool not allocated with "new" for each use by MyClass
  shared_ptr<SomeResourceA> m_resA;
  shared_ptr<SomeResourceB> m_resB;

public:
  MyClass(const shared_ptr<SomeResourceA> &resA, const shared_ptr<SomeResourceB> &resB):
    m_resA(resA), m_resB(resB)
    {
       ABC(); // ... more code here, could throw exceptions
    }
  }
};

void myFunc(void)
{
  shared_ptr<SomeResourceA> resA(g_pPoolA->Allocate(), bind(&SomeResourceA::Release, _1));
  shared_ptr<SomeResourceB> resB(g_pPoolB->Allocate(), bind(&SomeResourceB::Release, _1));
  MyClass pMyInst(resA,resB);

  // you can reset them here if you want, but it's not necessery:
  resA.reset(), resB.reset();

  // use pMyInst
}

I find this solution with RAII much simpler.

Answer 3

I don't see any leak in this small code.

If the constructor throws exception, then the destructor would not be called, since the object never existed. Hence I don't see double-delete either!

From this article by Herb Sutter :Constructor Exceptions in C++, C#, and Java:

• constructor conceptually turns a suitably sized chunk of raw memory into an object that obeys its invariants. An object’s lifetime doesn’t begin until its constructor completes successfully. If a constructor ends by throwing an exception, that means it never finished creating the object and setting up its invariants — and at the point the exceptional constructor exits, the object not only doesn’t exist, but never existed.
• A destructor/disposer conceptually turns an object back into raw memory. Therefore, just like all other nonprivate methods, destructors/disposers assume as a precondition that “this” object is actually a valid object and that its invariants hold. Hence, destructors/disposers only run on successfully constructed objects.

I think this should clear your doubts!

Answer 4

The classic usage to explicitly take ownership is the std::auto_ptr

Something like this:

std::auto_ptr<SomeResourceA>(g_pPoolA->Allocate()) resA;
std::auto_ptr<SomeResourceB>(g_pPoolB->Allocate()) resB;
pMyInst = new MyClass(resA.release(),resB.release());

You transfer the ownership when you call the constructor.

Answer 5

Just put if (pMyInst) { ... } around release/delete code in your catch and you are fine.