[Impala-ASF-CR] IMPALA-7145: fix leak of OpenSSL context when spilling
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10666 ) Change subject: IMPALA-7145: fix leak of OpenSSL context when spilling .. Patch Set 9: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/10666 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I98760ed8f31b18b489a156f945c29c95c9bf3184 Gerrit-Change-Number: 10666 Gerrit-PatchSet: 9 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 12 Jun 2018 22:08:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7145: fix leak of OpenSSL context when spilling
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10666 ) Change subject: 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 Tested-by: Impala Public Jenkins --- M be/CMakeLists.txt M be/src/util/CMakeLists.txt M be/src/util/asan.h M be/src/util/openssl-util.cc 4 files changed, 38 insertions(+), 14 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/10666 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I98760ed8f31b18b489a156f945c29c95c9bf3184 Gerrit-Change-Number: 10666 Gerrit-PatchSet: 10 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7145: fix leak of OpenSSL context when spilling
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10666 ) Change subject: IMPALA-7145: fix leak of OpenSSL context when spilling .. Patch Set 9: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10666 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I98760ed8f31b18b489a156f945c29c95c9bf3184 Gerrit-Change-Number: 10666 Gerrit-PatchSet: 9 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 12 Jun 2018 18:45:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7145: fix leak of OpenSSL context when spilling
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10666 ) Change subject: IMPALA-7145: fix leak of OpenSSL context when spilling .. Patch Set 9: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2642/ -- To view, visit http://gerrit.cloudera.org:8080/10666 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I98760ed8f31b18b489a156f945c29c95c9bf3184 Gerrit-Change-Number: 10666 Gerrit-PatchSet: 9 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 12 Jun 2018 18:45:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7145: fix leak of OpenSSL context when spilling
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10666 ) Change subject: IMPALA-7145: fix leak of OpenSSL context when spilling .. Patch Set 8: Code-Review+2 carry -- To view, visit http://gerrit.cloudera.org:8080/10666 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I98760ed8f31b18b489a156f945c29c95c9bf3184 Gerrit-Change-Number: 10666 Gerrit-PatchSet: 8 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 12 Jun 2018 18:44:43 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7145: fix leak of OpenSSL context when spilling
Hello Sailesh Mukil, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10666 to look at the new patch set (#8). Change subject: 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 --- M be/CMakeLists.txt M be/src/util/CMakeLists.txt M be/src/util/asan.h M be/src/util/openssl-util.cc 4 files changed, 38 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/10666/8 -- To view, visit http://gerrit.cloudera.org:8080/10666 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I98760ed8f31b18b489a156f945c29c95c9bf3184 Gerrit-Change-Number: 10666 Gerrit-PatchSet: 8 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7145: fix leak of OpenSSL context when spilling
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10666 ) Change subject: IMPALA-7145: fix leak of OpenSSL context when spilling .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/10666/7/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/10666/7/be/src/util/openssl-util.cc@103 PS7, Line 103: > DISALLOW_COPY_AND_ASSIGN(ScopedEVPCipherCtx); Done -- To view, visit http://gerrit.cloudera.org:8080/10666 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I98760ed8f31b18b489a156f945c29c95c9bf3184 Gerrit-Change-Number: 10666 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 12 Jun 2018 18:44:15 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7145: fix leak of OpenSSL context when spilling
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10666 ) Change subject: IMPALA-7145: fix leak of OpenSSL context when spilling .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10666 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I98760ed8f31b18b489a156f945c29c95c9bf3184 Gerrit-Change-Number: 10666 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 12 Jun 2018 17:33:53 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7145: fix leak of OpenSSL context when spilling
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/10666 ) Change subject: IMPALA-7145: fix leak of OpenSSL context when spilling .. Patch Set 7: Code-Review+1 (1 comment) One last comment. LGTM otherwise. Feel free to upgrade to a +2 if no one else is looking at this. http://gerrit.cloudera.org:8080/#/c/10666/7/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/10666/7/be/src/util/openssl-util.cc@103 PS7, Line 103: DISALLOW_COPY_AND_ASSIGN(ScopedEVPCipherCtx); -- To view, visit http://gerrit.cloudera.org:8080/10666 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I98760ed8f31b18b489a156f945c29c95c9bf3184 Gerrit-Change-Number: 10666 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 12 Jun 2018 15:53:15 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7145: fix leak of OpenSSL context when spilling
Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/10666 )
Change subject: IMPALA-7145: fix leak of OpenSSL context when spilling
..
Patch Set 7:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/10666/6/be/src/util/asan.h
File be/src/util/asan.h:
http://gerrit.cloudera.org:8080/#/c/10666/6/be/src/util/asan.h@a32
PS6, Line 32:
> Could you add a comment to the commit message as to why you removed this?
It's a dumb bug - clang 5.0 >= 3.7 but this expression returns false.
http://gerrit.cloudera.org:8080/#/c/10666/6/be/src/util/openssl-util.cc
File be/src/util/openssl-util.cc:
http://gerrit.cloudera.org:8080/#/c/10666/6/be/src/util/openssl-util.cc@96
PS6, Line 96: ScopedEVPCip
> We usually prepend the names of these kinds of classes/structs with 'scoped
Done
http://gerrit.cloudera.org:8080/#/c/10666/6/be/src/util/openssl-util.cc@97
PS6, Line 97: explicit ScopedEVPCipherCtx(int padding) {
> Make 'explicit' to avoid implicit conversions.
Done
http://gerrit.cloudera.org:8080/#/c/10666/6/be/src/util/openssl-util.cc@171
PS6, Line 171: ctx.ctx
> nit: Maybe it would be cleaner to add a getter() to the newly added struct,
I didn't want to overengineer it - the plain struct seemed simpler here.
--
To view, visit http://gerrit.cloudera.org:8080/10666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I98760ed8f31b18b489a156f945c29c95c9bf3184
Gerrit-Change-Number: 10666
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Tue, 12 Jun 2018 15:05:41 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7145: fix leak of OpenSSL context when spilling
Hello Sailesh Mukil, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10666 to look at the new patch set (#7). Change subject: 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 --- M be/CMakeLists.txt M be/src/util/CMakeLists.txt M be/src/util/asan.h M be/src/util/openssl-util.cc 4 files changed, 36 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/10666/7 -- To view, visit http://gerrit.cloudera.org:8080/10666 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I98760ed8f31b18b489a156f945c29c95c9bf3184 Gerrit-Change-Number: 10666 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-7145: fix leak of OpenSSL context when spilling
Sailesh Mukil has posted comments on this change. (
http://gerrit.cloudera.org:8080/10666 )
Change subject: IMPALA-7145: fix leak of OpenSSL context when spilling
..
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/10666/6/be/src/util/asan.h
File be/src/util/asan.h:
http://gerrit.cloudera.org:8080/#/c/10666/6/be/src/util/asan.h@a32
PS6, Line 32:
Could you add a comment to the commit message as to why you removed this?
http://gerrit.cloudera.org:8080/#/c/10666/6/be/src/util/openssl-util.cc
File be/src/util/openssl-util.cc:
http://gerrit.cloudera.org:8080/#/c/10666/6/be/src/util/openssl-util.cc@96
PS6, Line 96: EVPCipherCtx
We usually prepend the names of these kinds of classes/structs with 'scoped'.
So renaming to ScopedEVPCipherCtx would be better.
http://gerrit.cloudera.org:8080/#/c/10666/6/be/src/util/openssl-util.cc@97
PS6, Line 97: EVPCipherCtx(int padding) {
Make 'explicit' to avoid implicit conversions.
http://gerrit.cloudera.org:8080/#/c/10666/6/be/src/util/openssl-util.cc@171
PS6, Line 171: ctx.ctx
nit: Maybe it would be cleaner to add a getter() to the newly added struct, to
get the context?
Your call though.
--
To view, visit http://gerrit.cloudera.org:8080/10666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I98760ed8f31b18b489a156f945c29c95c9bf3184
Gerrit-Change-Number: 10666
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Comment-Date: Mon, 11 Jun 2018 18:37:10 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7145: fix leak of OpenSSL context when spilling
Tim Armstrong has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/10666 ) Change subject: 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. Testing: Enable LeakSanitizer for openssl-util-test. This reliably found the bug. Ran core tests under ASAN. Change-Id: I98760ed8f31b18b489a156f945c29c95c9bf3184 --- M be/CMakeLists.txt M be/src/util/CMakeLists.txt M be/src/util/asan.h M be/src/util/openssl-util.cc 4 files changed, 36 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/10666/6 -- To view, visit http://gerrit.cloudera.org:8080/10666 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I98760ed8f31b18b489a156f945c29c95c9bf3184 Gerrit-Change-Number: 10666 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong
[Impala-ASF-CR] IMPALA-7145: fix leak of OpenSSL context when spilling
Tim Armstrong has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/10666 ) Change subject: 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. Testing: Enable LeakSanitizer for openssl-util-test. This reliably found the bug. Ran core tests under ASAN. Change-Id: I98760ed8f31b18b489a156f945c29c95c9bf3184 --- M be/CMakeLists.txt M be/src/util/CMakeLists.txt M be/src/util/asan.h M be/src/util/openssl-util.cc 4 files changed, 35 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/10666/5 -- To view, visit http://gerrit.cloudera.org:8080/10666 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I98760ed8f31b18b489a156f945c29c95c9bf3184 Gerrit-Change-Number: 10666 Gerrit-PatchSet: 5 Gerrit-Owner: Tim Armstrong
[Impala-ASF-CR] IMPALA-7145: fix leak of OpenSSL context when spilling
Tim Armstrong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10666 Change subject: 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. I enabled util/ tests that passed LeakSanitizer with minimal modifications. Any test that starts a JVM has a bunch of false positives. Testing: Enable LeakSanitizer for openssl-util-test. This reliably found the bug. Ran core tests under ASAN. Change-Id: I98760ed8f31b18b489a156f945c29c95c9bf3184 --- M be/CMakeLists.txt M be/src/util/CMakeLists.txt M be/src/util/asan.h M be/src/util/openssl-util.cc 4 files changed, 35 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/10666/4 -- To view, visit http://gerrit.cloudera.org:8080/10666 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I98760ed8f31b18b489a156f945c29c95c9bf3184 Gerrit-Change-Number: 10666 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong
