Commit d22d402e authored by Xiao Shi's avatar Xiao Shi Committed by Facebook Github Bot

fix erase use-after-free in F14Vector containers

Summary:
Upon `erase`, F14 vector containers destroys the value and erases the item
(i.e., index into the `values_` vector) from the underlying hashtable. However,
the item still needs to be hashable when erasing from the hashtable, so we have
to destroy the value _afterwards_.

This diff fixes the bug.

There are a couple of reasons that this was previous undetected / did not cause
a problem:
  * for POD types, `allocator_traits::destroy` is a no-op.
  * this code path is only hit if the chunk of the destroyed item has hosted
    overflowed items
  * the use was immediately after free
  * our test coverage did not have vector policy + non-SSO string keys

Reviewed By: nbronson

Differential Revision: D7488050

fbshipit-source-id: ea29e875a0c7a39b8deed40a15777a6983438836
parent 3cec1c6b
...@@ -965,10 +965,12 @@ class F14VectorMap ...@@ -965,10 +965,12 @@ class F14VectorMap
Alloc& a = this->table_.alloc(); Alloc& a = this->table_.alloc();
auto values = this->table_.values_; auto values = this->table_.values_;
// destroy the value and remove the ptr from the base table // Remove the ptr from the base table and destroy the value.
auto index = underlying.item(); auto index = underlying.item();
std::allocator_traits<Alloc>::destroy(a, std::addressof(values[index])); // The item still needs to be hashable during this call, so we must destroy
// the value _afterwards_.
this->table_.erase(underlying); this->table_.erase(underlying);
std::allocator_traits<Alloc>::destroy(a, std::addressof(values[index]));
// move the last element in values_ down and fix up the inbound index // move the last element in values_ down and fix up the inbound index
auto tailIndex = this->size(); auto tailIndex = this->size();
......
...@@ -520,6 +520,32 @@ TEST(F14ValueMap, steady_state_stats) { ...@@ -520,6 +520,32 @@ TEST(F14ValueMap, steady_state_stats) {
F14TableStats::compute(h); F14TableStats::compute(h);
} }
TEST(F14VectorMap, steady_state_stats) {
// 10k keys, 14% probability of insert, 90% chance of erase, so the
// table should converge to 1400 size without triggering the rehash
// that would occur at 1536.
F14VectorMap<std::string, uint64_t> h;
std::mt19937_64 gen(0);
std::uniform_int_distribution<> dist(0, 10000);
for (std::size_t i = 0; i < 100000; ++i) {
auto key = "0123456789ABCDEFGHIJKLMNOPQ" + std::to_string(dist(gen));
if (dist(gen) < 1400) {
h.insert_or_assign(key, i);
} else {
h.erase(key);
}
if (((i + 1) % 10000) == 0) {
auto stats = F14TableStats::compute(h);
// Verify that average miss probe length is bounded despite continued
// erase + reuse. p99 of the average across 10M random steps is 4.69,
// average is 2.96.
EXPECT_LT(f14::expectedProbe(stats.missProbeLengthHisto), 10.0);
}
}
// F14ValueMap at steady state
F14TableStats::compute(h);
}
TEST(Tracked, baseline) { TEST(Tracked, baseline) {
Tracked<0> a0; Tracked<0> a0;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment