[squid-dev] [PATCH] TidyPointer removal
Amos Jeffries
squid3 at treenet.co.nz
Mon Jun 27 10:35:54 UTC 2016
This splits TidyPointer and LockingPointer by removing the inheritence
and copying the needed TidyPointer code into the LockingPointer as-is.
The resulting situation is that TidyPointer is a stand-alone Pointer
type that duplicates the std::unique_ptr API so closely it can almost be
a drop-in replacement. The remaining difference is that TidyPointer took
a function pointer whereas unique_ptr uses a Functor type.
Adding a UniaryFunctor(function, arg) macro in src/base/MakeFunctor.h to
replace the CtoCpp1(function, arg) macro used by TidyPointer allows us
to remove TidyPointer completely from Squid-4.
The LockingPointer still has the issues which led us down the path to
getting here. I've chosen to stop and submit for review at this point to
ensure we still have a fully working Squid before going further down
this trail.
Amos
-------------- next part --------------
=== renamed file 'src/base/TidyPointer.h' => 'src/base/MakeFunctor.h'
--- src/base/TidyPointer.h 2016-03-31 23:33:45 +0000
+++ src/base/MakeFunctor.h 2016-06-26 06:04:29 +0000
@@ -6,64 +6,18 @@
* Please see the COPYING and CONTRIBUTORS files for details.
*/
-#ifndef SQUID_BASE_TIDYPOINTER_H
-#define SQUID_BASE_TIDYPOINTER_H
-
-/**
- * A pointer that deletes the object it points to when the pointer's owner or
- * context is gone. Similar to std::unique_ptr but without confusing assignment
- * and with a customizable cleanup method. Prevents memory leaks in
- * the presence of exceptions and processing short cuts.
-*/
-template <typename T, void (*DeAllocator)(T *t)> class TidyPointer
-{
-public:
- /// Delete callback.
- typedef void DCB (T *t);
- TidyPointer(T *t = NULL)
- : raw(t) {}
-public:
- bool operator !() const { return !raw; }
- explicit operator bool() const { return raw; }
- /// Returns raw and possibly NULL pointer
- T *get() const { return raw; }
-
- /// Reset raw pointer - delete last one and save new one.
- void reset(T *t) {
- deletePointer();
- raw = t;
- }
-
- /// Forget the raw pointer without freeing it. Become a nil pointer.
- T *release() {
- T *ret = raw;
- raw = NULL;
- return ret;
- }
- /// Deallocate raw pointer.
- ~TidyPointer() {
- deletePointer();
- }
-private:
- /// Forbidden copy constructor.
- TidyPointer(TidyPointer<T, DeAllocator> const &);
- /// Forbidden assigment operator.
- TidyPointer <T, DeAllocator> & operator = (TidyPointer<T, DeAllocator> const &);
- /// Deallocate raw pointer. Become a nil pointer.
- void deletePointer() {
- if (raw) {
- DeAllocator(raw);
+#ifndef SQUID_BASE_MAKEFUNCTOR_H
+#define SQUID_BASE_MAKEFUNCTOR_H
+
+// Macro to be used to define a C++ functor of an extern "C"
+// function. The C++ functor is suffixed with the _functor extension
+#define UniaryFunctor(function, argument_type) \
+ struct function ## _functor { \
+ void operator()(argument_type a) { function(a); } \
}
- raw = NULL;
- }
- T *raw; ///< pointer to T object or NULL
-};
-
-/// DeAllocator for pointers that need free(3) from the std C library
-template<typename T> void tidyFree(T *p)
-{
- xfree(p);
-}
-
-#endif // SQUID_BASE_TIDYPOINTER_H
+
+/// DeAllocator functor for pointers that need free(3) from the std C library
+UniaryFunctor(xfree, char *);
+
+#endif // SQUID_BASE_MAKEFUNCTOR_H
=== modified file 'src/base/Makefile.am'
--- src/base/Makefile.am 2016-01-15 06:47:59 +0000
+++ src/base/Makefile.am 2016-06-25 18:03:03 +0000
@@ -29,6 +29,7 @@
Lock.h \
LookupTable.h \
LruMap.h \
+ MakeFunctor.h \
Packable.h \
PackableStream.h \
RegexPattern.cc \
@@ -38,5 +39,4 @@
Subscription.h \
TextException.cc \
TextException.h \
- TidyPointer.h \
YesNoNone.h
=== modified file 'src/security/LockingPointer.h'
--- src/security/LockingPointer.h 2016-03-31 23:33:45 +0000
+++ src/security/LockingPointer.h 2016-06-25 15:23:17 +0000
@@ -9,8 +9,6 @@
#ifndef SQUID_SRC_SECURITY_LOCKINGPOINTER_H
#define SQUID_SRC_SECURITY_LOCKINGPOINTER_H
-#include "base/TidyPointer.h"
-
#if USE_OPENSSL
#if HAVE_OPENSSL_CRYPTO_H
#include <openssl/crypto.h>
@@ -24,7 +22,7 @@
sk_object ## _pop_free(a, freefunction); \
}
-#endif
+#endif /* USE_OPENSSL */
// Macro to be used to define the C++ equivalent function of an extern "C"
// function. The C++ function suffixed with the _cpp extension
@@ -37,36 +35,57 @@
{
/**
- * Add SSL locking (a.k.a. reference counting) and assignment to TidyPointer
+ * A pointer that deletes the object it points to when the pointer's owner or
+ * context is gone.
+ * Maintains locking using OpenSSL crypto API when exporting the stored value
+ * between objects.
+ * Prevents memory leaks in the presence of exceptions and processing short
+ * cuts.
*/
template <typename T, void (*DeAllocator)(T *t), int lock>
-class LockingPointer: public TidyPointer<T, DeAllocator>
+class LockingPointer
{
public:
- typedef TidyPointer<T, DeAllocator> Parent;
+ /// a helper label to simplify this objects API definitions below
typedef LockingPointer<T, DeAllocator, lock> SelfType;
- explicit LockingPointer(T *t = nullptr): Parent(t) {}
-
- explicit LockingPointer(const SelfType &o): Parent() {
- resetAndLock(o.get());
- }
-
+ /**
+ * Construct directly from a raw pointer.
+ * This action requires that the producer of that pointer has already
+ * created one reference lock for the object pointed to.
+ * Our destructor will do the matching unlock/free.
+ */
+ explicit LockingPointer(T *t = nullptr): raw(t) {}
+
+ /// use the custom DeAllocator to unlock and/or free any value still stored.
+ ~LockingPointer() { deletePointer(); }
+
+ // copy semantics are okay only when adding a lock reference
+ explicit LockingPointer(const SelfType &o) : raw(nullptr) { resetAndLock(o.get()); }
SelfType &operator =(const SelfType & o) {
resetAndLock(o.get());
return *this;
}
-#if __cplusplus >= 201103L
- explicit LockingPointer(LockingPointer<T, DeAllocator, lock> &&o): Parent(o.release()) {
- }
-
- LockingPointer<T, DeAllocator, lock> &operator =(LockingPointer<T, DeAllocator, lock> &&o) {
- if (o.get() != this->get())
- this->reset(o.release());
+ // move semantics are definitely okay, when possible
+ explicit LockingPointer(SelfType &&) = default;
+ SelfType &operator =(SelfType &&o) {
+ if (o.get() != raw)
+ reset(o.release());
return *this;
}
-#endif
+
+ bool operator !() const { return !raw; }
+ explicit operator bool() const { return raw; }
+
+ /// Returns raw and possibly nullptr pointer
+ T *get() const { return raw; }
+
+ /// Reset raw pointer - delete last one and save new one.
+ void reset(T *t) {
+ deletePointer();
+ raw = t;
+ }
void resetAndLock(T *t) {
if (t != this->get()) {
@@ -81,6 +100,23 @@
#endif
}
}
+
+ /// Forget the raw pointer without freeing it. Become a nil pointer.
+ T *release() {
+ T *ret = raw;
+ raw = nullptr;
+ return ret;
+ }
+
+private:
+ /// Deallocate raw pointer. Become a nil pointer.
+ void deletePointer() {
+ if (raw)
+ DeAllocator(raw);
+ raw = nullptr;
+ }
+
+ T *raw; ///< pointer to T object or nullptr
};
} // namespace Security
=== modified file 'src/security/Session.h'
--- src/security/Session.h 2016-02-13 12:12:10 +0000
+++ src/security/Session.h 2016-06-26 06:07:02 +0000
@@ -9,9 +9,11 @@
#ifndef SQUID_SRC_SECURITY_SESSION_H
#define SQUID_SRC_SECURITY_SESSION_H
-// LockingPointer.h instead of TidyPointer.h for CtoCpp1()
+#include "base/MakeFunctor.h"
#include "security/LockingPointer.h"
+#include <memory>
+
#if USE_OPENSSL
#if HAVE_OPENSSL_SSL_H
#include <openssl/ssl.h>
@@ -33,17 +35,18 @@
#elif USE_GNUTLS
typedef gnutls_session_t SessionPtr;
-CtoCpp1(gnutls_deinit, gnutls_session_t);
+UniaryFunctor(gnutls_deinit, gnutls_session_t);
// TODO: Convert to Locking pointer.
// Locks can be implemented attaching locks counter to gnutls_session_t
// objects using the gnutls_session_set_ptr()/gnutls_session_get_ptr ()
// library functions
-typedef TidyPointer<struct gnutls_session_int, Security::gnutls_deinit_cpp> SessionPointer;
+typedef std::unique_ptr<struct gnutls_session_int, Security::gnutls_deinit_functor> SessionPointer;
#else
// use void* so we can check against NULL
typedef void* SessionPtr;
-typedef TidyPointer<void, nullptr> SessionPointer;
+// use nullptr_t so default_delete works
+typedef std::unique_ptr<nullptr_t> SessionPointer;
#endif
=== modified file 'src/security/cert_generators/file/certificate_db.cc'
--- src/security/cert_generators/file/certificate_db.cc 2016-02-13 16:10:26 +0000
+++ src/security/cert_generators/file/certificate_db.cc 2016-06-24 12:00:04 +0000
@@ -11,6 +11,7 @@
#include <cerrno>
#include <fstream>
+#include <memory>
#include <stdexcept>
#if HAVE_SYS_STAT_H
#include <sys/stat.h>
@@ -252,7 +253,6 @@
db_full(aDb_path + "/" + db_file),
cert_full(aDb_path + "/" + cert_dir),
size_full(aDb_path + "/" + size_file),
- db(NULL),
max_db_size(aMax_db_size),
fs_block_size((aFs_block_size ? aFs_block_size : 2048)),
dbLock(db_full),
@@ -282,6 +282,11 @@
return true;
}
+/// DeAllocator functor for unique_ptr that need free(3) from the std C library
+struct xfree_char {
+ void operator()(char *a) { xfree(a); }
+};
+
bool Ssl::CertificateDb::addCertAndPrivateKey(Security::CertPointer & cert, Ssl::EVP_PKEY_Pointer & pkey, std::string const & useName) {
const Locker locker(dbLock, Here);
load();
@@ -292,7 +297,7 @@
std::string serial_string;
Ssl::BIGNUM_Pointer serial(ASN1_INTEGER_to_BN(ai, NULL));
{
- TidyPointer<char, tidyFree> hex_bn(BN_bn2hex(serial.get()));
+ std::unique_ptr<char, xfree_char> hex_bn(BN_bn2hex(serial.get()));
serial_string = std::string(hex_bn.get());
}
row.setValue(cnlSerial, serial_string.c_str());
@@ -305,7 +310,7 @@
}
{
- TidyPointer<char, tidyFree> subject(X509_NAME_oneline(X509_get_subject_name(cert.get()), NULL, 0));
+ std::unique_ptr<char, xfree_char> subject(X509_NAME_oneline(X509_get_subject_name(cert.get()), nullptr, 0));
Security::CertPointer findCert;
Ssl::EVP_PKEY_Pointer findPkey;
if (pure_find(useName.empty() ? subject.get() : useName, findCert, findPkey)) {
@@ -348,7 +353,7 @@
if (!useName.empty())
row.setValue(cnlName, useName.c_str());
else {
- TidyPointer<char, tidyFree> subject(X509_NAME_oneline(X509_get_subject_name(cert.get()), NULL, 0));
+ std::unique_ptr<char, xfree_char> subject(X509_NAME_oneline(X509_get_subject_name(cert.get()), nullptr, 0));
row.setValue(cnlName, subject.get());
}
=== modified file 'src/security/forward.h'
--- src/security/forward.h 2016-01-01 00:12:18 +0000
+++ src/security/forward.h 2016-06-24 13:09:31 +0000
@@ -19,6 +19,16 @@
#endif
#include <list>
+#if USE_OPENSSL
+// Macro to be used to define the C++ wrapper functor of the sk_*_pop_free
+// OpenSSL family of functions. The C++ functor is suffixed with the _free_wrapper
+// extension
+#define sk_dtor_wrapper(sk_object, argument_type, freefunction) \
+ struct sk_object ## _free_wrapper { \
+ void operator()(argument_type a) { sk_object ## _pop_free(a, freefunction); } \
+ }
+#endif /* USE_OPENSSL */
+
/* flags a SSL connection can be configured with */
#define SSL_FLAG_NO_DEFAULT_CA (1<<0)
#define SSL_FLAG_DELAYED_AUTH (1<<1)
=== modified file 'src/ssl/gadgets.h'
--- src/ssl/gadgets.h 2016-06-14 15:56:12 +0000
+++ src/ssl/gadgets.h 2016-06-26 06:06:19 +0000
@@ -39,53 +39,52 @@
#endif
/**
- \ingroup SslCrtdSslAPI
- * TidyPointer typedefs for common SSL objects
+ * std::unique_ptr typedefs for common SSL objects
*/
-sk_free_wrapper(sk_X509, STACK_OF(X509) *, X509_free)
-typedef TidyPointer<STACK_OF(X509), sk_X509_free_wrapper> X509_STACK_Pointer;
+sk_dtor_wrapper(sk_X509, STACK_OF(X509) *, X509_free);
+typedef std::unique_ptr<STACK_OF(X509), sk_X509_free_wrapper> X509_STACK_Pointer;
CtoCpp1(EVP_PKEY_free, EVP_PKEY *)
typedef Security::LockingPointer<EVP_PKEY, EVP_PKEY_free_cpp, CRYPTO_LOCK_EVP_PKEY> EVP_PKEY_Pointer;
-CtoCpp1(BN_free, BIGNUM *)
-typedef TidyPointer<BIGNUM, BN_free_cpp> BIGNUM_Pointer;
-
-CtoCpp1(BIO_free, BIO *)
-typedef TidyPointer<BIO, BIO_free_cpp> BIO_Pointer;
-
-CtoCpp1(ASN1_INTEGER_free, ASN1_INTEGER *)
-typedef TidyPointer<ASN1_INTEGER, ASN1_INTEGER_free_cpp> ASN1_INT_Pointer;
-
-CtoCpp1(ASN1_OCTET_STRING_free, ASN1_OCTET_STRING *)
-typedef TidyPointer<ASN1_OCTET_STRING, ASN1_OCTET_STRING_free_cpp> ASN1_OCTET_STRING_Pointer;
-
-CtoCpp1(TXT_DB_free, TXT_DB *)
-typedef TidyPointer<TXT_DB, TXT_DB_free_cpp> TXT_DB_Pointer;
-
-CtoCpp1(X509_NAME_free, X509_NAME *)
-typedef TidyPointer<X509_NAME, X509_NAME_free_cpp> X509_NAME_Pointer;
-
-CtoCpp1(RSA_free, RSA *)
-typedef TidyPointer<RSA, RSA_free_cpp> RSA_Pointer;
-
-CtoCpp1(X509_REQ_free, X509_REQ *)
-typedef TidyPointer<X509_REQ, X509_REQ_free_cpp> X509_REQ_Pointer;
-
-sk_free_wrapper(sk_X509_NAME, STACK_OF(X509_NAME) *, X509_NAME_free)
-typedef TidyPointer<STACK_OF(X509_NAME), sk_X509_NAME_free_wrapper> X509_NAME_STACK_Pointer;
-
-CtoCpp1(AUTHORITY_KEYID_free, AUTHORITY_KEYID *)
-typedef TidyPointer<AUTHORITY_KEYID, AUTHORITY_KEYID_free_cpp> AUTHORITY_KEYID_Pointer;
-
-sk_free_wrapper(sk_GENERAL_NAME, STACK_OF(GENERAL_NAME) *, GENERAL_NAME_free)
-typedef TidyPointer<STACK_OF(GENERAL_NAME), sk_GENERAL_NAME_free_wrapper> GENERAL_NAME_STACK_Pointer;
-
-CtoCpp1(GENERAL_NAME_free, GENERAL_NAME *)
-typedef TidyPointer<GENERAL_NAME, GENERAL_NAME_free_cpp> GENERAL_NAME_Pointer;
-
-CtoCpp1(X509_EXTENSION_free, X509_EXTENSION *)
-typedef TidyPointer<X509_EXTENSION, X509_EXTENSION_free_cpp> X509_EXTENSION_Pointer;
+UniaryFunctor(BN_free, BIGNUM *);
+typedef std::unique_ptr<BIGNUM, BN_free_functor> BIGNUM_Pointer;
+
+UniaryFunctor(BIO_free, BIO *);
+typedef std::unique_ptr<BIO, BIO_free_functor> BIO_Pointer;
+
+UniaryFunctor(ASN1_INTEGER_free, ASN1_INTEGER *);
+typedef std::unique_ptr<ASN1_INTEGER, ASN1_INTEGER_free_functor> ASN1_INT_Pointer;
+
+UniaryFunctor(ASN1_OCTET_STRING_free, ASN1_OCTET_STRING *);
+typedef std::unique_ptr<ASN1_OCTET_STRING, ASN1_OCTET_STRING_free_functor> ASN1_OCTET_STRING_Pointer;
+
+UniaryFunctor(TXT_DB_free, TXT_DB *);
+typedef std::unique_ptr<TXT_DB, TXT_DB_free_functor> TXT_DB_Pointer;
+
+UniaryFunctor(X509_NAME_free, X509_NAME *);
+typedef std::unique_ptr<X509_NAME, X509_NAME_free_functor> X509_NAME_Pointer;
+
+UniaryFunctor(RSA_free, RSA *);
+typedef std::unique_ptr<RSA, RSA_free_functor> RSA_Pointer;
+
+UniaryFunctor(X509_REQ_free, X509_REQ *);
+typedef std::unique_ptr<X509_REQ, X509_REQ_free_functor> X509_REQ_Pointer;
+
+sk_dtor_wrapper(sk_X509_NAME, STACK_OF(X509_NAME) *, X509_NAME_free);
+typedef std::unique_ptr<STACK_OF(X509_NAME), sk_X509_NAME_free_wrapper> X509_NAME_STACK_Pointer;
+
+UniaryFunctor(AUTHORITY_KEYID_free, AUTHORITY_KEYID *);
+typedef std::unique_ptr<AUTHORITY_KEYID, AUTHORITY_KEYID_free_functor> AUTHORITY_KEYID_Pointer;
+
+sk_dtor_wrapper(sk_GENERAL_NAME, STACK_OF(GENERAL_NAME) *, GENERAL_NAME_free);
+typedef std::unique_ptr<STACK_OF(GENERAL_NAME), sk_GENERAL_NAME_free_wrapper> GENERAL_NAME_STACK_Pointer;
+
+UniaryFunctor(GENERAL_NAME_free, GENERAL_NAME *);
+typedef std::unique_ptr<GENERAL_NAME, GENERAL_NAME_free_functor> GENERAL_NAME_Pointer;
+
+UniaryFunctor(X509_EXTENSION_free, X509_EXTENSION *);
+typedef std::unique_ptr<X509_EXTENSION, X509_EXTENSION_free_functor> X509_EXTENSION_Pointer;
/**
\ingroup SslCrtdSslAPI
More information about the squid-dev
mailing list