Re: [RFC] cbdataFree does not call destructor

On 7/07/2012 6:04 p.m., Alex Rousskov wrote:
> Hello,
>
> 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). Some code tries to reset
> those pointers before calling cbdataFree(), but, naturally, there are
> quite a few places were we fail to cleanup. The other places are just
> ticking bombs until somebody adds a sensitive non-POD data member. See
> bug #3133 for an example.
>
> 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)?

I agree. The more of those cbdata macros we can erase the better.

>
> Such a rewrite would require adding CBDATA_CLASS2 macros to some
> structs, but it will be much safer than trying to find all such bugs and
> to monitor that new ones do not crop up (especially where a non-POD
> member is added indirectly -- to a type used by a previously POD class
> member; for example String added to Ip::Address used by many
> cbdataFree()d types).
>
> 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.
Such as member toCbdata() ?

> 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.

Yes.

>
> Any better ideas?

Making cbdata an inherited-from class with all the cbdataInternal*()
functions its methods? why is that not done already?

Can CBDATA_CLASS2 operations be made a template which classes inherit
from? for something like RefCountable_.

>
>
> Thank you,
>
> Alex.
> 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 :-).

Am I right in thinking cbdataAlloc/cbdataFree are the remaining blockers
to CbcPointer<> just being inserted everywhere? is there any more
complication to that rollout?
That would enable us to get rid of the manual cbdata-reference
accounting macros and functions.

One other simplification idea: What use is the HASHED_CBDATA split?
removing that conditional stuff clears away a moderate chunk of cbdata.

Amos
Received on Sat Jul 07 2012 - 07:29:55 MDT