On 06/29/2016 05:45 AM, Amos Jeffries wrote: > /** > + * A pointer that deletes the object it points to when the pointer's owner or > + * context is gone. [...] > */ ... > + explicit LockingPointer(const SelfType &o) : raw(nullptr) { > resetAndLock(o.get()); }
Something went wrong here: If both the class description and the copy constructor are correct, then the following pseudo code ought to crash Squid: LockingPointer a(OpenSSL_new(...)); // a points to a new object X { LockingPointer b(a); // a and b point to the same object X ... // b context ends so b deletes X } ... // a context ends so a deletes X // Squid crashes because the same X was deleted twice I hope that Squid code is actually correct, but the proposed class description is not. LockingPointer does _not_ delete the object it points to when the pointer's owner or context is gone. LockingPointer is actually a reference counting pointer like shared_ptr (not a unique_ptr or auto_ptr!). There is no concept of [a single] owner here -- the object is _shared_ among many "owners". That makes copy assignment safe. If I am right, then we need to fix the LockingPointer class description (and make sure the implementation matches it). There several ways to do that, including the following three: A. Provide two different LockingPointer classes, one for OpenSSL, and one for GnuTLS. The tricky shared pointer code will be duplicated (bad), but each class itself would be easy to comprehend because it will lack #ifdefs (good): #if USE_OPENSSL template <...> class LockingPointer ... #elif USE_GNUTLS template <...> class LockingPointer ... #else B. Provide a single LockingPointer class along with two wrappers that customize template parameters, one for OpenSSL, and one for GnuTLS. No tricky shared pointer duplication (good), but an extra level of wrapping would complicate comprehension (bad): template <...> class LockingPointer ... #if USE_OPENSSL template <...> using OpenSslLockingPointer = LockingPointer<...> #elif USE_GNUTLS template <...> class GnuTlsLockingPointer = LockingPointer<...> #else I have attached a sketch for B in case you want to see what that would look like. C. Provide a single LockingPointer class sprinkled with #ifdefs customizing its API and implementation for each library. No tricky shared pointer duplication (good), but large number of #ifdefs, including #ifdefs in class template parameters, will make the code difficult to comprehend (bad). We are currently doing C. I have no objections to us continuing doing C, at least for now. If you pick C, then the fixed LockingPointer class documentation may look like the following (compare that with the documentation for LockingPointer in the attached sketch for B): ----- A shared pointer to a reference-counting Object with library-specific absorption, locking, and unlocking implementations. The API largely follows std::shared_ptr. The constructor and the absorb() method import a raw Object pointer. Normally, absorb() would just lock(), but libraries like OpenSSL pre-lock objects before they are fed to LockingPointer, necessitating this customization hook. The lock() method increments Object's reference counter. The unlock() method decrements Object's reference counter and destroys the object when the counter reaches zero. ----- The absorb/lock/unlock split above may help with seamless GnuTLS support, assuming GnuTLS does not pre-lock objects like OpenSSL does. However, somebody familiar with GnuTLS should check whether it has other special needs not addressed by the above LockingPointer sketch. For example, if GnuTLS does not have a concept of locking at all, then we may be better of with approach A where GnuTLS code just uses std::shared_ptr! > + /// Deallocate raw pointer. Become a nil pointer. > + void deletePointer() { > + if (raw) > + DeAllocator(raw); > + raw = nullptr; > + } please rename to unlock(). We are not deleting the pointer here and, depending on the lock count, we are not deleting the object either! Renaming DeAllocator to Unlock or Unlocker is also a good idea -- we have wasted enough time being confused by OpenSSL _free() functions not freeing locked objects! > + /// Reset raw pointer - delete last one and save new one. > + void reset(T *t) { > + deletePointer(); > + raw = t; > } I do not think we can have a reset() method like this because the standard shared_ptr::reset() has a very different semantics. Let's call this absorb(). This is like a raw pointer assignment operator, but we want to keep it "explicit" because we think this is a dangerous operation. > void resetAndLock(T *t) { > if (t != this->get()) { > this->reset(t); > #if USE_OPENSSL > if (t) > CRYPTO_add(&t->references, 1, lock); > #elif USE_GNUTLS > // XXX: GnuTLS does not provide locking ? > #else > assert(false); > #endif > } > } Please rewrite as a pair of public reset(t) and private lock(void) methods. reset(t) calls lock() for non-nil t, of course. Please also add a public clear(void) method that reset(t) will call instead of lock() when t is nil. Alternatively, add a reset(void) method with the same semantics as clear(void) -- that is what std::shared_ptr does. To resolve conflicts between abused reset() in the old code and the new reset(), I suggest the following procedure: 1. Add absorb() and clear(void)/reset(void) methods as discussed above. 2. Remove LockingPointer::reset(x). Replace all calls to LockingPointer::reset(x) with absorb() and clear(void)/reset(void), depending on whether x is nil. See earlier emails on how to find all those calls. IIRC, Christos has certified that all those old reset(x) calls are meant to be either non-locking absorption or clearance calls. 3. Rename LockingPointer::resetAndLock(x) to reset(x). Replace all calls to LockingPointer::resetAndLock() with reset(x). You may argue that these reset()-related changes are outside this project scope, but I think it is too dangerous to commit LockingPointer with a reset(x) method that does not work like shared_ptr::reset(x). In the current trunk code, we are calling TidyPointer::reset(), which is already very ugly, but can at least be half-justified because TidyPointer::reset() does what it is supposed to do. Moving that method from TidyPointer to LockingPointer moves the needle from "terribly ugly" to "unacceptably dangerous" IMO. > + void operator()(argument_type a) { sk_object ## _pop_free(a, > freefunction); } \ If you can make this operator "const", please make it "const". Please note that the originally proposed commit message no longer applies to your changes so we need a new one. HTH, Alex.
// g++ -Wall -std=c++11 -DUSE_OPENSSL -c t-lockingp.cc /* A shared pointer to a reference-counting Object with customizable absorption, locking, and unlocking functions. The Absorb() function imports a raw Object pointer. It is usually identical to Lock(), but libraries like OpenSSL pre-lock objects before they are fed to LockingPointer, necessitating this customization hook. The Lock() function increments Object's reference counter. The Unlock() function decrements Object's reference counter and destroys the object when the counter reaches zero. */ template <typename Object, class Lock, class Unlock, class Absorb = Lock> class LockingPointer; #if USE_OPENSSL // fake declarations of OpenSSL functions for testing extern void CRYPTO_add(int *, int, int); typedef struct { int references; } X509; extern int CRYPTO_LOCK_X509; extern void X509_free(X509 *x); template <typename Object, int &Lock> class OpenSslLocker { public: void operator()(Object *x) { CRYPTO_add(x->references, 1, Lock); } }; template <typename Object, void (*Unlock)(Object*) > class OpenSslUnlocker { public: void operator()(Object *x) { Unlock(x); } }; template <typename Object> class OpenSslAbsorber { public: void operator()(Object *x) { /* OpenSSL locks raw objects for us */ } }; template <typename Object, int &lock, // 3rd CRYPTO_add() parameter void (*Unlock)(Object *x) > // usually called Object_free() using OpenSslLockingPointer = LockingPointer<Object, OpenSslLocker<Object, lock>, OpenSslUnlocker<Object, Unlock>, OpenSslAbsorber<Object> >; typedef OpenSslLockingPointer<X509, CRYPTO_LOCK_X509, X509_free> CertPointer; #endif /* USE_OPENSSL */
_______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev