Move malloc failure testing into OPENSSL_malloc

Rather than trying to override the actual malloc symbol, just intercept
OPENSSL_malloc and gate it on a build flag. (When we first wrote these,
OPENSSL_malloc was just an alias for malloc.)

This has several benefits:

- This is cross platform. We don't interfere with sanitizers or the
  libc, or have to mess with global symbols.

- This removes the reason bssl_shim and handshaker linked
  test_support_lib, so we can fix the tes_support_lib / gtest
  dependency.

- If we ever reduce the scope of fallible mallocs, we'll want to
  constrain the tests to only the ones that are fallible. An
  interception strategy like this can do it. Hopefully that will also
  take less time to run in the future.

Also fix the ssl malloc failure tests, as they haven't been working for
a while. (Malloc failure tests still take far too long to run to the end
though. My immediate motivation is less malloc failure and more to tidy
up the build.)

Bug: 563
Change-Id: I32165b8ecbebfdcfde26964e06a404762edd28e3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56925
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>

CMakeLists.txt

@@ -364,6 +364,10 @@ if(CONSTANT_TIME_VALIDATION)
   add_definitions(-DNDEBUG)
 endif()
 
+if(MALLOC_FAILURE_TESTING)
+  add_definitions(-DBORINGSSL_MALLOC_FAILURE_TESTING)
+endif()
+
 function(go_executable dest package)
   set(godeps "${CMAKE_SOURCE_DIR}/util/godeps.go")
   if(NOT CMAKE_GENERATOR STREQUAL "Ninja")

crypto/mem.c

@@ -58,6 +58,7 @@
 
 #include <assert.h>
 #include <stdarg.h>
+#include <stdlib.h>
 #include <stdio.h>
 
 #include <openssl/err.h>
@@ -68,6 +69,12 @@ OPENSSL_MSVC_PRAGMA(warning(push, 3))
 OPENSSL_MSVC_PRAGMA(warning(pop))
 #endif
 
+#if defined(BORINGSSL_MALLOC_FAILURE_TESTING)
+#include <errno.h>
+#include <signal.h>
+#include <unistd.h>
+#endif
+
 #include "internal.h"
 
 
@@ -134,7 +141,68 @@ static const uint8_t kBoringSSLBinaryTag[18] = {
     3, 0,
 };
 
