You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by to...@apache.org on 2016/02/18 03:15:59 UTC
[3/4] incubator-kudu git commit: catalog_manager: fix a
use-after-free on TabletInfo
catalog_manager: fix a use-after-free on TabletInfo
This fixes an issue seen in a new stress test that Adar is working on:
T1:
- HandleAssignCreatingTablet calls CreateTabletInfo, which returns a raw
pointer 'replacement'
- we added that raw pointer to the table's tablet map (which also stores it as raw)
T2:
- GetTableLocations() constructs a scoped_refptr to the TabletInfo object,
acting as its first referer.
- GetTableLocations() finishes and drops the scoped_refptr, deleting the TabletInfo
T1:
- we try to dereference 'replacement', and crash, because T2 freed the pointer.
This patch fixes the issue by using a scoped_refptr to hold the 'replacement'
tablet before exposing it to other threads.
No new unit test here, because it was easily reproduced by the stress test that
Adar's working on, which will be committed soon.
Change-Id: I67259d54f03bded7c7bbbea677093f39e0efa528
Reviewed-on: http://gerrit.cloudera.org:8080/2213
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/incubator-kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-kudu/commit/9ca41277
Tree: http://git-wip-us.apache.org/repos/asf/incubator-kudu/tree/9ca41277
Diff: http://git-wip-us.apache.org/repos/asf/incubator-kudu/diff/9ca41277
Branch: refs/heads/master
Commit: 9ca41277167240084f0731a97255ccfce0bf3e80
Parents: e284a93
Author: Todd Lipcon <to...@apache.org>
Authored: Wed Feb 17 16:14:00 2016 -0800
Committer: Todd Lipcon <to...@apache.org>
Committed: Thu Feb 18 02:14:51 2016 +0000
----------------------------------------------------------------------
src/kudu/master/catalog_manager.cc | 28 +++++++++++++++-------------
src/kudu/master/catalog_manager.h | 4 ++--
2 files changed, 17 insertions(+), 15 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/9ca41277/src/kudu/master/catalog_manager.cc
----------------------------------------------------------------------
diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc
index b3c0468..e5958c2 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -785,15 +785,17 @@ Status CatalogManager::CreateTable(const CreateTableRequestPB* orig_req,
for (const Partition& partition : partitions) {
PartitionPB partition_pb;
partition.ToPB(&partition_pb);
- tablets.push_back(CreateTabletInfo(table.get(), partition_pb));
+ scoped_refptr<TabletInfo> t = CreateTabletInfo(table.get(), partition_pb);
+ tablets.push_back(t.get());
+
+ // Add the new tablet to the catalog-manager-wide map by tablet ID.
+ InsertOrDie(&tablet_map_, t->tablet_id(), std::move(t));
}
- // Add the table/tablets to the in-memory map for the assignment.
resp->set_table_id(table->id());
+
+ // Add the tablets to the table's map.
table->AddTablets(tablets);
- for (TabletInfo* tablet : tablets) {
- InsertOrDie(&tablet_map_, tablet->tablet_id(), tablet);
- }
}
TRACE("Inserted new table and tablet info into CatalogManager maps");
@@ -892,9 +894,9 @@ TableInfo *CatalogManager::CreateTableInfo(const CreateTableRequestPB& req,
return table;
}
-TabletInfo* CatalogManager::CreateTabletInfo(TableInfo* table,
- const PartitionPB& partition) {
- TabletInfo* tablet = new TabletInfo(table, GenerateId());
+scoped_refptr<TabletInfo> CatalogManager::CreateTabletInfo(TableInfo* table,
+ const PartitionPB& partition) {
+ scoped_refptr<TabletInfo> tablet(new TabletInfo(table, GenerateId()));
tablet->mutable_metadata()->StartMutation();
SysTabletsEntryPB *metadata = &tablet->mutable_metadata()->mutable_dirty()->pb;
metadata->set_state(SysTabletsEntryPB::PREPARING);
@@ -2556,13 +2558,13 @@ void CatalogManager::HandleAssignCreatingTablet(TabletInfo* tablet,
// The "tablet creation" was already sent, but we didn't receive an answer
// within the timeout. So the tablet will be replaced by a new one.
- TabletInfo *replacement = CreateTabletInfo(tablet->table().get(),
+ scoped_refptr<TabletInfo> replacement = CreateTabletInfo(tablet->table().get(),
old_info.pb.partition());
LOG(WARNING) << "Tablet " << tablet->ToString() << " was not created within "
<< "the allowed timeout. Replacing with a new tablet "
<< replacement->tablet_id();
- tablet->table()->AddTablet(replacement);
+ tablet->table()->AddTablet(replacement.get());
{
boost::lock_guard<LockType> l_maps(lock_);
tablet_map_[replacement->tablet_id()] = replacement;
@@ -2580,13 +2582,13 @@ void CatalogManager::HandleAssignCreatingTablet(TabletInfo* tablet,
Substitute("Replacement for $0", tablet->tablet_id()));
deferred->tablets_to_update.push_back(tablet);
- deferred->tablets_to_add.push_back(replacement);
- deferred->needs_create_rpc.push_back(replacement);
+ deferred->tablets_to_add.push_back(replacement.get());
+ deferred->needs_create_rpc.push_back(replacement.get());
VLOG(1) << "Replaced tablet " << tablet->tablet_id()
<< " with " << replacement->tablet_id()
<< " (Table " << tablet->table()->ToString() << ")";
- new_tablets->push_back(replacement);
+ new_tablets->emplace_back(std::move(replacement));
}
// TODO: we could batch the IO onto a background thread.
http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/9ca41277/src/kudu/master/catalog_manager.h
----------------------------------------------------------------------
diff --git a/src/kudu/master/catalog_manager.h b/src/kudu/master/catalog_manager.h
index b06cb5a..49e39f8 100644
--- a/src/kudu/master/catalog_manager.h
+++ b/src/kudu/master/catalog_manager.h
@@ -473,8 +473,8 @@ class CatalogManager : public tserver::TabletPeerLookupIf {
// Helper for creating the initial TabletInfo state.
// Leaves the tablet "write locked" with the new info in the
// "dirty" state field.
- TabletInfo *CreateTabletInfo(TableInfo* table,
- const PartitionPB& partition);
+ scoped_refptr<TabletInfo> CreateTabletInfo(TableInfo* table,
+ const PartitionPB& partition);
// Builds the TabletLocationsPB for a tablet based on the provided TabletInfo.
// Populates locs_pb and returns true on success.