From 35b3559c3f45d8affdcf3efd6a99b7e58d89f248 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Wed, 6 Jul 2016 11:37:12 +1200 Subject: [PATCH] Replace LockingPointer::reset with resetWithoutLocking This required the GnuTLS and void session pointer definitions being made to use LockingPointer. Also, use std::move assignment instead of X.reset(Y.release()) pattern. --- src/anyp/PortCfg.cc | 2 +- src/client_side_request.cc | 2 +- src/comm.cc | 2 +- src/fde.h | 2 +- src/security/LockingPointer.h | 10 +++++----- src/security/ServerOptions.cc | 2 +- src/security/Session.h | 8 +++++--- .../cert_generators/file/certificate_db.cc | 4 ++-- .../file/security_file_certgen.cc | 4 ++-- src/ssl/PeekingPeerConnector.cc | 4 ++-- src/ssl/gadgets.cc | 20 +++++++++---------- src/ssl/support.cc | 12 +++++------ 12 files changed, 37 insertions(+), 35 deletions(-) diff --git a/src/anyp/PortCfg.cc b/src/anyp/PortCfg.cc index dfcc2dd9de..2f39f00255 100644 --- a/src/anyp/PortCfg.cc +++ b/src/anyp/PortCfg.cc @@ -143,7 +143,7 @@ AnyP::PortCfg::configureSslServerContext() } } - secure.staticContext.reset(secure.createStaticServerContext(*this)); + secure.staticContext.resetWithoutLocking(secure.createStaticServerContext(*this)); if (!secure.staticContext) { char buf[128]; fatalf("%s_port %s initialization error", AnyP::ProtocolType_str[transport.protocol], s.toUrl(buf, sizeof(buf))); diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 87e9aa7c88..638202d7ca 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -177,7 +177,7 @@ ClientHttpRequest::ClientHttpRequest(ConnStateData * aConn) : #if USE_OPENSSL if (aConn->clientConnection != NULL && aConn->clientConnection->isOpen()) { if (auto ssl = fd_table[aConn->clientConnection->fd].ssl.get()) - al->cache.sslClientCert.reset(SSL_get_peer_certificate(ssl)); + al->cache.sslClientCert.resetWithoutLocking(SSL_get_peer_certificate(ssl)); } #endif } diff --git a/src/comm.cc b/src/comm.cc index 4ca20ffac0..e606224740 100644 --- a/src/comm.cc +++ b/src/comm.cc @@ -838,7 +838,7 @@ void comm_close_complete(const FdeCbParams ¶ms) { fde *F = &fd_table[params.fd]; - F->ssl.reset(nullptr); + F->ssl.resetWithoutLocking(nullptr); #if USE_OPENSSL if (F->dynamicSslContext) { diff --git a/src/fde.h b/src/fde.h index 35f5387d1f..4978b5e377 100644 --- a/src/fde.h +++ b/src/fde.h @@ -167,7 +167,7 @@ public: halfClosedReader = NULL; read_method = NULL; write_method = NULL; - ssl.reset(nullptr); + ssl.resetWithoutLocking(nullptr); dynamicSslContext = NULL; #if _SQUID_WINDOWS_ win32.handle = (long)NULL; diff --git a/src/security/LockingPointer.h b/src/security/LockingPointer.h index ddb14da90b..7e736211e8 100644 --- a/src/security/LockingPointer.h +++ b/src/security/LockingPointer.h @@ -39,10 +39,10 @@ namespace Security * absorption, locking, and unlocking implementations. The API largely * follows std::shared_ptr. * - * The constructor and the reset() method import a raw Object pointer. + * The constructor and the resetWithoutLocking() method import a raw Object pointer. * Normally, reset() would lock(), but libraries like OpenSSL * pre-lock objects before they are fed to LockingPointer, necessitating - * this customization hook. + * this resetWithoutLocking() customization hook. * * The lock() method increments Object's reference counter. * @@ -78,7 +78,7 @@ public: explicit LockingPointer(SelfType &&) = default; SelfType &operator =(SelfType &&o) { if (o.get() != raw) - reset(o.release()); + resetWithoutLocking(o.release()); return *this; } @@ -89,14 +89,14 @@ public: T *get() const { return raw; } /// Reset raw pointer - unlock any previous one and save new one without locking. - void reset(T *t) { + void resetWithoutLocking(T *t) { unlock(); raw = t; } void resetAndLock(T *t) { if (t != get()) { - reset(t); + resetWithoutLocking(t); lock(t); } } diff --git a/src/security/ServerOptions.cc b/src/security/ServerOptions.cc index c6be66a8fc..fe37177de2 100644 --- a/src/security/ServerOptions.cc +++ b/src/security/ServerOptions.cc @@ -159,7 +159,7 @@ Security::ServerOptions::loadDhParams() } } - parsedDhParams.reset(dhp); + parsedDhParams.resetWithoutLocking(dhp); #endif } diff --git a/src/security/Session.h b/src/security/Session.h index ae2a8ba0fd..fb009df20f 100644 --- a/src/security/Session.h +++ b/src/security/Session.h @@ -38,13 +38,15 @@ typedef gnutls_session_t SessionPtr; // 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 std::unique_ptr> SessionPointer; +//typedef std::unique_ptr> SessionPointer; +CtoCpp1(gnutls_deinit, gnutls_session_t); +typedef LockingPointer SessionPointer; #else // use void* so we can check against NULL typedef void* SessionPtr; -// use nullptr_t so default_delete works -typedef std::unique_ptr SessionPointer; +CtoCpp1(xfree, SessionPtr); +typedef LockingPointer SessionPointer; #endif diff --git a/src/security/cert_generators/file/certificate_db.cc b/src/security/cert_generators/file/certificate_db.cc index b91512402d..89ec323828 100644 --- a/src/security/cert_generators/file/certificate_db.cc +++ b/src/security/cert_generators/file/certificate_db.cc @@ -310,8 +310,8 @@ bool Ssl::CertificateDb::addCertAndPrivateKey(Security::CertPointer & cert, Ssl: Ssl::EVP_PKEY_Pointer findPkey; if (pure_find(useName.empty() ? subject.get() : useName, findCert, findPkey)) { // Replace with database certificate - cert.reset(findCert.release()); - pkey.reset(findPkey.release()); + cert = std::move(findCert); + pkey = std::move(findPkey); return true; } // pure_find may fail because the entry is expired, or because the diff --git a/src/security/cert_generators/file/security_file_certgen.cc b/src/security/cert_generators/file/security_file_certgen.cc index ab9f9a9df5..63e96c889d 100644 --- a/src/security/cert_generators/file/security_file_certgen.cc +++ b/src/security/cert_generators/file/security_file_certgen.cc @@ -204,8 +204,8 @@ static bool processNewRequest(Ssl::CrtdMessage & request_message, std::string co if (!Ssl::certificateMatchesProperties(cert.get(), certProperties)) { // The certificate changed (renewed or other reason). // Generete a new one with the updated fields. - cert.reset(NULL); - pkey.reset(NULL); + cert.resetWithoutLocking(nullptr); + pkey.resetWithoutLocking(nullptr); db.purgeCert(cert_subject); } } diff --git a/src/ssl/PeekingPeerConnector.cc b/src/ssl/PeekingPeerConnector.cc index a2db2e16ff..dec4a337f8 100644 --- a/src/ssl/PeekingPeerConnector.cc +++ b/src/ssl/PeekingPeerConnector.cc @@ -339,7 +339,7 @@ Ssl::PeekingPeerConnector::handleServerCertificate() // remember the server certificate for later use if (Ssl::ServerBump *serverBump = csd->serverBump()) { - serverBump->serverCert.reset(serverCert.release()); + serverBump->serverCert = std::move(serverCert); } } } @@ -354,7 +354,7 @@ Ssl::PeekingPeerConnector::serverCertificateVerified() else { const int fd = serverConnection()->fd; Security::SessionPtr ssl = fd_table[fd].ssl.get(); - serverCert.reset(SSL_get_peer_certificate(ssl)); + serverCert.resetWithoutLocking(SSL_get_peer_certificate(ssl)); } if (serverCert.get()) { csd->resetSslCommonName(Ssl::CommonHostName(serverCert.get())); diff --git a/src/ssl/gadgets.cc b/src/ssl/gadgets.cc index 9d594be572..3fc5f5d655 100644 --- a/src/ssl/gadgets.cc +++ b/src/ssl/gadgets.cc @@ -130,12 +130,12 @@ bool Ssl::readCertAndPrivateKeyFromMemory(Security::CertPointer & cert, Ssl::EVP BIO_puts(bio.get(), bufferToRead); X509 * certPtr = NULL; - cert.reset(PEM_read_bio_X509(bio.get(), &certPtr, 0, 0)); + cert.resetWithoutLocking(PEM_read_bio_X509(bio.get(), &certPtr, 0, 0)); if (!cert) return false; EVP_PKEY * pkeyPtr = NULL; - pkey.reset(PEM_read_bio_PrivateKey(bio.get(), &pkeyPtr, 0, 0)); + pkey.resetWithoutLocking(PEM_read_bio_PrivateKey(bio.get(), &pkeyPtr, 0, 0)); if (!pkey) return false; @@ -148,7 +148,7 @@ bool Ssl::readCertFromMemory(Security::CertPointer & cert, char const * bufferTo BIO_puts(bio.get(), bufferToRead); X509 * certPtr = NULL; - cert.reset(PEM_read_bio_X509(bio.get(), &certPtr, 0, 0)); + cert.resetWithoutLocking(PEM_read_bio_X509(bio.get(), &certPtr, 0, 0)); if (!cert) return false; @@ -511,7 +511,7 @@ static bool generateFakeSslCertificate(Security::CertPointer & certToStore, Ssl: if (properties.signWithPkey.get()) pkey.resetAndLock(properties.signWithPkey.get()); else // if not exist generate one - pkey.reset(Ssl::createSslPrivateKey()); + pkey.resetWithoutLocking(Ssl::createSslPrivateKey()); if (!pkey) return false; @@ -550,8 +550,8 @@ static bool generateFakeSslCertificate(Security::CertPointer & certToStore, Ssl: if (!ret) return false; - certToStore.reset(cert.release()); - pkeyToStore.reset(pkey.release()); + certToStore = std::move(cert); + pkeyToStore = std::move(pkey); return true; } @@ -676,11 +676,11 @@ void Ssl::readCertAndPrivateKeyFromFiles(Security::CertPointer & cert, Ssl::EVP_ { if (keyFilename == NULL) keyFilename = certFilename; - pkey.reset(readSslPrivateKey(keyFilename)); - cert.reset(readSslX509Certificate(certFilename)); + pkey.resetWithoutLocking(readSslPrivateKey(keyFilename)); + cert.resetWithoutLocking(readSslX509Certificate(certFilename)); if (!pkey || !cert || !X509_check_private_key(cert.get(), pkey.get())) { - pkey.reset(NULL); - cert.reset(NULL); + pkey.resetWithoutLocking(nullptr); + cert.resetWithoutLocking(nullptr); } } diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 7f9e569ec5..be9f84dea3 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -315,7 +315,7 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) } delete filledCheck->sslErrors; filledCheck->sslErrors = NULL; - filledCheck->serverCert.reset(NULL); + filledCheck->serverCert.resetWithoutLocking(nullptr); } // If the certificate validator is used then we need to allow all errors and // pass them to certficate validator for more processing @@ -1273,11 +1273,11 @@ void Ssl::readCertChainAndPrivateKeyFromFiles(Security::CertPointer & cert, EVP_ // XXX: ssl_ask_password_cb needs SSL_CTX_set_default_passwd_cb_userdata() // so this may not fully work iff Config.Program.ssl_password is set. pem_password_cb *cb = ::Config.Program.ssl_password ? &ssl_ask_password_cb : NULL; - pkey.reset(readSslPrivateKey(keyFilename, cb)); - cert.reset(readSslX509CertificatesChain(certFilename, chain.get())); + pkey.resetWithoutLocking(readSslPrivateKey(keyFilename, cb)); + cert.resetWithoutLocking(readSslX509CertificatesChain(certFilename, chain.get())); if (!pkey || !cert || !X509_check_private_key(cert.get(), pkey.get())) { - pkey.reset(NULL); - cert.reset(NULL); + pkey.resetWithoutLocking(nullptr); + cert.resetWithoutLocking(nullptr); } } @@ -1319,7 +1319,7 @@ SslCreate(Security::ContextPtr sslContext, const int fd, Ssl::Bio::Type type, co if (BIO *bio = Ssl::Bio::Create(fd, type)) { Ssl::Bio::Link(ssl, bio); // cannot fail - fd_table[fd].ssl.reset(ssl); + fd_table[fd].ssl.resetWithoutLocking(ssl); fd_table[fd].read_method = &ssl_read_method; fd_table[fd].write_method = &ssl_write_method; fd_note(fd, squidCtx); -- 2.47.2