You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2018/06/13 15:48:47 UTC
[09/10] impala git commit: IMPALA-7145: fix leak of OpenSSL context
when spilling
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 <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/7c5eb933
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/7c5eb933
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/7c5eb933
Branch: refs/heads/2.x
Commit: 7c5eb933b810c476505c9f5a044b749ed630da86
Parents: 2e5143d
Author: Tim Armstrong <ta...@cloudera.com>
Authored: Fri Jun 8 09:31:19 2018 -0700
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Wed Jun 13 03:10:18 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/7c5eb933/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/7c5eb933/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/7c5eb933/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/7c5eb933/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);
}
}