You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Sailesh Mukil (Code Review)" <ge...@cloudera.org> on 2017/10/30 16:28:01 UTC
[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
Sailesh Mukil has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8412
Change subject: IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
......................................................................
IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
A recent patch for IMPALA-5129 introduced a use-after-free bug in
thrift-server-test. It is fixed in this patch.
Change-Id: I2cd434757de2cd384def5b360a479e51812cccca
---
M be/src/rpc/thrift-server-test.cc
1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/12/8412/1
--
To view, visit http://gerrit.cloudera.org:8080/8412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812cccca
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8412 )
Change subject: IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8412/1/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:
http://gerrit.cloudera.org:8080/#/c/8412/1/be/src/rpc/thrift-server-test.cc@109
PS1, Line 109: string current_executable_path;
Brief comment why this needs to be here. Alternative: static member in the ThriftParamsTest class?
--
To view, visit http://gerrit.cloudera.org:8080/8412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812cccca
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Oct 2017 16:41:16 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8412 )
Change subject: IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/8412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812cccca
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Oct 2017 21:14:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8412 )
Change subject: IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
......................................................................
Patch Set 2:
I wonder if the bug is elsewhere and the copy-on-write refcounted string was somehow covering it up - I've seen that before when getting a const char* from a std::string - the memory remains valid until the last reference is destroyed.
--
To view, visit http://gerrit.cloudera.org:8080/8412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812cccca
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Oct 2017 16:18:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8412 )
Change subject: IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
......................................................................
Patch Set 5: Code-Review+2
Thanks for the review Tim. I added a function header comment for InitAuth() documenting this.
Rebase. Carry +2.
--
To view, visit http://gerrit.cloudera.org:8080/8412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812cccca
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Oct 2017 17:21:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8412 )
Change subject: IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
......................................................................
Patch Set 2:
(1 comment)
Thanks for the explanation.
http://gerrit.cloudera.org:8080/#/c/8412/2/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:
http://gerrit.cloudera.org:8080/#/c/8412/2/be/src/rpc/thrift-server-test.cc@111
PS2, Line 111: string current_executable_path;
Correct me if I'm wrong, but it seems like the root of the problem is that InitAuth() does not create a copy of the app name. To avoid problems like this in the future, how about we move this string to authentication.cc and have InitAuth() copy appname argument into this new string. Then callers of InitAuth() don't need to worry about the lifetime of the app name string they pass.
--
To view, visit http://gerrit.cloudera.org:8080/8412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812cccca
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Oct 2017 17:50:35 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8412 )
Change subject: IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
......................................................................
Patch Set 2: Code-Review+1
I'm ok with this to unblock things, but we should make sure to get to the root cause of the problem.
--
To view, visit http://gerrit.cloudera.org:8080/8412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812cccca
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Oct 2017 16:41:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8412 )
Change subject: IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
......................................................................
Patch Set 3:
Looks like I found the real bug. The SASL library takes the string and holds a reference to it instead of copying it in sasl_server_init().
However, when we reinitialize the SASL library, it doesn't take in the new string because it detects that it was already previously initialized:
https://github.com/cyrusimap/cyrus-sasl/blob/master/lib/server.c#L841
And we end up discarding the string that was held by it.
So the fix is to get the string once and make sure it lives as long as the process does.
--
To view, visit http://gerrit.cloudera.org:8080/8412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812cccca
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Oct 2017 17:00:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8412 )
Change subject: IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
......................................................................
IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
A recent patch for IMPALA-5129 introduced a use-after-free bug in
thrift-server-test. It is fixed in this patch.
Change-Id: I2cd434757de2cd384def5b360a479e51812cccca
Reviewed-on: http://gerrit.cloudera.org:8080/8412
Reviewed-by: Sailesh Mukil <sa...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/rpc/auth-provider.h
M be/src/rpc/thrift-server-test.cc
2 files changed, 19 insertions(+), 3 deletions(-)
Approvals:
Sailesh Mukil: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/8412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812cccca
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Tim Armstrong, Alex Behm, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8412
to look at the new patch set (#3).
Change subject: IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
......................................................................
IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
A recent patch for IMPALA-5129 introduced a use-after-free bug in
thrift-server-test. It is fixed in this patch.
Change-Id: I2cd434757de2cd384def5b360a479e51812cccca
---
M be/src/rpc/thrift-server-test.cc
1 file changed, 15 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/12/8412/3
--
To view, visit http://gerrit.cloudera.org:8080/8412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812cccca
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8412 )
Change subject: IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8412/2/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:
http://gerrit.cloudera.org:8080/#/c/8412/2/be/src/rpc/thrift-server-test.cc@111
PS2, Line 111: string current_executable_path;
> Correct me if I'm wrong, but it seems like the root of the problem is that
After moving the string into InitAuth() and storing it in AuthManager (which is a global singleton), the ASAN error still kept showing up. I spent many hours trying to debug this problem and I have no understanding of why it happens. My inclination is that ASAN gets confused by how kudu::PosixEnv::GetExecutablePath() creates a string. It creates a unique_ptr<char[]> and then assign()'s it to a string.
I see no reason why your suggestion shouldn't work, but for some reason this is seen as a heap-use-after free if it is called multiple times.
The only thing that works for now is to leave the code as is in this patch set.
I have 2 attempts to move/copy the string into GetExecutablePath() and their corresponding errors here:
https://github.com/smukil/incubator-impala/commit/d3fedb0d665def4870b787a586e922e361cac2dc (copy)
https://github.com/smukil/incubator-impala/commit/c2e6ca8987054539be70e5ef7e3244c49df2ee40 (move)
Opened IMPALA-6132 to track this.
--
To view, visit http://gerrit.cloudera.org:8080/8412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812cccca
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Oct 2017 05:46:47 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8412 )
Change subject: IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8412/3/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:
http://gerrit.cloudera.org:8080/#/c/8412/3/be/src/rpc/thrift-server-test.cc@145
PS3, Line 145: InitAuth
Can we document this InitAuth() behaviour in the comment? I guess it only matters for tests that reinitialise it but good to have some documentation.
--
To view, visit http://gerrit.cloudera.org:8080/8412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812cccca
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Oct 2017 17:13:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8412 )
Change subject: IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812cccca
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Oct 2017 16:49:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8412
to look at the new patch set (#2).
Change subject: IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
......................................................................
IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
A recent patch for IMPALA-5129 introduced a use-after-free bug in
thrift-server-test. It is fixed in this patch.
Change-Id: I2cd434757de2cd384def5b360a479e51812cccca
---
M be/src/rpc/thrift-server-test.cc
1 file changed, 4 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/12/8412/2
--
To view, visit http://gerrit.cloudera.org:8080/8412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812cccca
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8412 )
Change subject: IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812cccca
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Oct 2017 17:13:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8412 )
Change subject: IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8412/1/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:
http://gerrit.cloudera.org:8080/#/c/8412/1/be/src/rpc/thrift-server-test.cc@109
PS1, Line 109: string current_executable_path;
> Brief comment why this needs to be here. Alternative: static member in the
Added a comment.
Adding this to the ThriftParamsTest will not fix this issue since the InitAuth() code maintains process wide state and does not have a shutdown mechanism.
So, the threads started by it will keep running in the background even when ~ThriftParamsTest() is called, leading to the possibility of the same use-after-free bug.
This is not ideal, but for the Impala process we need to start the authentication code only once. For the purpose of testing, we need to add a clean shutdown mechanism, so that we can start and stop the auth library for each test case. Right now we do it only for one test case. This is tracked by IMPALA-6085.
--
To view, visit http://gerrit.cloudera.org:8080/8412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812cccca
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Oct 2017 17:35:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8412 )
Change subject: IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
......................................................................
Patch Set 2:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1418/
--
To view, visit http://gerrit.cloudera.org:8080/8412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812cccca
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Oct 2017 16:51:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8412 )
Change subject: IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1419/
--
To view, visit http://gerrit.cloudera.org:8080/8412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812cccca
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Oct 2017 17:23:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Tim Armstrong, Alex Behm, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8412
to look at the new patch set (#4).
Change subject: IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
......................................................................
IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test
A recent patch for IMPALA-5129 introduced a use-after-free bug in
thrift-server-test. It is fixed in this patch.
Change-Id: I2cd434757de2cd384def5b360a479e51812cccca
---
M be/src/rpc/auth-provider.h
M be/src/rpc/thrift-server-test.cc
2 files changed, 19 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/12/8412/4
--
To view, visit http://gerrit.cloudera.org:8080/8412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812cccca
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>