IMPALA-7145: fix leak of OpenSSL context when spilling Add a RAII wrapper for the OpenSSL context that automatically frees on all exit paths from the function.
Add a backend test wrapper that enables LeakSanitizer for an individual test. This is a step towards IMPALA-2746. Fix version check bug in asan.h. Testing: Enable LeakSanitizer for openssl-util-test. This reliably found the bug. Ran core tests under ASAN. Change-Id: I98760ed8f31b18b489a156f945c29c95c9bf3184 Reviewed-on: http://gerrit.cloudera.org:8080/10666 Reviewed-by: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Tested-by: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Project: http://git-wip-us.apache.org/repos/asf/impala/repo Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/501dc6ab Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/501dc6ab Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/501dc6ab Branch: refs/heads/master Commit: 501dc6abfb360575613eb2210c0c87f3e37235ce Parents: dc8dd3e Author: Tim Armstrong <tarmstr...@cloudera.com> Authored: Fri Jun 8 09:31:19 2018 -0700 Committer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Committed: Tue Jun 12 22:08:22 2018 +0000 ---------------------------------------------------------------------- be/CMakeLists.txt | 8 ++++++++ be/src/util/CMakeLists.txt | 2 +- be/src/util/asan.h | 2 +- be/src/util/openssl-util.cc | 40 ++++++++++++++++++++++++++++------------ 4 files changed, 38 insertions(+), 14 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/impala/blob/501dc6ab/be/CMakeLists.txt ---------------------------------------------------------------------- diff --git a/be/CMakeLists.txt b/be/CMakeLists.txt index 2ef7b85..1eed615 100644 --- a/be/CMakeLists.txt +++ b/be/CMakeLists.txt @@ -510,6 +510,14 @@ FUNCTION(ADD_BE_TEST TEST_NAME) ADD_DEPENDENCIES(be-test ${TEST_NAME}) ENDFUNCTION() +# Same as ADD_BE_TEST, but also enable LeakSanitizer. +# TODO: IMPALA-2746: we should make this the default. +FUNCTION(ADD_BE_LSAN_TEST TEST_NAME) + ADD_BE_TEST(${TEST_NAME}) + SET_TESTS_PROPERTIES(${TEST_NAME} PROPERTIES ENVIRONMENT + "ASAN_OPTIONS=handle_segv=0 detect_leaks=1 allocator_may_return_null=1") +ENDFUNCTION() + # Similar utility function for tests that use the UDF SDK FUNCTION(ADD_UDF_TEST TEST_NAME) # This gets the directory where the test is from (e.g. 'exprs' or 'runtime') http://git-wip-us.apache.org/repos/asf/impala/blob/501dc6ab/be/src/util/CMakeLists.txt ---------------------------------------------------------------------- diff --git a/be/src/util/CMakeLists.txt b/be/src/util/CMakeLists.txt index 8e6c7c1..095193d 100644 --- a/be/src/util/CMakeLists.txt +++ b/be/src/util/CMakeLists.txt @@ -126,7 +126,7 @@ ADD_BE_TEST(logging-support-test) ADD_BE_TEST(lru-cache-test) ADD_BE_TEST(metrics-test) ADD_BE_TEST(min-max-filter-test) -ADD_BE_TEST(openssl-util-test) +ADD_BE_LSAN_TEST(openssl-util-test) ADD_BE_TEST(parse-util-test) ADD_BE_TEST(pretty-printer-test) ADD_BE_TEST(proc-info-test) http://git-wip-us.apache.org/repos/asf/impala/blob/501dc6ab/be/src/util/asan.h ---------------------------------------------------------------------- diff --git a/be/src/util/asan.h b/be/src/util/asan.h index d6beb2b..07a970c 100644 --- a/be/src/util/asan.h +++ b/be/src/util/asan.h @@ -29,7 +29,7 @@ #define ASAN_NO_INSTRUMENTATION #endif -#if defined (ADDRESS_SANITIZER) && __clang_major__ >= 3 && __clang_minor__ >= 7 +#if defined(ADDRESS_SANITIZER) #include <sanitizer/lsan_interface.h> #define IGNORE_LEAKING_OBJECT(p) __lsan_ignore_object(p) #else http://git-wip-us.apache.org/repos/asf/impala/blob/501dc6ab/be/src/util/openssl-util.cc ---------------------------------------------------------------------- diff --git a/be/src/util/openssl-util.cc b/be/src/util/openssl-util.cc index 065dbc8..2b66b86 100644 --- a/be/src/util/openssl-util.cc +++ b/be/src/util/openssl-util.cc @@ -91,6 +91,21 @@ bool IsExternalTlsConfigured() { return !FLAGS_ssl_server_certificate.empty() && !FLAGS_ssl_private_key.empty(); } +/// Wrapper around EVP_CIPHER_CTX that automatically cleans up the context +/// when it is destroyed. This helps avoid leaks like IMPALA-7145. +struct ScopedEVPCipherCtx { + DISALLOW_COPY_AND_ASSIGN(ScopedEVPCipherCtx); + + explicit ScopedEVPCipherCtx(int padding) { + EVP_CIPHER_CTX_init(&ctx); + EVP_CIPHER_CTX_set_padding(&ctx, padding); + } + + ~ScopedEVPCipherCtx() { EVP_CIPHER_CTX_cleanup(&ctx); } + + EVP_CIPHER_CTX ctx; +}; + // Callback used by OpenSSLErr() - write the error given to us through buf to the // stringstream that's passed in through ctx. static int OpenSSLErrCallback(const char* buf, size_t len, void* ctx) { @@ -147,9 +162,7 @@ Status EncryptionKey::EncryptInternal( DCHECK_GE(len, 0); const char* err_context = encrypt ? "encrypting" : "decrypting"; // Create and initialize the context for encryption - EVP_CIPHER_CTX ctx; - EVP_CIPHER_CTX_init(&ctx); - EVP_CIPHER_CTX_set_padding(&ctx, 0); + ScopedEVPCipherCtx ctx(0); // Start encryption/decryption. We use a 256-bit AES key, and the cipher block mode // is either CTR or CFB(stream cipher), both of which support arbitrary length @@ -157,13 +170,14 @@ Status EncryptionKey::EncryptInternal( // mode is well-optimized(instruction level parallelism) with hardware acceleration // on x86 and PowerPC const EVP_CIPHER* evpCipher = GetCipher(); - int success = encrypt ? EVP_EncryptInit_ex(&ctx, evpCipher, NULL, key_, iv_) : - EVP_DecryptInit_ex(&ctx, evpCipher, NULL, key_, iv_); + int success = encrypt ? EVP_EncryptInit_ex(&ctx.ctx, evpCipher, NULL, key_, iv_) : + EVP_DecryptInit_ex(&ctx.ctx, evpCipher, NULL, key_, iv_); if (success != 1) { return OpenSSLErr(encrypt ? "EVP_EncryptInit_ex" : "EVP_DecryptInit_ex", err_context); } if (IsGcmMode()) { - if (EVP_CIPHER_CTX_ctrl(&ctx, EVP_CTRL_GCM_SET_IVLEN, AES_BLOCK_SIZE, NULL) != 1) { + if (EVP_CIPHER_CTX_ctrl(&ctx.ctx, EVP_CTRL_GCM_SET_IVLEN, AES_BLOCK_SIZE, NULL) + != 1) { return OpenSSLErr("EVP_CIPHER_CTX_ctrl", err_context); } } @@ -175,8 +189,8 @@ Status EncryptionKey::EncryptInternal( int in_len = static_cast<int>(min<int64_t>(len - offset, numeric_limits<int>::max())); int out_len; success = encrypt ? - EVP_EncryptUpdate(&ctx, out + offset, &out_len, data + offset, in_len) : - EVP_DecryptUpdate(&ctx, out + offset, &out_len, data + offset, in_len); + EVP_EncryptUpdate(&ctx.ctx, out + offset, &out_len, data + offset, in_len) : + EVP_DecryptUpdate(&ctx.ctx, out + offset, &out_len, data + offset, in_len); if (success != 1) { return OpenSSLErr(encrypt ? "EVP_EncryptUpdate" : "EVP_DecryptUpdate", err_context); } @@ -187,21 +201,23 @@ Status EncryptionKey::EncryptInternal( if (IsGcmMode() && !encrypt) { // Set expected tag value - if (EVP_CIPHER_CTX_ctrl(&ctx, EVP_CTRL_GCM_SET_TAG, AES_BLOCK_SIZE, gcm_tag_) != 1) { + if (EVP_CIPHER_CTX_ctrl(&ctx.ctx, EVP_CTRL_GCM_SET_TAG, AES_BLOCK_SIZE, gcm_tag_) + != 1) { return OpenSSLErr("EVP_CIPHER_CTX_ctrl", err_context); } } // Finalize encryption or decryption. int final_out_len; - success = encrypt ? EVP_EncryptFinal_ex(&ctx, out + offset, &final_out_len) : - EVP_DecryptFinal_ex(&ctx, out + offset, &final_out_len); + success = encrypt ? EVP_EncryptFinal_ex(&ctx.ctx, out + offset, &final_out_len) : + EVP_DecryptFinal_ex(&ctx.ctx, out + offset, &final_out_len); if (success != 1) { return OpenSSLErr(encrypt ? "EVP_EncryptFinal" : "EVP_DecryptFinal", err_context); } if (IsGcmMode() && encrypt) { - if (EVP_CIPHER_CTX_ctrl(&ctx, EVP_CTRL_GCM_GET_TAG, AES_BLOCK_SIZE, gcm_tag_) != 1) { + if (EVP_CIPHER_CTX_ctrl(&ctx.ctx, EVP_CTRL_GCM_GET_TAG, AES_BLOCK_SIZE, gcm_tag_) + != 1) { return OpenSSLErr("EVP_CIPHER_CTX_ctrl", err_context); } }