+#if defined(BORINGSSL_MALLOC_FAILURE_TESTING)
+static struct CRYPTO_STATIC_MUTEX malloc_failure_lock =
+    CRYPTO_STATIC_MUTEX_INIT;
+static uint64_t current_malloc_count = 0;
+static uint64_t malloc_number_to_fail = 0;
+static int malloc_failure_enabled = 0, break_on_malloc_fail = 0;
+
+static void malloc_exit_handler(void) {
+  CRYPTO_STATIC_MUTEX_lock_read(&malloc_failure_lock);
+  if (malloc_failure_enabled && current_malloc_count > malloc_number_to_fail) {
+    _exit(88);
+  }
+  CRYPTO_STATIC_MUTEX_unlock_read(&malloc_failure_lock);
+}
+
+static void init_malloc_failure(void) {
+  const char *env = getenv("MALLOC_NUMBER_TO_FAIL");
+  if (env != NULL && env[0] != 0) {
+    char *endptr;
+    malloc_number_to_fail = strtoull(env, &endptr, 10);
+    if (*endptr == 0) {
+      malloc_failure_enabled = 1;
+      atexit(malloc_exit_handler);
+    }
+  }
+  break_on_malloc_fail = getenv("MALLOC_BREAK_ON_FAIL") != NULL;
+}
+
+// should_fail_allocation returns one if the current allocation should fail and
+// zero otherwise.
+static int should_fail_allocation() {
+  static CRYPTO_once_t once = CRYPTO_ONCE_INIT;
+  CRYPTO_once(&once, init_malloc_failure);
+  if (!malloc_failure_enabled) {
+    return 0;
+  }
+
+  // We lock just so multi-threaded tests are still correct, but we won't test
+  // every malloc exhaustively.
+  CRYPTO_STATIC_MUTEX_lock_write(&malloc_failure_lock);
+  int should_fail = current_malloc_count == malloc_number_to_fail;
+  current_malloc_count++;
+  CRYPTO_STATIC_MUTEX_unlock_write(&malloc_failure_lock);
+
+  if (should_fail && break_on_malloc_fail) {
+    raise(SIGTRAP);
+  }
+  if (should_fail) {
+    errno = ENOMEM;
+  }
+  return should_fail;
+}
+
+#else
+static int should_fail_allocation(void) { return 0; }
+#endif
+
 void *OPENSSL_malloc(size_t size) {
+  if (should_fail_allocation()) {
+    return NULL;
+  }
+
   if (OPENSSL_memory_alloc != NULL) {
     assert(OPENSSL_memory_free != NULL);
     assert(OPENSSL_memory_get_size != NULL);
@@ -194,6 +262,10 @@ void OPENSSL_free(void *orig_ptr) {
 }
 
 void *OPENSSL_realloc(void *orig_ptr, size_t new_size) {
+  if (should_fail_allocation()) {
+    return NULL;
+  }
+
   if (orig_ptr == NULL) {
     return OPENSSL_malloc(new_size);
   }

crypto/test/CMakeLists.txt

@@ -5,7 +5,6 @@ add_library(
 
   abi_test.cc
   file_test.cc
-  malloc.cc
   test_util.cc
   wycheproof_util.cc
 )

crypto/test/malloc.cc

@@ -1,143 +0,0 @@
-/* Copyright (c) 2014, Google Inc.
- *
- * Permission to use, copy, modify, and/or distribute this software for any
- * purpose with or without fee is hereby granted, provided that the above
- * copyright notice and this permission notice appear in all copies.
- *
- * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
- * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
- * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY
- * SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
- * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION
- * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
- * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */
-
-#include <openssl/base.h>
-
-#if defined(__GLIBC__) && !defined(__UCLIBC__)
-#define OPENSSL_GLIBC
-#endif
-
-// This file isn't built on ARM or Aarch64 because we link statically in those
-// builds and trying to override malloc in a static link doesn't work. It also
-// requires glibc. It's also disabled on ASan builds as this interferes with
-// ASan's malloc interceptor.
-//
-// TODO(davidben): See if this and ASan's and MSan's interceptors can be made to
-// coexist.
-#if defined(__linux__) && defined(OPENSSL_GLIBC) && !defined(OPENSSL_ARM) && \
-    !defined(OPENSSL_AARCH64) && !defined(OPENSSL_ASAN) &&                   \
-    !defined(OPENSSL_MSAN) && !defined(OPENSSL_TSAN)
-
-#include <errno.h>
-#include <signal.h>
-#include <stdint.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <unistd.h>
-
-#include <new>
-
-
-// This file defines overrides for the standard allocation functions that allow
-// a given allocation to be made to fail for testing. If the program is run
-// with MALLOC_NUMBER_TO_FAIL set to a base-10 number then that allocation will
-// return NULL. If MALLOC_BREAK_ON_FAIL is also defined then the allocation
-// will signal SIGTRAP rather than return NULL.
-//
-// This code is not thread safe.
-
-static uint64_t current_malloc_count = 0;
-static uint64_t malloc_number_to_fail = 0;
-static bool failure_enabled = false, break_on_fail = false, in_call = false;
-
-extern "C" {
-// These are other names for the standard allocation functions.
-extern void *__libc_malloc(size_t size);
-extern void *__libc_calloc(size_t num_elems, size_t size);
-extern void *__libc_realloc(void *ptr, size_t size);
-}
-
-static void exit_handler(void) {
-  if (failure_enabled && current_malloc_count > malloc_number_to_fail) {
-    _exit(88);
-  }
-}
-
-static void cpp_new_handler() {
-  // Return to try again. It won't fail a second time.
-  return;
-}
-
-// should_fail_allocation returns true if the current allocation should fail.
-static bool should_fail_allocation() {
-  static bool init = false;
-
-  if (in_call) {
-    return false;
-  }
-
-  in_call = true;
-
-  if (!init) {
-    const char *env = getenv("MALLOC_NUMBER_TO_FAIL");
-    if (env != NULL && env[0] != 0) {
-      char *endptr;
-      malloc_number_to_fail = strtoull(env, &endptr, 10);
-      if (*endptr == 0) {
-        failure_enabled = true;
-        atexit(exit_handler);
-        std::set_new_handler(cpp_new_handler);
-      }
-    }
-    break_on_fail = (NULL != getenv("MALLOC_BREAK_ON_FAIL"));
-    init = true;
-  }
-
-  in_call = false;
-
-  if (!failure_enabled) {
-    return false;
-  }
-
-  bool should_fail = (current_malloc_count == malloc_number_to_fail);
-  current_malloc_count++;
-
-  if (should_fail && break_on_fail) {
-    raise(SIGTRAP);
-  }
-  return should_fail;
-}
-
-extern "C" {
-
-void *malloc(size_t size) {
-  if (should_fail_allocation()) {
-    errno = ENOMEM;
-    return NULL;
-  }
-
-  return __libc_malloc(size);
-}
-
-void *calloc(size_t num_elems, size_t size) {
-  if (should_fail_allocation()) {
-    errno = ENOMEM;
-    return NULL;
-  }
-
-  return __libc_calloc(num_elems, size);
-}
-
-void *realloc(void *ptr, size_t size) {
-  if (should_fail_allocation()) {
-    errno = ENOMEM;
-    return NULL;
-  }
-
-  return __libc_realloc(ptr, size);
-}
-
-}  // extern "C"
-
-#endif  // defined(linux) && GLIBC && !ARM && !AARCH64 && !ASAN && !TSAN

ssl/test/CMakeLists.txt

@@ -15,7 +15,7 @@ add_executable(
 
 add_dependencies(bssl_shim global_target)
 
-target_link_libraries(bssl_shim test_support_lib ssl crypto)
+target_link_libraries(bssl_shim ssl crypto)
 
 if(CMAKE_SYSTEM_NAME STREQUAL "Linux")
   add_executable(
@@ -33,7 +33,7 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Linux")
 
   add_dependencies(handshaker global_target)
 
-  target_link_libraries(handshaker test_support_lib ssl crypto)
+  target_link_libraries(handshaker ssl crypto)
 else()
   # Declare a dummy target for run_tests to depend on.
   add_custom_target(handshaker)

ssl/test/test_config.cc

@@ -496,25 +496,26 @@ static CRYPTO_once_t once = CRYPTO_ONCE_INIT;
 static int g_config_index = 0;
 static CRYPTO_BUFFER_POOL *g_pool = nullptr;
 
-static void init_once() {
+static bool InitGlobals() {
-  g_config_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, NULL);
+  CRYPTO_once(&once, [] {
-  if (g_config_index < 0) {
+    g_config_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, NULL);
-    abort();
+    g_pool = CRYPTO_BUFFER_POOL_new();
-  }
+  });
-  g_pool = CRYPTO_BUFFER_POOL_new();
+  return g_config_index >= 0 && g_pool != nullptr;
-  if (!g_pool) {
-    abort();
-  }
 }
 
 bool SetTestConfig(SSL *ssl, const TestConfig *config) {
-  CRYPTO_once(&once, init_once);
+  if (!InitGlobals()) {
+    return false;
+  }
   return SSL_set_ex_data(ssl, g_config_index, (void *)config) == 1;
 }
 
 const TestConfig *GetTestConfig(const SSL *ssl) {
-  CRYPTO_once(&once, init_once);
+  if (!InitGlobals()) {
-  return (const TestConfig *)SSL_get_ex_data(ssl, g_config_index);
+    return nullptr;
+  }
+  return static_cast<const TestConfig *>(SSL_get_ex_data(ssl, g_config_index));
 }
 
 static int LegacyOCSPCallback(SSL *ssl, void *arg) {
@@ -1371,13 +1372,16 @@ static bool MaybeInstallCertCompressionAlg(
 }
 
 bssl::UniquePtr<SSL_CTX> TestConfig::SetupCtx(SSL_CTX *old_ctx) const {
+  if (!InitGlobals()) {
+    return nullptr;
+  }
+
   bssl::UniquePtr<SSL_CTX> ssl_ctx(
       SSL_CTX_new(is_dtls ? DTLS_method() : TLS_method()));
   if (!ssl_ctx) {
     return nullptr;
   }
 
-  CRYPTO_once(&once, init_once);
   SSL_CTX_set0_buffer_pool(ssl_ctx.get(), g_pool);
 
   std::string cipher_list = "ALL";

ssl/test/test_state.cc

@@ -32,25 +32,26 @@ static void TestStateExFree(void *parent, void *ptr, CRYPTO_EX_DATA *ad,
   delete ((TestState *)ptr);
 }
 
-static void init_once() {
+static bool InitGlobals() {
-  g_state_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, TestStateExFree);
+  CRYPTO_once(&g_once, [] {
-  if (g_state_index < 0) {
+    g_state_index =
-    abort();
+        SSL_get_ex_new_index(0, nullptr, nullptr, nullptr, TestStateExFree);
-  }
+  });
+  return g_state_index >= 0;
 }
 
 struct timeval *GetClock() {
-  CRYPTO_once(&g_once, init_once);
   return &g_clock;
 }
 
 void AdvanceClock(unsigned seconds) {
-  CRYPTO_once(&g_once, init_once);
   g_clock.tv_sec += seconds;
 }
 
 bool SetTestState(SSL *ssl, std::unique_ptr<TestState> state) {
-  CRYPTO_once(&g_once, init_once);
+  if (!InitGlobals()) {
+    return false;
+  }
   // |SSL_set_ex_data| takes ownership of |state| only on success.
   if (SSL_set_ex_data(ssl, g_state_index, state.get()) == 1) {
     state.release();
@@ -60,8 +61,10 @@ bool SetTestState(SSL *ssl, std::unique_ptr<TestState> state) {
 }
 
 TestState *GetTestState(const SSL *ssl) {
-  CRYPTO_once(&g_once, init_once);
+  if (!InitGlobals()) {
-  return (TestState *)SSL_get_ex_data(ssl, g_state_index);
+    return nullptr;
+  }
+  return static_cast<TestState *>(SSL_get_ex_data(ssl, g_state_index));
 }
 
 static void ssl_ctx_add_session(SSL_SESSION *session, void *void_param) {