From f0941861a140fd2337dd5f220b673fb2aeac2433 Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Fri, 24 Apr 2020 17:27:50 +0200 Subject: [PATCH] dnsdist: Wrap pthread_ objects --- pdns/dnscrypt.cc | 6 --- pdns/dnscrypt.hh | 3 +- pdns/dnsdist-cache.hh | 8 +--- pdns/dnsdist.hh | 11 ++--- pdns/dnsdistdist/dnsdist-backend.cc | 1 - pdns/dnsdistdist/dnsdist-kvs.cc | 2 - pdns/dnsdistdist/dnsdist-kvs.hh | 2 +- pdns/dnsdistdist/dnsdist-rules.hh | 10 ++--- pdns/dnsdistdist/libssl.cc | 30 +++++--------- pdns/dnsdistdist/tcpiohandler.cc | 7 +--- pdns/libssl.hh | 2 +- pdns/lock.hh | 64 ++++++++++++++++++++++++++++- pdns/test-dnsdistpacketcache_cc.cc | 35 ++++++++-------- 13 files changed, 105 insertions(+), 76 deletions(-) diff --git a/pdns/dnscrypt.cc b/pdns/dnscrypt.cc index db096659de..17eb99666f 100644 --- a/pdns/dnscrypt.cc +++ b/pdns/dnscrypt.cc @@ -25,7 +25,6 @@ #include "dolog.hh" #include "dnscrypt.hh" #include "dnswriter.hh" -#include "lock.hh" DNSCryptPrivateKey::DNSCryptPrivateKey() { @@ -125,20 +124,15 @@ DNSCryptQuery::~DNSCryptQuery() DNSCryptContext::~DNSCryptContext() { - pthread_rwlock_destroy(&d_lock); } DNSCryptContext::DNSCryptContext(const std::string& pName, const std::vector& certKeys): d_certKeyPaths(certKeys), providerName(pName) { - pthread_rwlock_init(&d_lock, 0); - reloadCertificates(); } DNSCryptContext::DNSCryptContext(const std::string& pName, const DNSCryptCert& certificate, const DNSCryptPrivateKey& pKey): providerName(pName) { - pthread_rwlock_init(&d_lock, 0); - addNewCertificate(certificate, pKey); } diff --git a/pdns/dnscrypt.hh b/pdns/dnscrypt.hh index 53b09f5fad..a010ebdc5a 100644 --- a/pdns/dnscrypt.hh +++ b/pdns/dnscrypt.hh @@ -51,6 +51,7 @@ private: #include #include "dnsname.hh" +#include "lock.hh" #define DNSCRYPT_PROVIDER_PUBLIC_KEY_SIZE (crypto_sign_ed25519_PUBLICKEYBYTES) #define DNSCRYPT_PROVIDER_PRIVATE_KEY_SIZE (crypto_sign_ed25519_SECRETKEYBYTES) @@ -285,7 +286,7 @@ private: void addNewCertificate(std::shared_ptr& newCert, bool reload=false); - pthread_rwlock_t d_lock; + ReadWriteLock d_lock; std::vector> d_certs; std::vector d_certKeyPaths; DNSName providerName; diff --git a/pdns/dnsdist-cache.hh b/pdns/dnsdist-cache.hh index 4b03ad4151..966f55cf50 100644 --- a/pdns/dnsdist-cache.hh +++ b/pdns/dnsdist-cache.hh @@ -92,22 +92,18 @@ private: public: CacheShard(): d_entriesCount(0) { - pthread_rwlock_init(&d_lock, nullptr); } CacheShard(const CacheShard& old): d_entriesCount(0) { - pthread_rwlock_init(&d_lock, nullptr); - } - ~CacheShard() { - pthread_rwlock_destroy(&d_lock); } + void setSize(size_t maxSize) { d_map.reserve(maxSize); } std::unordered_map d_map; - pthread_rwlock_t d_lock; + ReadWriteLock d_lock; std::atomic d_entriesCount; }; diff --git a/pdns/dnsdist.hh b/pdns/dnsdist.hh index 3785baffd8..949e5a08c7 100644 --- a/pdns/dnsdist.hh +++ b/pdns/dnsdist.hh @@ -574,15 +574,13 @@ typedef std::function(const DNSQuestion* dq)> QueryCoun struct QueryCount { QueryCount() { - pthread_rwlock_init(&queryLock, nullptr); } ~QueryCount() { - pthread_rwlock_destroy(&queryLock); } QueryCountRecords records; QueryCountFilter filter; - pthread_rwlock_t queryLock; + ReadWriteLock queryLock; bool enabled{false}; }; @@ -771,11 +769,10 @@ struct DownstreamState fd = -1; } } - pthread_rwlock_destroy(&d_lock); } boost::uuids::uuid id; std::vector hashes; - mutable pthread_rwlock_t d_lock; + mutable ReadWriteLock d_lock; std::vector sockets; const std::string sourceItfName; std::mutex socketsLock; @@ -913,11 +910,9 @@ struct ServerPool { ServerPool() { - pthread_rwlock_init(&d_lock, nullptr); } ~ServerPool() { - pthread_rwlock_destroy(&d_lock); } const std::shared_ptr getCache() const { return packetCache; }; @@ -997,7 +992,7 @@ struct ServerPool private: ServerPolicy::NumberedServerVector d_servers; - pthread_rwlock_t d_lock; + ReadWriteLock d_lock; bool d_useECS{false}; }; diff --git a/pdns/dnsdistdist/dnsdist-backend.cc b/pdns/dnsdistdist/dnsdist-backend.cc index fbce5dd708..2f7a7dbff8 100644 --- a/pdns/dnsdistdist/dnsdist-backend.cc +++ b/pdns/dnsdistdist/dnsdist-backend.cc @@ -137,7 +137,6 @@ void DownstreamState::setWeight(int newWeight) DownstreamState::DownstreamState(const ComboAddress& remote_, const ComboAddress& sourceAddr_, unsigned int sourceItf_, const std::string& sourceItfName_, size_t numberOfSockets, bool connect=true): sourceItfName(sourceItfName_), remote(remote_), sourceAddr(sourceAddr_), sourceItf(sourceItf_), name(remote_.toStringWithPort()), nameWithAddr(remote_.toStringWithPort()) { - pthread_rwlock_init(&d_lock, nullptr); id = getUniqueID(); threadStarted.clear(); diff --git a/pdns/dnsdistdist/dnsdist-kvs.cc b/pdns/dnsdistdist/dnsdist-kvs.cc index e4cc36b506..090a17c2e2 100644 --- a/pdns/dnsdistdist/dnsdist-kvs.cc +++ b/pdns/dnsdistdist/dnsdist-kvs.cc @@ -117,7 +117,6 @@ bool LMDBKVStore::keyExists(const std::string& key) CDBKVStore::CDBKVStore(const std::string& fname, time_t refreshDelay): d_fname(fname), d_refreshDelay(refreshDelay) { - pthread_rwlock_init(&d_lock, nullptr); d_refreshing.clear(); time_t now = time(nullptr); @@ -129,7 +128,6 @@ CDBKVStore::CDBKVStore(const std::string& fname, time_t refreshDelay): d_fname(f } CDBKVStore::~CDBKVStore() { - pthread_rwlock_destroy(&d_lock); } bool CDBKVStore::reload(const struct stat& st) diff --git a/pdns/dnsdistdist/dnsdist-kvs.hh b/pdns/dnsdistdist/dnsdist-kvs.hh index 63e18fdf5e..997de05a2b 100644 --- a/pdns/dnsdistdist/dnsdist-kvs.hh +++ b/pdns/dnsdistdist/dnsdist-kvs.hh @@ -193,7 +193,7 @@ private: std::unique_ptr d_cdb{nullptr}; std::string d_fname; - pthread_rwlock_t d_lock; + ReadWriteLock d_lock; time_t d_mtime{0}; time_t d_nextCheck{0}; time_t d_refreshDelay{0}; diff --git a/pdns/dnsdistdist/dnsdist-rules.hh b/pdns/dnsdistdist/dnsdist-rules.hh index 0474a1e150..202e0b0553 100644 --- a/pdns/dnsdistdist/dnsdist-rules.hh +++ b/pdns/dnsdistdist/dnsdist-rules.hh @@ -234,13 +234,9 @@ private: public: TimedIPSetRule() { - pthread_rwlock_init(&d_lock4, 0); - pthread_rwlock_init(&d_lock6, 0); } ~TimedIPSetRule() { - pthread_rwlock_destroy(&d_lock4); - pthread_rwlock_destroy(&d_lock6); } bool matches(const DNSQuestion* dq) const override { @@ -302,7 +298,7 @@ public: void cleanup() { - time_t now=time(0); + time_t now = time(nullptr); { WriteLock rl(&d_lock4); @@ -360,8 +356,8 @@ private: }; std::unordered_map d_ip6s; std::unordered_map d_ip4s; - mutable pthread_rwlock_t d_lock4; - mutable pthread_rwlock_t d_lock6; + mutable ReadWriteLock d_lock4; + mutable ReadWriteLock d_lock6; }; diff --git a/pdns/dnsdistdist/libssl.cc b/pdns/dnsdistdist/libssl.cc index 064c1b46d6..fa58e39eec 100644 --- a/pdns/dnsdistdist/libssl.cc +++ b/pdns/dnsdistdist/libssl.cc @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -21,16 +22,18 @@ #if (OPENSSL_VERSION_NUMBER < 0x1010000fL || (defined LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER < 0x2090100fL) /* OpenSSL < 1.1.0 needs support for threading/locking in the calling application. */ -static pthread_mutex_t *openssllocks{nullptr}; + +#include "lock.hh" +static std::vector openssllocks; extern "C" { static void openssl_pthreads_locking_callback(int mode, int type, const char *file, int line) { if (mode & CRYPTO_LOCK) { - pthread_mutex_lock(&(openssllocks[type])); + openssllocks.at(type).lock(); } else { - pthread_mutex_unlock(&(openssllocks[type])); + openssllocks.at(type).unlock(); } } @@ -42,24 +45,15 @@ static unsigned long openssl_pthreads_id_callback() static void openssl_thread_setup() { - openssllocks = (pthread_mutex_t*)OPENSSL_malloc(CRYPTO_num_locks() * sizeof(pthread_mutex_t)); - - for (int i = 0; i < CRYPTO_num_locks(); i++) - pthread_mutex_init(&(openssllocks[i]), NULL); - - CRYPTO_set_id_callback(openssl_pthreads_id_callback); - CRYPTO_set_locking_callback(openssl_pthreads_locking_callback); + openssllocks = std::vector(CRYPTO_num_locks()); + CRYPTO_set_id_callback(&openssl_pthreads_id_callback); + CRYPTO_set_locking_callback(&openssl_pthreads_locking_callback); } static void openssl_thread_cleanup() { - CRYPTO_set_locking_callback(NULL); - - for (int i=0; i& ctx OpenSSLTLSTicketKeysRing::OpenSSLTLSTicketKeysRing(size_t capacity) { - pthread_rwlock_init(&d_lock, nullptr); d_ticketKeys.set_capacity(capacity); } OpenSSLTLSTicketKeysRing::~OpenSSLTLSTicketKeysRing() { - pthread_rwlock_destroy(&d_lock); } void OpenSSLTLSTicketKeysRing::addKey(std::shared_ptr newKey) diff --git a/pdns/dnsdistdist/tcpiohandler.cc b/pdns/dnsdistdist/tcpiohandler.cc index 9823ae569c..b4ea4c9677 100644 --- a/pdns/dnsdistdist/tcpiohandler.cc +++ b/pdns/dnsdistdist/tcpiohandler.cc @@ -791,8 +791,6 @@ public: throw std::runtime_error("Error setting up TLS cipher preferences to '" + fe.d_tlsConfig.d_ciphers + "' (" + gnutls_strerror(rc) + ") on " + fe.d_addr.toStringWithPort()); } - pthread_rwlock_init(&d_lock, nullptr); - try { if (fe.d_tlsConfig.d_ticketKeyFile.empty()) { handleTicketsKeyRotation(time(nullptr)); @@ -802,15 +800,12 @@ public: } } catch(const std::runtime_error& e) { - pthread_rwlock_destroy(&d_lock); throw std::runtime_error("Error generating tickets key for TLS context on " + fe.d_addr.toStringWithPort() + ": " + e.what()); } } virtual ~GnuTLSIOCtx() override { - pthread_rwlock_destroy(&d_lock); - d_creds.reset(); if (d_priorityCache) { @@ -876,7 +871,7 @@ private: std::unique_ptr d_creds; gnutls_priority_t d_priorityCache{nullptr}; std::shared_ptr d_ticketsKey{nullptr}; - pthread_rwlock_t d_lock; + ReadWriteLock d_lock; bool d_enableTickets{true}; }; diff --git a/pdns/libssl.hh b/pdns/libssl.hh index b113592555..bdca6eaa14 100644 --- a/pdns/libssl.hh +++ b/pdns/libssl.hh @@ -94,7 +94,7 @@ public: private: boost::circular_buffer > d_ticketKeys; - pthread_rwlock_t d_lock; + ReadWriteLock d_lock; }; void* libssl_get_ticket_key_callback_data(SSL* s); diff --git a/pdns/lock.hh b/pdns/lock.hh index 7e9a11547b..d3933059f5 100644 --- a/pdns/lock.hh +++ b/pdns/lock.hh @@ -54,6 +54,33 @@ public: } }; +class ReadWriteLock +{ +public: + ReadWriteLock() + { + if (pthread_rwlock_init(&d_lock, nullptr) != 0) { + throw std::runtime_error("Error creating a read-write lock: " + stringerror()); + } + } + + ~ReadWriteLock() { + /* might have been moved */ + pthread_rwlock_destroy(&d_lock); + } + + ReadWriteLock(const ReadWriteLock& rhs) = delete; + ReadWriteLock& operator=(const ReadWriteLock& rhs) = delete; + + pthread_rwlock_t* getLock() + { + return &d_lock; + } + +private: + pthread_rwlock_t d_lock; +}; + class WriteLock { pthread_rwlock_t *d_lock; @@ -77,6 +104,14 @@ public: pthread_rwlock_unlock(d_lock); } + WriteLock(ReadWriteLock& lock): WriteLock(lock.getLock()) + { + } + + WriteLock(ReadWriteLock* lock): WriteLock(lock->getLock()) + { + } + WriteLock(WriteLock&& rhs) { d_lock = rhs.d_lock; @@ -118,7 +153,14 @@ public: rhs.d_havelock = false; } - + TryWriteLock(ReadWriteLock& lock): TryWriteLock(lock.getLock()) + { + } + + TryWriteLock(ReadWriteLock* lock): TryWriteLock(lock->getLock()) + { + } + ~TryWriteLock() { if(g_singleThreaded) @@ -157,6 +199,15 @@ public: } d_havelock=(err==0); } + + TryReadLock(ReadWriteLock& lock): TryReadLock(lock.getLock()) + { + } + + TryReadLock(ReadWriteLock* lock): TryReadLock(lock->getLock()) + { + } + TryReadLock(TryReadLock&& rhs) { d_lock = rhs.d_lock; @@ -198,6 +249,15 @@ public: throw PDNSException("error acquiring rwlock readlock: "+stringerror(err)); } } + + ReadLock(ReadWriteLock& lock): ReadLock(lock.getLock()) + { + } + + ReadLock(ReadWriteLock* lock): ReadLock(lock->getLock()) + { + } + ~ReadLock() { if(g_singleThreaded) @@ -209,7 +269,7 @@ public: ReadLock(ReadLock&& rhs) { d_lock = rhs.d_lock; - rhs.d_lock=0; + rhs.d_lock = nullptr; } ReadLock(const ReadLock& rhs) = delete; ReadLock& operator=(const ReadLock& rhs) = delete; diff --git a/pdns/test-dnsdistpacketcache_cc.cc b/pdns/test-dnsdistpacketcache_cc.cc index 8e41798cb9..becd1ec2e7 100644 --- a/pdns/test-dnsdistpacketcache_cc.cc +++ b/pdns/test-dnsdistpacketcache_cc.cc @@ -297,14 +297,13 @@ BOOST_AUTO_TEST_CASE(test_PacketCacheNXDomainTTL) { static DNSDistPacketCache g_PC(500000); -static void *threadMangler(void* off) +static void threadMangler(unsigned int offset) { struct timespec queryTime; gettime(&queryTime); // does not have to be accurate ("realTime") in tests try { ComboAddress remote; bool dnssecOK = false; - unsigned int offset=(unsigned int)(unsigned long)off; for(unsigned int counter=0; counter < 100000; ++counter) { DNSName a=DNSName("hello ")+DNSName(std::to_string(counter+offset)); vector query; @@ -337,19 +336,17 @@ static void *threadMangler(void* off) cerr<<"Had error: "< entry; ComboAddress remote; for(unsigned int counter=0; counter < 100000; ++counter) { @@ -373,25 +370,31 @@ static void *threadReader(void* off) cerr<<"Had error in threadReader: "< threads; + for (int i = 0; i < 4; ++i) { + threads.push_back(std::thread(threadMangler, i*1000000UL)); + } + + for (auto& t : threads) { + t.join(); + } + + threads.clear(); BOOST_CHECK_EQUAL(g_PC.getSize() + g_PC.getDeferredInserts() + g_PC.getInsertCollisions(), 400000U); BOOST_CHECK_SMALL(1.0*g_PC.getInsertCollisions(), 10000.0); - for(int i=0; i < 4; ++i) - pthread_create(&tid[i], 0, threadReader, (void*)(i*1000000UL)); - for(int i=0; i < 4 ; ++i) - pthread_join(tid[i], &res); + for (int i = 0; i < 4; ++i) { + threads.push_back(std::thread(threadReader, i*1000000UL)); + } + + for (auto& t : threads) { + t.join(); + } BOOST_CHECK((g_PC.getDeferredInserts() + g_PC.getDeferredLookups() + g_PC.getInsertCollisions()) >= g_missing); } -- 2.39.2