Skip to content

Commit 55626f7

Browse files
Lenar Fatikhovmeta-codesync[bot]
authored andcommitted
Fix use-after-free in moveForSlabRelease chained item path
Summary: Investigation doc: https://fburl.com/gdoc/oxsftkii Fix a use-after-free race condition in `CacheAllocator::moveForSlabRelease()` for chained items. After `parentItem->unmarkMoving()`, another thread can evict and free the parent item's memory. The subsequent calls to `findInternal(parentKey)` and `wakeUpWaiters(parentItem->getKey(), ...)` would then read freed memory through the dangling `Key` (which is a `folly::StringPiece` — a non-owning view into the item's data buffer). If the freed memory is reused with different content, `wakeUpWaiters` fails to find the MoveCtx entry in the MoveMap (hash/equality mismatch on garbage bytes), leaving the waiter's Baton permanently unposted. This causes threads blocked on `ItemWaitContext::wait()` to deadlock indefinitely. The fix copies the parent key to an owning `std::string` before calling `unmarkMoving()`, ensuring all subsequent key uses reference valid memory. Also fixes a typo: "Parnet" → "Parent" in a comment. Reviewed By: stuclar, AlnisM Differential Revision: D98565055 fbshipit-source-id: ea88bd4fbeaa4fc7cd992eca06af549b99633d30
1 parent b956d38 commit 55626f7

1 file changed

Lines changed: 10 additions & 5 deletions

File tree

cachelib/allocator/CacheAllocator.h

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5216,26 +5216,31 @@ bool CacheAllocator<CacheTrait>::moveForSlabRelease(Item& oldItem) {
52165216
const auto allocInfo = allocator_->getAllocInfo(oldItem.getMemory());
52175217
if (chainedItem) {
52185218
newItemHdl.reset();
5219-
auto parentKey = parentItem->getKey();
5219+
// Copy the parent key before unmarkMoving because once we unmark,
5220+
// another thread is free to remove/evict and free the parent item,
5221+
// which would make parentItem->getKey() a dangling StringPiece.
5222+
std::string parentKey(parentItem->getKey().data(),
5223+
parentItem->getKey().size());
5224+
const auto parentKeyView = Key{folly::StringPiece{parentKey}};
52205225
parentItem->unmarkMoving();
52215226
// We do another lookup here because once we unmark moving, another thread
52225227
// is free to remove/evict the parent item. So its unsafe to increment
52235228
// refcount on the parent item's memory. Instead we rely on a proper lookup.
5224-
auto parentHdl = findInternal(parentKey);
5229+
auto parentHdl = findInternal(parentKeyView);
52255230
if (!parentHdl) {
52265231
// Parent is gone, so we wake up waiting threads with a null handle.
5227-
wakeUpWaiters(parentItem->getKey(), {});
5232+
wakeUpWaiters(parentKeyView, {});
52285233
} else {
52295234
if (!parentHdl.isReady()) {
5230-
// Parnet handle isn't ready. This can be due to the parent got evicted
5235+
// Parent handle isn't ready. This can be due to the parent got evicted
52315236
// into NvmCache, or another thread is moving the slab that the parent
52325237
// handle is on (e.g. the parent got replaced and the new parent's slab
52335238
// is being moved). In this case, we must wait synchronously and block
52345239
// the current slab moving thread until parent is ready. This is
52355240
// expected to be very rare.
52365241
parentHdl.wait();
52375242
}
5238-
wakeUpWaiters(parentItem->getKey(), std::move(parentHdl));
5243+
wakeUpWaiters(parentKeyView, std::move(parentHdl));
52395244
}
52405245
} else {
52415246
auto ref = unmarkMovingAndWakeUpWaiters(oldItem, std::move(newItemHdl));

0 commit comments

Comments
 (0)