Make changing a record's type an atomic operation.
Accessing the type_id is generally thread-safe within the PMA
code because it is only accessed once the memory is owned to
the exclusion of any other allocator. The same can't be said,
however, once access to it is given to a caller and so
protection needs to be added so that callers can't introduce
race conditions due to their inter-process thread-unsafety.
BUG=546019
Review-Url: https://codereview.chromium.org/2034813003
Cr-Commit-Position: refs/heads/master@{#397711}
CrOS-Libchrome-Original-Commit: 84a8e0963774266d197d9bf90c952d983ab74869
diff --git a/base/metrics/persistent_histogram_allocator.cc b/base/metrics/persistent_histogram_allocator.cc
index ce6d23a..7e41943 100644
--- a/base/metrics/persistent_histogram_allocator.cc
+++ b/base/metrics/persistent_histogram_allocator.cc
@@ -481,7 +481,7 @@
// two to be created. The allocator does not support releasing the
// acquired memory so just change the type to be empty.
else
- memory_allocator_->SetType(ref, 0);
+ memory_allocator_->ChangeType(ref, 0, kTypeIdHistogram);
}
void PersistentHistogramAllocator::MergeHistogramToStatisticsRecorder(
diff --git a/base/metrics/persistent_memory_allocator.cc b/base/metrics/persistent_memory_allocator.cc
index 6770c3d..dfa408f 100644
--- a/base/metrics/persistent_memory_allocator.cc
+++ b/base/metrics/persistent_memory_allocator.cc
@@ -84,8 +84,8 @@
struct PersistentMemoryAllocator::BlockHeader {
uint32_t size; // Number of bytes in this block, including header.
uint32_t cookie; // Constant value indicating completed allocation.
- uint32_t type_id; // A number provided by caller indicating data type.
- std::atomic<uint32_t> next; // Pointer to the next block when iterating.
+ std::atomic<uint32_t> type_id; // Arbitrary number indicating data type.
+ std::atomic<uint32_t> next; // Pointer to the next block when iterating.
};
// The shared metadata exists once at the top of the memory segment to
@@ -194,7 +194,7 @@
// "strong" compare-exchange is used because failing unnecessarily would
// mean repeating some fairly costly validations above.
if (last_record_.compare_exchange_strong(last, next)) {
- *type_return = block->type_id;
+ *type_return = block->type_id.load(std::memory_order_relaxed);
break;
}
}
@@ -301,7 +301,7 @@
shared_meta()->queue.next.load(std::memory_order_relaxed) != 0 ||
first_block->size != 0 ||
first_block->cookie != 0 ||
- first_block->type_id != 0 ||
+ first_block->type_id.load(std::memory_order_relaxed) != 0 ||
first_block->next != 0) {
// ...or something malicious has been playing with the metadata.
SetCorrupt();
@@ -428,15 +428,20 @@
const volatile BlockHeader* const block = GetBlock(ref, 0, 0, false, false);
if (!block)
return 0;
- return block->type_id;
+ return block->type_id.load(std::memory_order_relaxed);
}
-void PersistentMemoryAllocator::SetType(Reference ref, uint32_t type_id) {
+bool PersistentMemoryAllocator::ChangeType(Reference ref,
+ uint32_t to_type_id,
+ uint32_t from_type_id) {
DCHECK(!readonly_);
volatile BlockHeader* const block = GetBlock(ref, 0, 0, false, false);
if (!block)
- return;
- block->type_id = type_id;
+ return false;
+
+ // This is a "strong" exchange because there is no loop that can retry in
+ // the wake of spurious failures possible with "weak" exchanges.
+ return block->type_id.compare_exchange_strong(from_type_id, to_type_id);
}
PersistentMemoryAllocator::Reference PersistentMemoryAllocator::Allocate(
@@ -550,7 +555,7 @@
// writing beyond the allocated space and into unallocated space.
if (block->size != 0 ||
block->cookie != kBlockCookieFree ||
- block->type_id != 0 ||
+ block->type_id.load(std::memory_order_relaxed) != 0 ||
block->next.load(std::memory_order_relaxed) != 0) {
SetCorrupt();
return kReferenceNull;
@@ -558,7 +563,7 @@
block->size = size;
block->cookie = kBlockCookieAllocated;
- block->type_id = type_id;
+ block->type_id.store(type_id, std::memory_order_relaxed);
return freeptr;
}
}
@@ -690,8 +695,10 @@
return nullptr;
if (ref != kReferenceQueue && block->cookie != kBlockCookieAllocated)
return nullptr;
- if (type_id != 0 && block->type_id != type_id)
+ if (type_id != 0 &&
+ block->type_id.load(std::memory_order_relaxed) != type_id) {
return nullptr;
+ }
}
// Return pointer to block data.
diff --git a/base/metrics/persistent_memory_allocator.h b/base/metrics/persistent_memory_allocator.h
index bd35c3c..2fc0d2d 100644
--- a/base/metrics/persistent_memory_allocator.h
+++ b/base/metrics/persistent_memory_allocator.h
@@ -241,9 +241,11 @@
// Access the internal "type" of an object. This generally isn't necessary
// but can be used to "clear" the type and so effectively mark it as deleted
- // even though the memory stays valid and allocated.
+ // even though the memory stays valid and allocated. Changing the type is
+ // an atomic compare/exchange and so requires knowing the existing value.
+ // It will return false if the existing type is not what is expected.
uint32_t GetType(Reference ref) const;
- void SetType(Reference ref, uint32_t type_id);
+ bool ChangeType(Reference ref, uint32_t to_type_id, uint32_t from_type_id);
// Reserve space in the memory segment of the desired |size| and |type_id|.
// A return value of zero indicates the allocation failed, otherwise the
diff --git a/base/metrics/persistent_memory_allocator_unittest.cc b/base/metrics/persistent_memory_allocator_unittest.cc
index 7307aea..b8f3707 100644
--- a/base/metrics/persistent_memory_allocator_unittest.cc
+++ b/base/metrics/persistent_memory_allocator_unittest.cc
@@ -181,9 +181,9 @@
// Check that an objcet's type can be changed.
EXPECT_EQ(2U, allocator_->GetType(block2));
- allocator_->SetType(block2, 3);
+ allocator_->ChangeType(block2, 3, 2);
EXPECT_EQ(3U, allocator_->GetType(block2));
- allocator_->SetType(block2, 2);
+ allocator_->ChangeType(block2, 2, 3);
EXPECT_EQ(2U, allocator_->GetType(block2));
// Create second allocator (read/write) using the same memory segment.
@@ -511,7 +511,7 @@
r456 = local.Allocate(456, 456);
r789 = local.Allocate(789, 789);
local.MakeIterable(r123);
- local.SetType(r456, 654);
+ local.ChangeType(r456, 654, 456);
local.MakeIterable(r789);
local.GetMemoryInfo(&meminfo1);
EXPECT_FALSE(local.IsFull());
@@ -599,7 +599,7 @@
r456 = local.Allocate(456, 456);
r789 = local.Allocate(789, 789);
local.MakeIterable(r123);
- local.SetType(r456, 654);
+ local.ChangeType(r456, 654, 456);
local.MakeIterable(r789);
local.GetMemoryInfo(&meminfo1);
EXPECT_FALSE(local.IsFull());