Re: [RFC] cbdataFree does not call destructor

On 07/07/2012 04:30 AM, Henrik Nordström wrote:
> lör 2012-07-07 klockan 00:04 -0600 skrev Alex Rousskov:
>
>> Squid calls cbdataFree() for objects that have non-POD data members
>> such as refcounted pointers. Since cbdataFree() does not call the object
>> destructor when freeing object memory, those data members are not
>> properly destroyed, leading to leaks (at best).
>
> I had the impression that the CBDATA_CLASS controlled classes should
> have their destructors called on free?

Please do not use the term "free" in this context. It is ambiguous (free
as in cbdataFree(foo) or free as in "delete foo").

cbdataFree(foo) does not know foo's type and, hence, cannot call foo's
destructor. It does not matter what you stuff inside foo's declaration.
CBDATA_CLASS, CBDATA_CLASS2, or nothing. cbdataFree(foo) does not call
foo's destructor.

CBDATA_CLASS* macros allow us to use new/delete instead of
cbdataAlloc/Free. If we use new/delete, the constructors and destructors
are called, of course. This solves one problem, but we have these three
other problems to solve:

1. Classes that have CBDATA_CLASS* macros but use cbdataAlloc/Free.
2. Classes that do not have CBDATA_CLASS* macros but are not PODs.
3. cbdataReferenceDone() that does not call the destructor either.

> Are you talking about the old C struct based ones? Those should go.

The class/struct declaration or history does not matter. What matters is
how you manipulate it. If you use cbdataAlloc(), the constructor is not
called. If you use cbdataFree() or cbdataReferenceDone(), the destructor
is not called. For PODs, this is OK. For non PODs, this leads to leaks
and probably other bugs.

>> The attached untested patch tries to fix the bugs I could identify, but
>> I wonder whether it would be better to remove cbdataAlloc/Free and call
>> new/delete instead (to properly execute constructors and destructors)?
>
> The cbdata interface do not fit entirely to new/delete. The difference
> is that cbdataFree only marks objects as "to be freed". There may still
> be reference counts keeping it alive.

That is what "delete foo" does as well if foo's type has CBDATA_CLASS2
macros or equivalent. Remember that

    delete foo

in this context essentially means this:

    if (foo) {
        foo->~Foo(); // call the destructor (#1)
        Foo::delete(foo); // call the delete method (#2)
    }

The destructor call itself (#1) never deallocates object memory. The
delete method call (#2) usually deallocates object memory but does not
really have to, and that is what CBDATA_CLASS2 macros (and friends)
abuse to emulate cbdataFree().

> But this said cbdata really SHOULD get replaced by type hiding refcount
> + invalidation. Moving to CBDATA_CLASS macros is a good step towards
> that.

Agreed (though I am not sure what you mean by "type hiding" here).

>> I think this transition can be done more-or-less safely by first
>> adjusting cbdataAlloc/Free() macros so that they fail to compile if the
>> corresponding class does not have a special member added by
>> CBDATA_CLASS2 macros. This way, we will not forget to add CBDATA_CLASS2
>> to any class/struct that uses cbdataFree(). cbdataFree() also sets the
>> pointer to NULL so we would still have to manually check that it is not
>> needed if we want to remove that macro.
>>
>> Any better ideas?
>
> As said above.. refcount instead of explicit counts.

>> P.S. I am also concerned what happens when cbdataReferenceDone() reaches
>> zero lock counter for a non-POD object. Will need to get some sleep
>> before researching that :-).
>
> Hmm... yes it's odd.
>
> Looking at CBDATA_CLASS2 it does NOT provide the original cbdata
> interface where the object is destructed when last reference is gone
> AFTER free. Here the object is destructed on delete but not freed until
> last cbdata reference is gone.

I agree with your description of what happens, but I disagree that this
violates the original cbdata interface.

"destructed" in your context means calling the destructor. For PODs,
calling the destructor is a no-op, essentially. Thus, CBDATA_CLASS2
macros DO provide the original cbdata interface: delete invalidates
immediately and delete/cbdataReferenceDone() free memory when all
references are gone. Calling the destructor is something extra they add
on top of the old interface (which is not an interface violation, of
course).

> The interface provided by CBDATA_CLASS actually requires that any
> references are anonymous not knowing their target type, making sure that
> they do not attempt to access the object.

The interface provided by cbdata*(), you mean? Yes, I agree. And that
creates problems with properly cleaning up non-PODs because to proper
clean a non-POD you must know its exact type.

Thank you,

Alex.
Received on Sat Jul 07 2012 - 15:25:24 MDT