fixing leaks and macros..

Right. Here's a little exercise into .. well, a few examples of badness
in C/C++ code.

We're seeing a memory leak in the fwdState cbdata allocation. After
a bit of tracing I discovered this routine:

void
ConnectStateData::callCallback(comm_err_t status, int xerrno)
{
    debug(5, 3) ("commConnectCallback: fd %d, data %p\n", fd, callback.data);
    comm_remove_close_handler(fd, commConnectFree, this);
   CallBack<CNCB> aCallback = callback;
        
    callback = CallBack<CNCB>();
            
    commSetTimeout(fd, -1, NULL, NULL);

    if (cbdataReferenceValid(aCallback.data)) {
        aCallback.handler(fd, status, xerrno, aCallback.data);
    }
    cbdataReferenceDone(aCallback.data);
    
    commConnectFree(fd, this);
}

Now. Once this routine had completed the fwdState cbdata allocation was still
at a refcount of 1.

After replacing a few things with pointer copies I discovered that
the cbdataReferenceDone(aCallback.data) seemed to be the culprit - if I
replaced it with:

 void *dd = aCallback.data;
 cbdataReferenceDone(dd);

.. then everything worked.

Then, I discovered this:

#define cbdataReferenceDone(var) do {if (var) {cbdataInternalUnlock(var); var = NULL;}} while(0)

.. this is what was happening.

* the copy (callback => aCallback) happened, so the cbdata was ref'ed again
* the callback was called
* cbdataReferenceDone was called, which through the power of evil macro
  evaluation set aCallback.data to NULL
* the function completed, aCallback was destructed but since the data pointer was
  NULL we didn't unref the data pointer

Now. The solution isn't terribly simple - there was a good reason that bit
of code was placed in the macro - but, its going to interfere with uses such
as the above.

Now, what to do? I'd just replace em all with capitalised versions of the strings
to highlight the macro-ness and then write a new one which we use if we know
that our usage is correct (ie, after an unref we treat the data pointer as possibly
invalid) ..

This may be also responsible for the FTP leak I've seen in my testing and may also
be causing strangeness in other areas of thec ode.

Thoughts? Ideas on the best way to fix it?

Adrian
Received on Sun Feb 15 2004 - 00:02:50 MST