KUDU-2295 fix nullptr dereference in Tablet
Prior to this patch, in the case of concurrent events of shutting
down a tablet and running a snapshot scan on it, sometimes the code
ended up trying to access already freed members of a Tablet object,
resulting in stack traces like below.
The traces were captured from modified raft_consensus_stress-itest
(added a single read thread for TestWorkload) runs using dist_test.
(after RYW changes):
PC: @ 0x7f563b5e31cb std::atomic_bool::load()
*** SIGSEGV (@0x1f8) received by PID 19893 (TID 0x7f561ce82700) from PID 504;
stack trace: ***
@ 0x7f5638713330 (unknown) at ??:0
@ 0x7f563b5e31cb std::atomic_bool::load() at ??:0
@ 0x7f563b609c31 kudu::tablet::MvccManager::is_open() at ??:0
@ 0x7f563b6085f3 kudu::tablet::MvccManager::CheckOpen() at ??:0
@ 0x7f563b607fc5 kudu::tablet::MvccManager::WaitUntil() at ??:0
@ 0x7f563b608938
kudu::tablet::MvccManager::WaitForSnapshotWithAllCommitted() at ??:0
@ 0x7f563ca61b55
kudu::tserver::TabletServiceImpl::HandleScanAtSnapshot() at ??:0
@ 0x7f563ca5c0e2
kudu::tserver::TabletServiceImpl::HandleNewScanRequest() at ??:0
@ 0x7f563ca59793 kudu::tserver::TabletServiceImpl::Scan() at ??:0
@ 0x7f5637324e4d
kudu::tserver::TabletServerServiceIf::TabletServerServiceIf()::$_5::operator()()
at ??:0
@ 0x7f5637324c92 std::_Function_handler<>::_M_invoke() at ??:0
@ 0x7f563648992b std::function<>::operator()() at ??:0
@ 0x7f56364891ed kudu::rpc::GeneratedServiceIf::Handle() at ??:0
@ 0x7f563648b5e6 kudu::rpc::ServicePool::RunThread() at ??:0
@ 0x7f563648dc29 boost::_mfi::mf0<>::operator()() at ??:0
@ 0x7f563648db90 boost::_bi::list1<>::operator()<>() at ??:0
@ 0x7f563648db3a boost::_bi::bind_t<>::operator()() at ??:0
@ 0x7f563648d91d
boost::detail::function::void_function_obj_invoker0<>::invoke() at ??:0
@ 0x7f5636430078 boost::function0<>::operator()() at ??:0
@ 0x7f563472c08d kudu::Thread::SuperviseThread() at ??:0
@ 0x7f563870b184 start_thread at ??:0
@ 0x7f5630a2affd clone at ??:0
(before RYW changes):
PC: @ 0x7f1e02025790 scoped_refptr<>::operator->()
*** SIGSEGV (@0x160) received by PID 8782 (TID 0x7f1de3c7e700) from PID 352;
stack trace: ***
@ 0x7f1dfdcfc330 (unknown) at ??:0
@ 0x7f1e02025790 scoped_refptr<>::operator->() at ??:0
@ 0x7f1e00ae62e7 kudu::tablet::Tablet::GetTabletAncientHistoryMark() at
??:0
@ 0x7f1e00ae627d kudu::tablet::Tablet::GetHistoryGcOpts() at ??:0
@ 0x7f1e02012c53 kudu::tserver::(anonymous
namespace)::VerifyNotAncientHistory() at ??:0
@ 0x7f1e0201223b
kudu::tserver::TabletServiceImpl::HandleScanAtSnapshot() at ??:0
@ 0x7f1e0200c6dd
kudu::tserver::TabletServiceImpl::HandleNewScanRequest() at ??:0
@ 0x7f1e02009d33 kudu::tserver::TabletServiceImpl::Scan() at ??:0
@ 0x7f1dfc90de4d
kudu::tserver::TabletServerServiceIf::TabletServerServiceIf()::$_5::operator()()
at ??:0
@ 0x7f1dfc90dc92 std::_Function_handler<>::_M_invoke() at ??:0
@ 0x7f1dfba728ab std::function<>::operator()() at ??:0
@ 0x7f1dfba7216d kudu::rpc::GeneratedServiceIf::Handle() at ??:0
@ 0x7f1dfba74526 kudu::rpc::ServicePool::RunThread() at ??:0
@ 0x7f1dfba76ad9 boost::_mfi::mf0<>::operator()() at ??:0
@ 0x7f1dfba76a40 boost::_bi::list1<>::operator()<>() at ??:0
@ 0x7f1dfba769ea boost::_bi::bind_t<>::operator()() at ??:0
@ 0x7f1dfba767cd
boost::detail::function::void_function_obj_invoker0<>::invoke() at ??:0
@ 0x7f1dfba190f8 boost::function0<>::operator()() at ??:0
@ 0x7f1df9d1788d kudu::Thread::SuperviseThread() at ??:0
@ 0x7f1dfdcf4184 start_thread at ??:0
@ 0x7f1df6023ffd clone at ??:0
@ 0x0 (unknown)
Change-Id: Ib4b8cc2a307e5a0bd6019bd155f33c1565fca513
Reviewed-on: http://gerrit.cloudera.org:8080/9574
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <[email protected]>
Reviewed-by: Mike Percy <[email protected]>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/fa2b4954
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/fa2b4954
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/fa2b4954
Branch: refs/heads/master
Commit: fa2b495487e520f911a0ddbd8b2bf52bfe01e28e
Parents: 038c3eb
Author: Alexey Serbin <[email protected]>
Authored: Fri Mar 9 17:54:05 2018 -0800
Committer: Alexey Serbin <[email protected]>
Committed: Tue Mar 13 00:01:01 2018 +0000
----------------------------------------------------------------------
src/kudu/tserver/tablet_service.cc | 25 ++++++++++++-------------
src/kudu/tserver/tablet_service.h | 4 +++-
2 files changed, 15 insertions(+), 14 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/fa2b4954/src/kudu/tserver/tablet_service.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/tablet_service.cc
b/src/kudu/tserver/tablet_service.cc
index 4d79d29..f43e254 100644
--- a/src/kudu/tserver/tablet_service.cc
+++ b/src/kudu/tserver/tablet_service.cc
@@ -149,13 +149,12 @@ using kudu::consensus::GetNodeInstanceResponsePB;
using kudu::consensus::LeaderStepDownRequestPB;
using kudu::consensus::LeaderStepDownResponsePB;
using kudu::consensus::OpId;
-using kudu::consensus::UnsafeChangeConfigRequestPB;
-using kudu::consensus::UnsafeChangeConfigResponsePB;
using kudu::consensus::RaftConsensus;
using kudu::consensus::RunLeaderElectionRequestPB;
using kudu::consensus::RunLeaderElectionResponsePB;
using kudu::consensus::StartTabletCopyRequestPB;
using kudu::consensus::StartTabletCopyResponsePB;
+using kudu::consensus::TimeManager;
using kudu::consensus::UnsafeChangeConfigRequestPB;
using kudu::consensus::UnsafeChangeConfigResponsePB;
using kudu::consensus::VoteRequestPB;
@@ -1738,6 +1737,8 @@ Status
TabletServiceImpl::HandleNewScanRequest(TabletReplica* replica,
// Preset the error code for when creating the iterator on the tablet fails
TabletServerErrorPB::Code tmp_error_code =
TabletServerErrorPB::MISMATCHED_SCHEMA;
+ // It's important to keep the reference to the tablet for the case when the
+ // tablet replica's shutdown is run concurrently with the code below.
shared_ptr<Tablet> tablet;
RETURN_NOT_OK(GetTabletRef(replica, &tablet, error_code));
{
@@ -1756,21 +1757,21 @@ Status
TabletServiceImpl::HandleNewScanRequest(TabletReplica* replica,
}
case READ_YOUR_WRITES: // Fallthrough intended
case READ_AT_SNAPSHOT: {
- s = HandleScanAtSnapshot(scan_pb, rpc_context, projection, replica,
- &iter, snap_timestamp);
+ scoped_refptr<consensus::TimeManager> time_manager =
replica->time_manager();
+ s = HandleScanAtSnapshot(scan_pb, rpc_context, projection,
tablet.get(),
+ time_manager.get(), &iter, snap_timestamp);
// If we got a Status::ServiceUnavailable() from
HandleScanAtSnapshot() it might
// mean we're just behind so let the client try again.
if (s.IsServiceUnavailable()) {
*error_code = TabletServerErrorPB::THROTTLED;
return s;
}
-
if (!s.ok()) {
tmp_error_code = TabletServerErrorPB::INVALID_SNAPSHOT;
}
break;
}
- TRACE("Iterator created");
+ TRACE("Iterator created");
}
}
@@ -2035,7 +2036,8 @@ MonoTime ClampScanDeadlineForWait(const MonoTime&
deadline, bool* was_clamped) {
Status TabletServiceImpl::HandleScanAtSnapshot(const NewScanRequestPB& scan_pb,
const RpcContext* rpc_context,
const Schema& projection,
- TabletReplica* tablet_replica,
+ Tablet* tablet,
+ consensus::TimeManager*
time_manager,
gscoped_ptr<RowwiseIterator>*
iter,
Timestamp* snap_timestamp) {
switch (scan_pb.read_mode()) {
@@ -2048,12 +2050,7 @@ Status TabletServiceImpl::HandleScanAtSnapshot(const
NewScanRequestPB& scan_pb,
// Based on the read mode, pick a timestamp and verify it.
Timestamp tmp_snap_timestamp;
- RETURN_NOT_OK(PickAndVerifyTimestamp(scan_pb, tablet_replica->tablet(),
&tmp_snap_timestamp));
-
- tablet::MvccSnapshot snap;
- Tablet* tablet = tablet_replica->tablet();
- scoped_refptr<consensus::TimeManager> time_manager =
tablet_replica->time_manager();
- tablet::MvccManager* mvcc_manager = tablet->mvcc_manager();
+ RETURN_NOT_OK(PickAndVerifyTimestamp(scan_pb, tablet, &tmp_snap_timestamp));
// Reduce the client's deadline by a few msecs to allow for overhead.
MonoTime client_deadline = rpc_context->GetClientDeadline() -
MonoDelta::FromMilliseconds(10);
@@ -2075,6 +2072,8 @@ Status TabletServiceImpl::HandleScanAtSnapshot(const
NewScanRequestPB& scan_pb,
MonoTime before = MonoTime::Now();
Status s = time_manager->WaitUntilSafe(tmp_snap_timestamp, final_deadline);
+ tablet::MvccSnapshot snap;
+ tablet::MvccManager* mvcc_manager = tablet->mvcc_manager();
if (s.ok()) {
// Wait for the in-flights in the snapshot to be finished.
TRACE("Waiting for operations to commit");
http://git-wip-us.apache.org/repos/asf/kudu/blob/fa2b4954/src/kudu/tserver/tablet_service.h
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/tablet_service.h
b/src/kudu/tserver/tablet_service.h
index 37e6bf2..7f44768 100644
--- a/src/kudu/tserver/tablet_service.h
+++ b/src/kudu/tserver/tablet_service.h
@@ -62,6 +62,7 @@ class RunLeaderElectionRequestPB;
class RunLeaderElectionResponsePB;
class StartTabletCopyRequestPB;
class StartTabletCopyResponsePB;
+class TimeManager;
class UnsafeChangeConfigRequestPB;
class UnsafeChangeConfigResponsePB;
class VoteRequestPB;
@@ -148,7 +149,8 @@ class TabletServiceImpl : public TabletServerServiceIf {
Status HandleScanAtSnapshot(const NewScanRequestPB& scan_pb,
const rpc::RpcContext* rpc_context,
const Schema& projection,
- tablet::TabletReplica* tablet_replica,
+ tablet::Tablet* tablet,
+ consensus::TimeManager* time_manager,
gscoped_ptr<RowwiseIterator>* iter,
Timestamp* snap_timestamp);
