Fixed memory leaks with CryptoContext

Author: bryancall

include/tscore/CryptoHash.h

@@ -23,6 +23,7 @@
 #pragma once
 
 #include "tscore/BufferWriter.h"
+#include <openssl/evp.h>
 #include <string_view>
 
 /// 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.
+  CryptoContextBase *_base = nullptr;
 };
 
 inline bool
 CryptoContext::update(void const *data, int length)
 {
-  return reinterpret_cast<CryptoContextBase *>(_obj)->update(data, length);
+  return _base->update(data, length);
 }
 
 inline bool
 CryptoContext::finalize(CryptoHash &hash)
 {
-  return reinterpret_cast<CryptoContextBase *>(_obj)->finalize(hash);
+  return _base->finalize(hash);
 }
 
 ts::BufferWriter &bwformat(ts::BufferWriter &w, ts::BWFSpec const &spec, ats::CryptoHash const &hash);

include/tscore/MD5.h

@@ -29,9 +29,6 @@
 
 class MD5Context : public ats::CryptoContextBase
 {
-protected:
-  EVP_MD_CTX *_ctx;
-
 public:
   MD5Context()
   {

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);
   }
 };

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

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
 }
 
 /**

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 \

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 <array>
+#include <string_view>
+
+#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);
+  }
+}