Skip to content

Commit 51b3a7f

Browse files
yfeldblummeta-codesync[bot]
authored andcommitted
fix integer overflow in IOBuf::cloneCoalescedAsValueWithHeadroomTailroom
Summary: Follow-up to D116335797, which fixed the integer-overflow → heap-buffer-overflow in `IOBuf::coalesceAndReallocate`. The sibling path `cloneCoalescedAsValueWithHeadroomTailroom` re-implements the same unchecked three-term sum `newLength + newHeadroom + newTailroom`, which wraps when attacker-controlled `headroom`/`tailroom` are forwarded through the public `cloneCoalesced*WithHeadroomTailroom` APIs, under-allocating the buffer and overflowing it while copying. This guards the "Coalesce into newBuf" capacity with `checked_add` + `kMaxIOBufSize`, throwing `std::bad_alloc` on overflow — mirroring the existing idiom in `coalesceAndReallocate`. The unique_ptr wrapper forwards to the by-value overload, so both public entry points are covered. This re-applies the `clone` hunk from the reverted D115286348 while leaving the already-landed `coalesceAndReallocate` guard untouched. Reviewed By: ilvokhin Differential Revision: D116669664 fbshipit-source-id: 14f433f1793ad102ef97e66a7ac52d62ca5bb5a3
1 parent 25062c6 commit 51b3a7f

2 files changed

Lines changed: 21 additions & 1 deletion

File tree

folly/io/IOBuf.cpp

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -860,7 +860,11 @@ IOBuf IOBuf::cloneCoalescedAsValueWithHeadroomTailroom(
860860

861861
// Coalesce into newBuf
862862
const std::size_t newLength = computeChainDataLength();
863-
const std::size_t newCapacity = newLength + newHeadroom + newTailroom;
863+
std::size_t newCapacity = 0;
864+
if (!checked_add(&newCapacity, newLength, newHeadroom, newTailroom) ||
865+
newCapacity > kMaxIOBufSize) {
866+
throw_exception<std::bad_alloc>();
867+
}
864868
IOBuf newBuf{CREATE, newCapacity};
865869
newBuf.advance(newHeadroom);
866870

folly/io/test/IOBufTest.cpp

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1589,6 +1589,22 @@ TEST(IOBuf, CoalesceCapacityOverflow) {
15891589
EXPECT_THROW(head->coalesceWithHeadroomTailroom(kMax, kMax), std::bad_alloc);
15901590
}
15911591

1592+
// cloneCoalescedAsValueWithHeadroomTailroom (and the unique_ptr wrapper that
1593+
// forwards to it) must throw std::bad_alloc when the coalesced capacity
1594+
// overflows size_t rather than wrapping around to an undersized buffer.
1595+
TEST(IOBuf, CloneCoalescedCapacityOverflow) {
1596+
auto head = fromStr("hello");
1597+
head->insertAfterThisOne(fromStr("world"));
1598+
ASSERT_TRUE(head->isChained());
1599+
1600+
constexpr auto kMax = std::numeric_limits<std::size_t>::max();
1601+
EXPECT_THROW(
1602+
head->cloneCoalescedWithHeadroomTailroom(kMax, kMax), std::bad_alloc);
1603+
EXPECT_THROW(
1604+
head->cloneCoalescedAsValueWithHeadroomTailroom(kMax, kMax),
1605+
std::bad_alloc);
1606+
}
1607+
15921608
TEST(IOBuf, CloneCoalescedChain) {
15931609
auto b = IOBuf::createChain(1000, 100);
15941610
b->advance(10);

0 commit comments

Comments
 (0)