From 1644f5d12afded9b8b2bcd444ed3ff56ac1db545 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Wed, 9 Mar 2022 13:26:20 -0800 Subject: [PATCH] Fixed memory leaks with CryptoContext --- include/tscore/CryptoHash.h | 20 ++++++--- include/tscore/MD5.h | 3 -- include/tscore/SHA256.h | 13 +++--- src/traffic_server/traffic_server.cc | 12 ------ src/tscore/CryptoHash.cc | 13 +----- src/tscore/Makefile.am | 1 + src/tscore/unit_tests/test_CryptoHash.cc | 55 ++++++++++++++++++++++++ 7 files changed, 76 insertions(+), 41 deletions(-) create mode 100644 src/tscore/unit_tests/test_CryptoHash.cc diff --git a/include/tscore/CryptoHash.h b/include/tscore/CryptoHash.h index 767cb7b64fe..4c2d97e3ff6 100644 --- a/include/tscore/CryptoHash.h +++ b/include/tscore/CryptoHash.h @@ -23,6 +23,7 @@ #pragma once #include "tscore/BufferWriter.h" +#include #include /// Apache Traffic Server commons. @@ -132,6 +133,9 @@ class CryptoContextBase /// @note This is just as fast as the previous style, as a new context must be initialized /// every time this is done. bool hash_immediate(CryptoHash &hash, void const *data, int length); + +protected: + EVP_MD_CTX *_ctx = nullptr; }; inline bool @@ -159,29 +163,31 @@ class CryptoContext : public CryptoContextBase UNSPECIFIED, #if TS_ENABLE_FIPS == 0 MD5, - MMH, #endif SHA256, }; ///< What type of hash we really are. static HashType Setting; - /// Size of storage for placement @c new of hashing context. - static size_t const OBJ_SIZE = 256; + ~CryptoContext() + { + delete _base; + _base = nullptr; + } -protected: - char _obj[OBJ_SIZE]; ///< Raw storage for instantiated context. +private: + CryptoContextBase *_base = nullptr; }; inline bool CryptoContext::update(void const *data, int length) { - return reinterpret_cast(_obj)->update(data, length); + return _base->update(data, length); } inline bool CryptoContext::finalize(CryptoHash &hash) { - return reinterpret_cast(_obj)->finalize(hash); + return _base->finalize(hash); } ts::BufferWriter &bwformat(ts::BufferWriter &w, ts::BWFSpec const &spec, ats::CryptoHash const &hash); diff --git a/include/tscore/MD5.h b/include/tscore/MD5.h index 3131197dfef..8b1e5bb26f7 100644 --- a/include/tscore/MD5.h +++ b/include/tscore/MD5.h @@ -29,9 +29,6 @@ class MD5Context : public ats::CryptoContextBase { -protected: - EVP_MD_CTX *_ctx; - public: MD5Context() { diff --git a/include/tscore/SHA256.h b/include/tscore/SHA256.h index 9b3140a028c..446ae0cb896 100644 --- a/include/tscore/SHA256.h +++ b/include/tscore/SHA256.h @@ -29,26 +29,23 @@ class SHA256Context : public ats::CryptoContextBase { -protected: - EVP_MD_CTX *ctx; - public: SHA256Context() { - ctx = EVP_MD_CTX_new(); - EVP_DigestInit_ex(ctx, EVP_sha256(), nullptr); + _ctx = EVP_MD_CTX_new(); + EVP_DigestInit_ex(_ctx, EVP_sha256(), nullptr); } - ~SHA256Context() { EVP_MD_CTX_free(ctx); } + ~SHA256Context() { EVP_MD_CTX_free(_ctx); } /// Update the hash with @a data of @a length bytes. bool update(void const *data, int length) override { - return EVP_DigestUpdate(ctx, data, length); + return EVP_DigestUpdate(_ctx, data, length); } /// Finalize and extract the @a hash. bool finalize(CryptoHash &hash) override { - return EVP_DigestFinal_ex(ctx, hash.u8, nullptr); + return EVP_DigestFinal_ex(_ctx, hash.u8, nullptr); } }; diff --git a/src/traffic_server/traffic_server.cc b/src/traffic_server/traffic_server.cc index 84c95aa7da6..efc7dc4be16 100644 --- a/src/traffic_server/traffic_server.cc +++ b/src/traffic_server/traffic_server.cc @@ -768,18 +768,6 @@ CB_After_Cache_Init() start = ink_atomic_swap(&delay_listen_for_cache, -1); emit_fully_initialized_message(); -#if TS_ENABLE_FIPS == 0 - // Check for cache BC after the cache is initialized and before listen, if possible. - if (cacheProcessor.min_stripe_version._major < CACHE_DB_MAJOR_VERSION) { - // Versions before 23 need the MMH hash. - if (cacheProcessor.min_stripe_version._major < 23) { - Debug("cache_bc", "Pre 4.0 stripe (cache version %d.%d) found, forcing MMH hash for cache URLs", - cacheProcessor.min_stripe_version._major, cacheProcessor.min_stripe_version._minor); - URLHashContext::Setting = URLHashContext::MMH; - } - } -#endif - if (1 == start) { // The delay_listen_for_cache value was 1, therefore the main function // delayed the call to start_HttpProxyServer until we got here. We must diff --git a/src/tscore/CryptoHash.cc b/src/tscore/CryptoHash.cc index 07bf2b68702..35fcbef3a65 100644 --- a/src/tscore/CryptoHash.cc +++ b/src/tscore/CryptoHash.cc @@ -45,25 +45,16 @@ CryptoContext::CryptoContext() case UNSPECIFIED: #if TS_ENABLE_FIPS == 0 case MD5: - new (_obj) MD5Context; - break; - case MMH: - new (_obj) MMHContext; + _base = new MD5Context; break; #else case SHA256: - new (_obj) SHA256Context; + _base = new SHA256Context; break; #endif default: ink_release_assert(!"Invalid global URL hash context"); }; -#if TS_ENABLE_FIPS == 0 - static_assert(CryptoContext::OBJ_SIZE >= sizeof(MD5Context), "bad OBJ_SIZE"); - static_assert(CryptoContext::OBJ_SIZE >= sizeof(MMHContext), "bad OBJ_SIZE"); -#else - static_assert(CryptoContext::OBJ_SIZE >= sizeof(SHA256Context), "bad OBJ_SIZE"); -#endif } /** diff --git a/src/tscore/Makefile.am b/src/tscore/Makefile.am index c4110a73170..e5b6ed6eb3e 100644 --- a/src/tscore/Makefile.am +++ b/src/tscore/Makefile.am @@ -170,6 +170,7 @@ test_tscore_SOURCES = \ unit_tests/test_ArgParser.cc \ unit_tests/test_BufferWriter.cc \ unit_tests/test_BufferWriterFormat.cc \ + unit_tests/test_CryptoHash.cc \ unit_tests/test_Extendible.cc \ unit_tests/test_History.cc \ unit_tests/test_ink_inet.cc \ diff --git a/src/tscore/unit_tests/test_CryptoHash.cc b/src/tscore/unit_tests/test_CryptoHash.cc new file mode 100644 index 00000000000..1544cd1f4ef --- /dev/null +++ b/src/tscore/unit_tests/test_CryptoHash.cc @@ -0,0 +1,55 @@ +/** + @file Test for CryptoHash + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +#include +#include + +#include "tscore/ink_assert.h" +#include "tscore/ink_defs.h" +#include "tscore/CryptoHash.h" +#include "catch.hpp" + +TEST_CASE("CrypoHash", "[libts][CrypoHash]") +{ + CryptoHash hash; + ats::CryptoContext ctx; + std::string_view test = "asdfsfsdfljhasdfkjasdkfuy239874kasjdf"; + std::string_view sha256 = "2602CBA2CC0331EB7C455E9F36030B32CE9BB432A90759075F5A702772BE123B"; + std::string_view md5 = "480AEF8C24AA94B80DC6214ECEC8CD1A"; + + // Hash the test data + ctx.update(test.data(), test.size()); + ctx.finalize(hash); + + // Write the output to a string + char buffer[(CRYPTO_HASH_SIZE * 2) + 1]; + hash.toHexStr(buffer); + + // Compair to a known hash value + if (CryptoContext::Setting == CryptoContext::SHA256) { + REQUIRE(strlen(buffer) == sha256.size()); + REQUIRE(memcmp(sha256.data(), buffer, sha256.size()) == 0); + } else { + REQUIRE(strlen(buffer) == md5.size()); + REQUIRE(memcmp(md5.data(), buffer, md5.size()) == 0); + } +}