From: Amos Jeffries Date: Sun, 26 Jul 2020 02:21:58 +0000 (+0000) Subject: Convert LRU map into a CLP map (#593) X-Git-Tag: 4.15-20210522-snapshot~78 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=7224761075454417d79b689d839c6e950e49924b;p=thirdparty%2Fsquid.git Convert LRU map into a CLP map (#593) Revamped LruMap implementation, adding support for value-specific TTLs, value-specific memory consumption estimates, memory pooling for value wrappers, and polishing code. Renamed the result to ClpMap. Fixed memory leak of generated fake SSL certificates in Squids configured with dynamic_cert_mem_cache_size=0 (the default is 4MB). Controversially changed sslcrtvalidator_program cache and TTL defaults: * After we started accounting for key lengths in validator cache entries, the old 2048 bytes default size became not just ridiculously small but insufficient to cache the vast majority of cache entries because an entry key contains a PEM certificate sent to the validator (among other validation parameters). A PEM certificate usually consumes at least a few kilobytes. Living the old 2KB default would essentially mean that we are disabling caching (by default). * And if we fix one default, it makes sense to fix the other as well. The old 60 second TTL default felt too conservative. Most certificate invalidations last days, not seconds. We picked one hour because that value is still fairly conservative, used as the default by several other Squid caches, and should allow for decent cache hit ratio. Dropped support for the undocumented "sslcrtvalidator_program ttl=-1" hack that meant unlimited TTL. Large TTL values should be used instead. TODO: The option parser should be revamped to reject too-large values. --- diff --git a/src/SquidMath.h b/src/SquidMath.h index 12bb57c9ab..480d7b931b 100644 --- a/src/SquidMath.h +++ b/src/SquidMath.h @@ -9,6 +9,13 @@ #ifndef _SQUID_SRC_SQUIDMATH_H #define _SQUID_SRC_SQUIDMATH_H +#include "base/forward.h" + +#include +#include + +// TODO: Move to src/base/Math.h and drop the Math namespace + /* Math functions we define locally for Squid */ namespace Math { @@ -21,5 +28,64 @@ double doubleAverage(const double, const double, int, const int); } // namespace Math +// If Sum() performance becomes important, consider using GCC and clang +// built-ins like __builtin_add_overflow() instead of manual overflow checks. + +/// std::enable_if_t replacement until C++14 +/// simplifies Sum() declarations below +template +using EnableIfType = typename std::enable_if::type; + +/// detects a pair of unsigned types +/// reduces code duplication in Sum() declarations below +template +using AllUnsigned = typename std::conditional< + std::is_unsigned::value && std::is_unsigned::value, + std::true_type, + std::false_type + >::type; + +/// \returns a non-overflowing sum of the two unsigned arguments (or nothing) +template ::value, int> = 0> +Optional +Sum(const T a, const U b) { + // Instead of computing the largest type dynamically, we simply go by T and + // reject cases like Sum(0, ULLONG_MAX) that would overflow on return. + // TODO: Consider using std::common_type in the return type instead. + static_assert(sizeof(T) >= sizeof(U), "Sum() return type can fit its (unsigned) result"); + + // this optimized implementation relies on unsigned overflows + static_assert(std::is_unsigned::value, "the first Sum(a,b) argument is unsigned"); + static_assert(std::is_unsigned::value, "the second Sum(a,b) argument is unsigned"); + const auto sum = a + b; + // when a+b overflows, the result becomes smaller than any operand + return (sum < a) ? Optional() : Optional(sum); +} + +/// \returns a non-overflowing sum of the two signed arguments (or nothing) +template ::value, int> = 0> +Optional constexpr +Sum(const T a, const U b) { + // Instead of computing the largest type dynamically, we simply go by T and + // reject cases like Sum(0, LLONG_MAX) that would overflow on return. + static_assert(sizeof(T) >= sizeof(U), "Sum() return type can fit its (signed) result"); + + // tests below avoid undefined behavior of signed under/overflows + return b >= 0 ? + ((a > std::numeric_limits::max() - b) ? Optional() : Optional(a + b)): + ((a < std::numeric_limits::min() - b) ? Optional() : Optional(a + b)); +} + +/// \returns a non-overflowing sum of the arguments (or nothing) +template +Optional +Sum(const T first, Args... args) { + if (const auto others = Sum(args...)) { + return Sum(first, others.value()); + } else { + return Optional(); + } +} + #endif /* _SQUID_SRC_SQUIDMATH_H */ diff --git a/src/base/ClpMap.h b/src/base/ClpMap.h new file mode 100644 index 0000000000..75e47a1ab7 --- /dev/null +++ b/src/base/ClpMap.h @@ -0,0 +1,285 @@ +/* + * Copyright (C) 1996-2020 The Squid Software Foundation and contributors + * + * Squid software is distributed under GPLv2+ license and includes + * contributions from numerous individuals and organizations. + * Please see the COPYING and CONTRIBUTORS files for details. + */ + +#ifndef SQUID__SRC_BASE_CLPMAP_H +#define SQUID__SRC_BASE_CLPMAP_H + +#include "base/Optional.h" +#include "mem/PoolingAllocator.h" +#include "SquidMath.h" +#include "SquidTime.h" + +#include +#include +#include +#include + +template +uint64_t +DefaultMemoryUsage(const Value &e) +{ + return sizeof(e); +} + +/// An in-memory associative container enforcing three primary caching policies: +/// * Capacity: The memory used by cached entries has a configurable limit; +/// * Lifetime: Entries are hidden (and may be deleted) after their TTL expires; +/// * Priority: Capacity victims are purged in LRU order. +/// Individual cache entry operations have average constant-time complexity. +/// +/// Value must meet STL requirements of Erasable and EmplaceConstructible. +/// Key must come with std::hash and std::equal_to instantiations. +/// Key::length() must return the number of memory bytes in use by the key. +/// MemoryUsedBy() must return the number of memory bytes in use by the value. +template +class ClpMap +{ +public: + /// maximum desired entry caching duration (a.k.a. TTL), in seconds + using Ttl = int; + + explicit ClpMap(const uint64_t capacity) { setMemLimit(capacity); } + ClpMap(uint64_t capacity, Ttl defaultTtl); + ~ClpMap() = default; + + // copying disabled because it may be expensive for large maps + // moving (implicitly) disabled for simplicity sake + ClpMap(const ClpMap &) = delete; + ClpMap &operator =(const ClpMap &) = delete; + + /// \return a pointer to a fresh cached value (or nil) + /// The underlying value is owned by the map, so the pointer may be + /// invalidated by any non-constant method call, including another get(). + /// Also moves the found entry to the end of the purging queue. + const Value *get(const Key &); + + /// Copy the given value into the map (with the given key and TTL) + /// \retval true the value was successfully copied into the map + /// \retval false caching was rejected (the map remains unchanged) + bool add(const Key &, const Value &, Ttl); + + /// Copy the given value into the map (with the given key and default TTL) + bool add(const Key &key, const Value &v) { return add(key, v, defaultTtl_); } + + /// Remove the corresponding entry (if any) + void del(const Key &); + + /// Reset the memory capacity for this map, purging if needed + void setMemLimit(uint64_t newLimit); + + /// The memory capacity for the map + uint64_t memLimit() const { return memLimit_; } + + /// The free space of the map + uint64_t freeMem() const { return memLimit() - memoryUsed(); } + + /// The current (approximate) memory usage of the map + uint64_t memoryUsed() const { return memUsed_; } + + /// The number of currently stored entries, including expired ones + size_t entries() const { return entries_.size(); } + +private: + /// the keeper of cache entry Key, Value, and caching-related entry metadata + class Entry + { + public: + Entry(const Key &, const Value &, const Ttl); + + /// whether the entry is stale + bool expired() const { return expires < squid_curtime; } + + public: + Key key; ///< the entry search key; see ClpMap::get() + Value value; ///< cached value provided by the map user + time_t expires = 0; ///< get() stops returning the entry after this time + uint64_t memCounted = 0; ///< memory accounted for this entry in our ClpMap + }; + + /// Entries in LRU order + using Entries = std::list >; + using EntriesIterator = typename Entries::iterator; + + using IndexItem = std::pair; + /// key:entry_position mapping for fast entry lookups by key + using Index = std::unordered_map, std::equal_to, PoolingAllocator >; + using IndexIterator = typename Index::iterator; + + static Optional MemoryCountedFor(const Key &, const Value &); + + void trim(uint64_t wantSpace); + void erase(const IndexIterator &); + IndexIterator find(const Key &); + + /// cached entries, including expired ones, in LRU order + Entries entries_; + + /// entries_ positions indexed by the entry key + Index index_; + + /// entry TTL to use if none provided to add() + Ttl defaultTtl_ = std::numeric_limits::max(); + + /// the maximum memory we are allowed to use for all cached entries + uint64_t memLimit_ = 0; + + /// the total amount of memory we currently use for all cached entries + uint64_t memUsed_ = 0; +}; + +template +ClpMap::ClpMap(const uint64_t capacity, const Ttl defaultTtl): + defaultTtl_(defaultTtl) +{ + assert(defaultTtl >= 0); + setMemLimit(capacity); +} + +template +void +ClpMap::setMemLimit(const uint64_t newLimit) +{ + assert(newLimit >= 0); + if (memUsed_ > newLimit) + trim(memLimit_ - newLimit); + memLimit_ = newLimit; +} + +/// \returns the index position of an entry identified by its key (or end()) +template +typename ClpMap::IndexIterator +ClpMap::find(const Key &key) +{ + const auto i = index_.find(key); + if (i == index_.end()) + return i; + + const auto entryPosition = i->second; + if (!entryPosition->expired()) { + if (entryPosition != entries_.begin()) + entries_.splice(entries_.begin(), entries_, entryPosition); + return i; + } + // else fall through to cleanup + + erase(i); + return index_.end(); +} + +template +const Value * +ClpMap::get(const Key &key) +{ + const auto i = find(key); + if (i != index_.end()) { + const auto &entry = *(i->second); + return &entry.value; + } + return nullptr; +} + +template +Optional +ClpMap::MemoryCountedFor(const Key &k, const Value &v) +{ + // Both storage and index store keys, but we count keySz once, assuming that + // copying a Key does not consume more memory. This assumption holds for + // Key=SBuf, but, ideally, we should be outsourcing this decision to another + // configurable function, storing each key once, or hard-coding Key=SBuf. + const auto keySz = k.length(); + + // approximate calculation (e.g., containers store wrappers not value_types) + return Sum( + keySz, + // storage + sizeof(typename Entries::value_type), + MemoryUsedBy(v), + // index + sizeof(typename Index::value_type)); +} + +template +bool +ClpMap::add(const Key &key, const Value &v, const Ttl ttl) +{ + // optimization: avoid del() search, MemoryCountedFor() in always-empty maps + if (memLimit() == 0) + return false; + + del(key); + + if (ttl < 0) + return false; // already expired; will never be returned by get() + + const auto memoryRequirements = MemoryCountedFor(key, v); + if (!memoryRequirements) + return false; // cannot even compute memory requirements + + const auto wantSpace = memoryRequirements.value(); + if (wantSpace > memLimit() || wantSpace == 0) // 0 is 64-bit integer overflow + return false; // will never fit + trim(wantSpace); + + entries_.emplace_front(key, v, ttl); // TODO: After C++17 migration, use the return value + index_.emplace(key, entries_.begin()); + + entries_.begin()->memCounted = wantSpace; + memUsed_ += wantSpace; + assert(memUsed_ >= wantSpace); // no overflows + return true; +} + +/// removes the cached entry (identified by its index) from the map +template +void +ClpMap::erase(const IndexIterator &i) +{ + assert(i != index_.end()); + const auto entryPosition = i->second; + + assert(entryPosition != entries_.end()); + const auto sz = entryPosition->memCounted; + assert(memUsed_ >= sz); + memUsed_ -= sz; + + index_.erase(i); // destroys a "pointer" to our Entry + entries_.erase(entryPosition); // destroys our Entry +} + +template +void +ClpMap::del(const Key &key) +{ + const auto i = find(key); + if (i != index_.end()) + erase(i); +} + +/// purges entries to make free memory large enough to fit wantSpace bytes +template +void +ClpMap::trim(const uint64_t wantSpace) +{ + assert(wantSpace <= memLimit()); // no infinite loops and in-vain trimming + while (freeMem() < wantSpace) { + assert(!entries_.empty()); + // TODO: Purge expired entries first. They are useless, but their + // presence may lead to purging potentially useful fresh entries here. + del(entries_.rbegin()->key); + } +} + +template +ClpMap::Entry::Entry(const Key &aKey, const Value &v, const Ttl ttl) : + key(aKey), + value(v), + expires(Sum(squid_curtime, ttl).value_or(std::numeric_limits::max())) +{ +} + +#endif /* SQUID__SRC_BASE_CLPMAP_H */ diff --git a/src/base/LruMap.h b/src/base/LruMap.h deleted file mode 100644 index 803f477286..0000000000 --- a/src/base/LruMap.h +++ /dev/null @@ -1,219 +0,0 @@ -/* - * Copyright (C) 1996-2020 The Squid Software Foundation and contributors - * - * Squid software is distributed under GPLv2+ license and includes - * contributions from numerous individuals and organizations. - * Please see the COPYING and CONTRIBUTORS files for details. - */ - -#ifndef SQUID_LRUMAP_H -#define SQUID_LRUMAP_H - -#include "SquidTime.h" - -#include -#include - -template class LruMap -{ -public: - class Entry - { - public: - Entry(const Key &aKey, EntryValue *t): key(aKey), value(t), date(squid_curtime) {} - ~Entry() {delete value;} - private: - Entry(Entry &); - Entry & operator = (Entry &); - public: - Key key; ///< the key of entry - EntryValue *value; ///< A pointer to the stored value - time_t date; ///< The date the entry created - }; - typedef std::list Queue; - typedef typename std::list::iterator QueueIterator; - - /// key:queue_item mapping for fast lookups by key - typedef std::map Map; - typedef typename Map::iterator MapIterator; - typedef std::pair MapPair; - - LruMap(int ttl, size_t size); - ~LruMap(); - /// Search for an entry, and return a pointer - EntryValue *get(const Key &key); - /// Add an entry to the map - bool add(const Key &key, EntryValue *t); - /// Delete an entry from the map - bool del(const Key &key); - /// (Re-)set the maximum size for this map - void setMemLimit(size_t aSize); - /// The available size for the map - size_t memLimit() const {return memLimit_;} - /// The free space of the map - size_t freeMem() const { return (memLimit() > size() ? memLimit() - size() : 0);} - /// The current size of the map - size_t size() const {return (entries_ * EntryCost);} - /// The number of stored entries - int entries() const {return entries_;} -private: - LruMap(LruMap const &); - LruMap & operator = (LruMap const &); - - bool expired(const Entry &e) const; - void trim(); - void touch(const MapIterator &i); - bool del(const MapIterator &i); - void findEntry(const Key &key, LruMap::MapIterator &i); - - Map storage; ///< The Key/value * pairs - Queue index; ///< LRU cache index - int ttl;///< >0 ttl for caching, == 0 cache is disabled, < 0 store for ever - size_t memLimit_; ///< The maximum memory to use - int entries_; ///< The stored entries -}; - -template -LruMap::LruMap(int aTtl, size_t aSize): entries_(0) -{ - ttl = aTtl; - - setMemLimit(aSize); -} - -template -LruMap::~LruMap() -{ - for (QueueIterator i = index.begin(); i != index.end(); ++i) { - delete *i; - } -} - -template -void -LruMap::setMemLimit(size_t aSize) -{ - if (aSize > 0) - memLimit_ = aSize; - else - memLimit_ = 0; -} - -template -void -LruMap::findEntry(const Key &key, LruMap::MapIterator &i) -{ - i = storage.find(key); - if (i == storage.end()) { - return; - } - index.push_front(*(i->second)); - index.erase(i->second); - i->second = index.begin(); - - if (const Entry *e = *i->second) { - if (!expired(*e)) - return; - // else fall through to cleanup - } - - del(i); - i = storage.end(); -} - -template -EntryValue * -LruMap::get(const Key &key) -{ - MapIterator i; - findEntry(key, i); - if (i != storage.end()) { - touch(i); - Entry *e = *i->second; - return e->value; - } - return NULL; -} - -template -bool -LruMap::add(const Key &key, EntryValue *t) -{ - if (ttl == 0) - return false; - - del(key); - trim(); - - if (memLimit() == 0) - return false; - - index.push_front(new Entry(key, t)); - storage.insert(MapPair(key, index.begin())); - - ++entries_; - return true; -} - -template -bool -LruMap::expired(const LruMap::Entry &entry) const -{ - if (ttl < 0) - return false; - - return (entry.date + ttl < squid_curtime); -} - -template -bool -LruMap::del(LruMap::MapIterator const &i) -{ - if (i != storage.end()) { - Entry *e = *i->second; - index.erase(i->second); - storage.erase(i); - delete e; - --entries_; - return true; - } - return false; -} - -template -bool -LruMap::del(const Key &key) -{ - MapIterator i; - findEntry(key, i); - return del(i); -} - -template -void -LruMap::trim() -{ - while (size() >= memLimit()) { - QueueIterator i = index.end(); - --i; - if (i != index.end()) { - del((*i)->key); - } - } -} - -template -void -LruMap::touch(LruMap::MapIterator const &i) -{ - // this must not be done when nothing is being cached. - if (ttl == 0) - return; - - index.push_front(*(i->second)); - index.erase(i->second); - i->second = index.begin(); -} - -#endif - diff --git a/src/base/Makefile.am b/src/base/Makefile.am index 3d1cc4ac22..4792522f8d 100644 --- a/src/base/Makefile.am +++ b/src/base/Makefile.am @@ -24,6 +24,7 @@ libbase_la_SOURCES = \ CbcPointer.h \ CharacterSet.cc \ CharacterSet.h \ + ClpMap.h \ CodeContext.cc \ CodeContext.h \ EnumIterator.h \ @@ -36,7 +37,7 @@ libbase_la_SOURCES = \ InstanceId.h \ Lock.h \ LookupTable.h \ - LruMap.h \ + Optional.h \ Packable.h \ PackableStream.h \ Range.h \ diff --git a/src/base/Optional.h b/src/base/Optional.h new file mode 100644 index 0000000000..f7c1e5834c --- /dev/null +++ b/src/base/Optional.h @@ -0,0 +1,58 @@ +/* + * Copyright (C) 1996-2020 The Squid Software Foundation and contributors + * + * Squid software is distributed under GPLv2+ license and includes + * contributions from numerous individuals and organizations. + * Please see the COPYING and CONTRIBUTORS files for details. + */ + +#ifndef SQUID__SRC_BASE_OPTIONAL_H +#define SQUID__SRC_BASE_OPTIONAL_H + +#include + +/// std::bad_optional_access replacement (until we upgrade to C++17) +class BadOptionalAccess: public std::exception +{ +public: + BadOptionalAccess() {} + /* std::exception API */ + virtual const char* what() const noexcept override { return "bad-optional-access"; } + virtual ~BadOptionalAccess() noexcept = default; +}; + +/// (limited) std::optional replacement (until we upgrade to C++17) +template +class Optional +{ +public: + // std::optional supports non-trivial types as well, but we + // do not want to fiddle with unions to disable default Value constructor + // until that work becomes necessary + static_assert(std::is_trivial::value, "Value is trivial"); + + constexpr Optional() noexcept {} + constexpr explicit Optional(const Value &v): value_(v), hasValue_(true) {} + + constexpr explicit operator bool() const noexcept { return hasValue_; } + constexpr bool has_value() const noexcept { return hasValue_; } + + const Value &value() const & + { + if (!hasValue_) + throw BadOptionalAccess(); + return value_; + } + + template + constexpr Value value_or(Other &&defaultValue) const & + { + return hasValue_ ? value_ : static_cast(std::forward(defaultValue)); + } + +private: + Value value_; // stored value; inaccessible/uninitialized unless hasValue_ + bool hasValue_ = false; +}; + +#endif /* SQUID__SRC_BASE_OPTIONAL_H */ diff --git a/src/base/forward.h b/src/base/forward.h index 6edd65e451..2527025646 100644 --- a/src/base/forward.h +++ b/src/base/forward.h @@ -14,6 +14,9 @@ class AsyncJob; class CallDialer; class CodeContext; class ScopedId; +class BadOptionalAccess; + +template class Optional; template class CbcPointer; template class RefCount; diff --git a/src/cf.data.pre b/src/cf.data.pre index 0b26d219e0..e43e875a4a 100644 --- a/src/cf.data.pre +++ b/src/cf.data.pre @@ -3406,11 +3406,26 @@ DOC_START Specify the location and options of the executable for ssl_crt_validator process. - Usage: sslcrtvalidator_program [ttl=n] [cache=n] path ... + Usage: sslcrtvalidator_program [ttl=...] [cache=n] path ... Options: - ttl=n TTL in seconds for cached results. The default is 60 secs - cache=n limit the result cache size. The default value is 2048 + + cache=bytes + Limits how much memory Squid can use for caching validator + responses. The default is 67108864 (i.e. 64 MB). + Reconfiguration purges any excess entries. To disable caching, + use cache=0. Currently, cache entry sizes are seriously + underestimated. Even with that bug, a typical estimate for a + single cache entry size would be at least a few kilobytes (the + size of the PEM certificates sent to the validator). + + ttl= + Approximately how long Squid may reuse the validator results + for. The default is 3600 (i.e. 1 hour). Using ttl=infinity + disables TTL checks. Reconfiguration does not affect TTLs of + the already cached entries. To disable caching, use zero cache + size, not zero TTL -- zero TTL allows reuse for the remainder + of the second when the result was cached. DOC_END NAME: sslcrtvalidator_children diff --git a/src/client_side.cc b/src/client_side.cc index 3a502639e4..a026a114a0 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -2802,7 +2802,7 @@ ConnStateData::getTlsContextFromCache(const SBuf &cacheKey, const Ssl::Certifica { debugs(33, 5, "Finding SSL certificate for " << cacheKey << " in cache"); Ssl::LocalContextStorage * ssl_ctx_cache = Ssl::TheGlobalContextStorage.getLocalStorage(port->s); - if (Security::ContextPointer *ctx = ssl_ctx_cache ? ssl_ctx_cache->get(cacheKey) : nullptr) { + if (const auto ctx = ssl_ctx_cache ? ssl_ctx_cache->get(cacheKey) : nullptr) { if (Ssl::verifySslCertificate(*ctx, certProperties)) { debugs(33, 5, "Cached SSL certificate for " << certProperties.commonName << " is valid"); return *ctx; @@ -2819,7 +2819,7 @@ void ConnStateData::storeTlsContextToCache(const SBuf &cacheKey, Security::ContextPointer &ctx) { Ssl::LocalContextStorage *ssl_ctx_cache = Ssl::TheGlobalContextStorage.getLocalStorage(port->s); - if (!ssl_ctx_cache || !ssl_ctx_cache->add(cacheKey, new Security::ContextPointer(ctx))) { + if (!ssl_ctx_cache || !ssl_ctx_cache->add(cacheKey, ctx)) { // If it is not in storage delete after using. Else storage deleted it. fd_table[clientConnection->fd].dynamicTlsContext = ctx; } diff --git a/src/mem/PoolingAllocator.h b/src/mem/PoolingAllocator.h index ff5de1094c..e1f236b284 100644 --- a/src/mem/PoolingAllocator.h +++ b/src/mem/PoolingAllocator.h @@ -25,7 +25,8 @@ public: // The following declarations are only necessary for compilers that do not // fully support C++11 Allocator-related APIs, such as GCC v4.8. - // TODO: Remove after dropping support for such compilers. + // The corresponding std::allocator declarations are deprecated in C++17. + // TODO: Remove after dropping support for deficient compilers. using size_type = size_t; using pointer = Value*; @@ -38,6 +39,7 @@ public: typedef PoolingAllocator other; }; + template void construct(U *p, Args && ... args) { new((void *)p) U(std::forward(args)...); } template void destroy(OtherValue *p) { p->~OtherValue(); } }; diff --git a/src/ssl/cert_validate_message.cc b/src/ssl/cert_validate_message.cc index a8184ef9c5..602d1b1058 100644 --- a/src/ssl/cert_validate_message.cc +++ b/src/ssl/cert_validate_message.cc @@ -197,6 +197,13 @@ Ssl::CertValidationMsg::getCertByName(std::vector const &certs, std::s return NULL; } +uint64_t +Ssl::CertValidationResponse::MemoryUsedByResponse(const CertValidationResponse::Pointer &) +{ + // XXX: This math does not account for most of the response size! + return sizeof(CertValidationResponse); +} + Ssl::CertValidationResponse::RecvdError & Ssl::CertValidationResponse::getError(int errorId) { diff --git a/src/ssl/cert_validate_message.h b/src/ssl/cert_validate_message.h index 822ecde3c0..58c8f51bd8 100644 --- a/src/ssl/cert_validate_message.h +++ b/src/ssl/cert_validate_message.h @@ -60,6 +60,9 @@ public: typedef std::vector RecvdErrors; explicit CertValidationResponse(const Security::SessionPointer &aSession) : ssl(aSession) {} + + static uint64_t MemoryUsedByResponse(const CertValidationResponse::Pointer &); + /// Search in errors list for the error item with id=errorId. /// If none found a new RecvdError item added with the given id; RecvdError &getError(int errorId); diff --git a/src/ssl/context_storage.cc b/src/ssl/context_storage.cc index 20dc491adb..4637b57b33 100644 --- a/src/ssl/context_storage.cc +++ b/src/ssl/context_storage.cc @@ -44,10 +44,12 @@ void Ssl::CertificateStorageAction::dump (StoreEntry *sentry) for (std::map::iterator i = TheGlobalContextStorage.storage.begin(); i != TheGlobalContextStorage.storage.end(); ++i) { stream << i->first << delimiter; LocalContextStorage & ssl_store_policy(*(i->second)); + const auto memoryPerEntry = ssl_store_policy.entries() ? + ssl_store_policy.memoryUsed() / ssl_store_policy.entries() : 0; stream << ssl_store_policy.memLimit() / 1024 << delimiter; stream << ssl_store_policy.entries() << delimiter; - stream << SSL_CTX_SIZE / 1024 << delimiter; - stream << ssl_store_policy.size() / 1024 << delimiter; + stream << memoryPerEntry / 1024 << delimiter; + stream << ssl_store_policy.memoryUsed() / 1024 << delimiter; stream << ssl_store_policy.freeMem() / 1024 << endString; } stream << endString; @@ -112,7 +114,7 @@ void Ssl::GlobalContextStorage::reconfigureFinish() // add new local storages. for (std::map::iterator conf_i = configureStorage.begin(); conf_i != configureStorage.end(); ++conf_i ) { if (storage.find(conf_i->first) == storage.end() && conf_i->second > 0) { - storage.insert(std::pair(conf_i->first, new LocalContextStorage(-1, conf_i->second))); + storage.insert(std::pair(conf_i->first, new LocalContextStorage(conf_i->second))); } } } diff --git a/src/ssl/context_storage.h b/src/ssl/context_storage.h index 73b3a0895b..6df979ae89 100644 --- a/src/ssl/context_storage.h +++ b/src/ssl/context_storage.h @@ -11,7 +11,7 @@ #if USE_OPENSSL -#include "base/LruMap.h" +#include "base/ClpMap.h" #include "CacheManager.h" #include "compat/openssl.h" #include "ip/Address.h" @@ -27,9 +27,6 @@ #include #endif -/// TODO: Replace on real size. -#define SSL_CTX_SIZE 1024 - namespace Ssl { @@ -49,7 +46,10 @@ public: virtual bool aggregatable() const { return false; } }; -typedef LruMap LocalContextStorage; +inline uint64_t MemoryUsedByContext(const Security::ContextPointer &) { + return 1024; // TODO: Calculate approximate memory usage by the context. +} +using LocalContextStorage = ClpMap; /// Class for storing/manipulating LocalContextStorage per local listening address/port. class GlobalContextStorage diff --git a/src/ssl/helper.cc b/src/ssl/helper.cc index f9ae2cd1cb..70721a489c 100644 --- a/src/ssl/helper.cc +++ b/src/ssl/helper.cc @@ -9,17 +9,20 @@ #include "squid.h" #include "../helper.h" #include "anyp/PortCfg.h" +#include "cache_cf.h" #include "fs_io.h" #include "helper/Reply.h" +#include "Parsing.h" #include "SquidConfig.h" #include "SquidString.h" #include "SquidTime.h" +#include "sbuf/Stream.h" #include "ssl/cert_validate_message.h" #include "ssl/Config.h" #include "ssl/helper.h" #include "wordlist.h" -Ssl::CertValidationHelper::LruCache *Ssl::CertValidationHelper::HelperCache = nullptr; +Ssl::CertValidationHelper::CacheType *Ssl::CertValidationHelper::HelperCache = nullptr; #if USE_SSL_CRTD @@ -186,20 +189,30 @@ void Ssl::CertValidationHelper::Init() ssl_crt_validator->eom = '\1'; assert(ssl_crt_validator->cmdline == NULL); - int ttl = 60; - size_t cache = 2048; + /* defaults */ + int ttl = 3600; // 1 hour + size_t cache = 64*1024*1024; // 64 MB { + // TODO: Do this during parseConfigFile() for proper parsing, error handling char *tmp = xstrdup(Ssl::TheConfig.ssl_crt_validator); char *tmp_begin = tmp; char * token = NULL; bool parseParams = true; while ((token = strwordtok(NULL, &tmp))) { if (parseParams) { - if (strncmp(token, "ttl=", 4) == 0) { - ttl = atoi(token + 4); + if (strcmp(token, "ttl=infinity") == 0) { + ttl = std::numeric_limits::max(); + continue; + } else if (strncmp(token, "ttl=", 4) == 0) { + ttl = xatoi(token + 4); + if (ttl < 0) { + throw TextException(ToSBuf("Negative TTL in sslcrtvalidator_program ", Ssl::TheConfig.ssl_crt_validator, + Debug::Extra, "For unlimited TTL, use ttl=infinity"), + Here()); + } continue; } else if (strncmp(token, "cache=", 6) == 0) { - cache = atoi(token + 6); + cache = xatoi(token + 6); continue; } else parseParams = false; @@ -212,7 +225,7 @@ void Ssl::CertValidationHelper::Init() //WARNING: initializing static member in an object initialization method assert(HelperCache == NULL); - HelperCache = new Ssl::CertValidationHelper::LruCache(ttl, cache); + HelperCache = new CacheType(cache, ttl); } void Ssl::CertValidationHelper::Shutdown() @@ -279,9 +292,7 @@ sslCrtvdHandleReplyWrapper(void *data, const ::Helper::Reply &reply) if (Ssl::CertValidationHelper::HelperCache && (validationResponse->resultCode == ::Helper::Okay || validationResponse->resultCode == ::Helper::Error)) { - Ssl::CertValidationResponse::Pointer *item = new Ssl::CertValidationResponse::Pointer(validationResponse); - if (!Ssl::CertValidationHelper::HelperCache->add(crtdvdData->query, item)) - delete item; + (void)Ssl::CertValidationHelper::HelperCache->add(crtdvdData->query, validationResponse); } delete crtdvdData; diff --git a/src/ssl/helper.h b/src/ssl/helper.h index f53a71e127..97949288b9 100644 --- a/src/ssl/helper.h +++ b/src/ssl/helper.h @@ -12,7 +12,7 @@ #if USE_OPENSSL #include "base/AsyncJobCalls.h" -#include "base/LruMap.h" +#include "base/ClpMap.h" #include "helper/forward.h" #include "security/forward.h" #include "ssl/cert_validate_message.h" @@ -54,8 +54,8 @@ public: private: static helper * ssl_crt_validator; ///< helper for management of ssl_crtd. public: - typedef LruMap LruCache; - static LruCache *HelperCache; ///< cache for cert validation helper + typedef ClpMap CacheType; + static CacheType *HelperCache; ///< cache for cert validation helper }; } //namespace Ssl diff --git a/src/ssl/support.cc b/src/ssl/support.cc index c3974c8c9a..2b5bec1621 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -869,7 +869,7 @@ Ssl::configureSSLUsingPkeyAndCertFromMemory(SSL *ssl, const char *data, AnyP::Po } bool -Ssl::verifySslCertificate(Security::ContextPointer &ctx, CertificateProperties const &properties) +Ssl::verifySslCertificate(const Security::ContextPointer &ctx, CertificateProperties const &properties) { #if HAVE_SSL_CTX_GET0_CERTIFICATE X509 * cert = SSL_CTX_get0_certificate(ctx.get()); diff --git a/src/ssl/support.h b/src/ssl/support.h index 7401149b81..28c6e5b3ae 100644 --- a/src/ssl/support.h +++ b/src/ssl/support.h @@ -231,7 +231,7 @@ Security::ContextPointer GenerateSslContext(CertificateProperties const &, Secur \param properties Check if the context certificate matches the given properties \return true if the contexts certificate is valid, false otherwise */ -bool verifySslCertificate(Security::ContextPointer &, CertificateProperties const &); +bool verifySslCertificate(const Security::ContextPointer &, CertificateProperties const &); /** \ingroup ServerProtocolSSLAPI diff --git a/src/tests/stub_libsslsquid.cc b/src/tests/stub_libsslsquid.cc index 9d9eb9cf33..4643032cbb 100644 --- a/src/tests/stub_libsslsquid.cc +++ b/src/tests/stub_libsslsquid.cc @@ -11,6 +11,7 @@ #if USE_OPENSSL #include "fatal.h" +#include "sbuf/Algorithms.h" #include "sbuf/SBuf.h" /* Stub File for the ssl/libsslsquid.la convenience library */ @@ -37,7 +38,7 @@ Ssl::CertificateStorageAction::Pointer Ssl::CertificateStorageAction::Create(con void Ssl::CertificateStorageAction::dump(StoreEntry *sentry) STUB void Ssl::GlobalContextStorage::addLocalStorage(Ip::Address const & address, size_t size_of_store) STUB Ssl::LocalContextStorage *Ssl::GlobalContextStorage::getLocalStorage(Ip::Address const & address) -{ fatal(STUB_API " required"); static Ssl::LocalContextStorage v(0,0); return &v; } +{ fatal(STUB_API " required"); static LocalContextStorage v(0); return &v; } void Ssl::GlobalContextStorage::reconfigureStart() STUB //Ssl::GlobalContextStorage Ssl::TheGlobalContextStorage;