You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Michael Ho (Code Review)" <ge...@cloudera.org> on 2017/12/06 17:33:12 UTC
[Impala-ASF-CR] IMPALA-6281: Fix use-after-free in InitAuth()
Michael Ho has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8777
Change subject: IMPALA-6281: Fix use-after-free in InitAuth()
......................................................................
IMPALA-6281: Fix use-after-free in InitAuth()
Previously, we implicitly create a local string object created from
the char* in argv[0] when calling InitAuth(). This string object goes
out of scope once InitAuth() returns but the pointer of this local
string's buffer is passed to the Sasl library which may reference
it after the local string has been deleted, leading to use-after-free.
This change fixes the problem above by passing the char* argv[0]
directly into InitAuth() and eventually to the Sasl library which
takes a const char* as argument anyway.
Testing done: Verified rpc-mgr-test and thrift-server-test no longer
fail in ASAN build.
Change-Id: I1f29c2396df114264dfc23726b8ba778f50e12e9
---
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-test.cc
M be/src/rpc/thrift-server-test.cc
M be/src/transport/TSasl.h
5 files changed, 9 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/77/8777/1
--
To view, visit http://gerrit.cloudera.org:8080/8777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1f29c2396df114264dfc23726b8ba778f50e12e9
Gerrit-Change-Number: 8777
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-6281: Fix use-after-free in InitAuth()
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/8777 )
Change subject: IMPALA-6281: Fix use-after-free in InitAuth()
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/8777/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8777/1//COMMIT_MSG@9
PS1, Line 9: Previously, we implicitly create a local string object created from
nit: can you briefly mention why this was found now / when it was introduced. I saw that you explained it in the JIRA but it may be helpful to include it here, too.
http://gerrit.cloudera.org:8080/#/c/8777/1/be/src/rpc/rpc-mgr-test.cc
File be/src/rpc/rpc-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/8777/1/be/src/rpc/rpc-mgr-test.cc@57
PS1, Line 57: static string current_executable_path;
Should we make this a char* (and spell all uppercase)?
http://gerrit.cloudera.org:8080/#/c/8777/1/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:
http://gerrit.cloudera.org:8080/#/c/8777/1/be/src/rpc/thrift-server-test.cc@94
PS1, Line 94: static string current_executable_path;
Change to char*, upper case?
http://gerrit.cloudera.org:8080/#/c/8777/1/be/src/transport/TSasl.h
File be/src/transport/TSasl.h:
http://gerrit.cloudera.org:8080/#/c/8777/1/be/src/transport/TSasl.h@213
PS1, Line 213: life cycle
nit: lifetime
--
To view, visit http://gerrit.cloudera.org:8080/8777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f29c2396df114264dfc23726b8ba778f50e12e9
Gerrit-Change-Number: 8777
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Dec 2017 19:15:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6281: Fix use-after-free in InitAuth()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8777 )
Change subject: IMPALA-6281: Fix use-after-free in InitAuth()
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f29c2396df114264dfc23726b8ba778f50e12e9
Gerrit-Change-Number: 8777
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Dec 2017 22:19:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6281: Fix use-after-free in InitAuth()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8777 )
Change subject: IMPALA-6281: Fix use-after-free in InitAuth()
......................................................................
Patch Set 2: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/8777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f29c2396df114264dfc23726b8ba778f50e12e9
Gerrit-Change-Number: 8777
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Dec 2017 09:48:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6281: Fix use-after-free in InitAuth()
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8777 )
Change subject: IMPALA-6281: Fix use-after-free in InitAuth()
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/8777/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8777/1//COMMIT_MSG@9
PS1, Line 9: Previously, we implicitly create a local string object created from
> nit: can you briefly mention why this was found now / when it was introduce
Done
http://gerrit.cloudera.org:8080/#/c/8777/1/be/src/rpc/rpc-mgr-test.cc
File be/src/rpc/rpc-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/8777/1/be/src/rpc/rpc-mgr-test.cc@57
PS1, Line 57: static string CURRENT_EXECUTABLE_PATH;
> Should we make this a char* (and spell all uppercase)?
Changed the interface of InitAuth() back to taking const string& in PS2. Switched to upper case.
http://gerrit.cloudera.org:8080/#/c/8777/1/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:
http://gerrit.cloudera.org:8080/#/c/8777/1/be/src/rpc/thrift-server-test.cc@94
PS1, Line 94: static string CURRENT_EXECUTABLE_PATH;
> Change to char*, upper case?
Changed the interface of InitAuth() back to taking const string& in PS2. Switch to upper case.
http://gerrit.cloudera.org:8080/#/c/8777/1/be/src/transport/TSasl.h
File be/src/transport/TSasl.h:
http://gerrit.cloudera.org:8080/#/c/8777/1/be/src/transport/TSasl.h@213
PS1, Line 213: life time
> nit: lifetime
Done
--
To view, visit http://gerrit.cloudera.org:8080/8777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f29c2396df114264dfc23726b8ba778f50e12e9
Gerrit-Change-Number: 8777
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Dec 2017 22:13:05 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6281: Fix use-after-free in InitAuth()
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/8777 )
Change subject: IMPALA-6281: Fix use-after-free in InitAuth()
......................................................................
IMPALA-6281: Fix use-after-free in InitAuth()
Previously, we implicitly create a local string object created from
the char* in argv[0] when calling InitAuth(). This string object goes
out of scope once InitAuth() returns but the pointer of this local
string's buffer is passed to the Sasl library which may reference
it after the local string has been deleted, leading to use-after-free.
This bug is exposed by recent change to enable Kerberos with KRPC as
we now always initialize Sasl even if Kerberos is not enabled.
This change fixes the problem above by making a copy of 'appname'
passed to InitAuth(). Also, the new code enforces that multiple
calls to InitAuth() must use the same 'appname' or it will fail.
Testing done: Verified rpc-mgr-test and thrift-server-test no longer
fail in ASAN build.
Change-Id: I1f29c2396df114264dfc23726b8ba778f50e12e9
Reviewed-on: http://gerrit.cloudera.org:8080/8777
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Reviewed-by: Lars Volker <lv...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-test.cc
M be/src/rpc/thrift-server-test.cc
M be/src/transport/TSasl.h
M common/thrift/generate_error_codes.py
6 files changed, 31 insertions(+), 15 deletions(-)
Approvals:
Tim Armstrong: Looks good to me, approved
Lars Volker: Looks good to me, but someone else must approve
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/8777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I1f29c2396df114264dfc23726b8ba778f50e12e9
Gerrit-Change-Number: 8777
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6281: Fix use-after-free in InitAuth()
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/8777 )
Change subject: IMPALA-6281: Fix use-after-free in InitAuth()
......................................................................
IMPALA-6281: Fix use-after-free in InitAuth()
Previously, we implicitly create a local string object created from
the char* in argv[0] when calling InitAuth(). This string object goes
out of scope once InitAuth() returns but the pointer of this local
string's buffer is passed to the Sasl library which may reference
it after the local string has been deleted, leading to use-after-free.
This bug is exposed by recent change to enable Kerberos with KRPC as
we now always initialize Sasl even if Kerberos is not enabled.
This change fixes the problem above by making a copy of 'appname'
passed to InitAuth(). Also, the new code enforces that multiple
calls to InitAuth() must use the same 'appname' or it will fail.
Testing done: Verified rpc-mgr-test and thrift-server-test no longer
fail in ASAN build.
Change-Id: I1f29c2396df114264dfc23726b8ba778f50e12e9
---
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-test.cc
M be/src/rpc/thrift-server-test.cc
M be/src/transport/TSasl.h
M common/thrift/generate_error_codes.py
6 files changed, 31 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/77/8777/2
--
To view, visit http://gerrit.cloudera.org:8080/8777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1f29c2396df114264dfc23726b8ba778f50e12e9
Gerrit-Change-Number: 8777
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-6281: Fix use-after-free in InitAuth()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8777 )
Change subject: IMPALA-6281: Fix use-after-free in InitAuth()
......................................................................
Patch Set 2:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1582/
--
To view, visit http://gerrit.cloudera.org:8080/8777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f29c2396df114264dfc23726b8ba778f50e12e9
Gerrit-Change-Number: 8777
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Dec 2017 00:18:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6281: Fix use-after-free in InitAuth()
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/8777 )
Change subject: IMPALA-6281: Fix use-after-free in InitAuth()
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/8777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f29c2396df114264dfc23726b8ba778f50e12e9
Gerrit-Change-Number: 8777
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Dec 2017 22:41:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6281: Fix use-after-free in InitAuth()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8777 )
Change subject: IMPALA-6281: Fix use-after-free in InitAuth()
......................................................................
Patch Set 2:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1586/
--
To view, visit http://gerrit.cloudera.org:8080/8777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f29c2396df114264dfc23726b8ba778f50e12e9
Gerrit-Change-Number: 8777
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Dec 2017 06:05:04 +0000
Gerrit-HasComments: No