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 <a...@cloudera.com>
Reviewed-by: Mike Percy <mpe...@apache.org>


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 <aser...@cloudera.com>
Authored: Fri Mar 9 17:54:05 2018 -0800
Committer: Alexey Serbin <aser...@cloudera.com>
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);
 

Reply via email to