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
