Commit b0c9ca0f authored by Marcus Holland-Moritz's avatar Marcus Holland-Moritz Committed by Facebook Github Bot

Protect memcpy calls against undefined behaviour

Summary:
While running a UBSan enabled binary, I got:

  folly/io/IOBuf.cpp:671:15: runtime error: null pointer passed as argument 2, which is declared to never be null

This change protects all calls to memcpy from passing `nullptr`.

Reviewed By: pixelb

Differential Revision: D4415355

fbshipit-source-id: a27ba74244abcca8cd4e106967222890a67f5b6d
parent 73199d4c
...@@ -210,8 +210,11 @@ IOBuf::IOBuf(CopyBufferOp /* op */, ...@@ -210,8 +210,11 @@ IOBuf::IOBuf(CopyBufferOp /* op */,
uint64_t minTailroom) uint64_t minTailroom)
: IOBuf(CREATE, headroom + size + minTailroom) { : IOBuf(CREATE, headroom + size + minTailroom) {
advance(headroom); advance(headroom);
memcpy(writableData(), buf, size); if (size > 0) {
append(size); assert(buf != nullptr);
memcpy(writableData(), buf, size);
append(size);
}
} }
IOBuf::IOBuf(CopyBufferOp op, ByteRange br, IOBuf::IOBuf(CopyBufferOp op, ByteRange br,
...@@ -545,7 +548,10 @@ void IOBuf::unshareOneSlow() { ...@@ -545,7 +548,10 @@ void IOBuf::unshareOneSlow() {
// Maintain the same amount of headroom. Since we maintained the same // Maintain the same amount of headroom. Since we maintained the same
// minimum capacity we also maintain at least the same amount of tailroom. // minimum capacity we also maintain at least the same amount of tailroom.
uint64_t headlen = headroom(); uint64_t headlen = headroom();
memcpy(buf + headlen, data_, length_); if (length_ > 0) {
assert(data_ != nullptr);
memcpy(buf + headlen, data_, length_);
}
// Release our reference on the old buffer // Release our reference on the old buffer
decrementRefcount(); decrementRefcount();
...@@ -666,10 +672,13 @@ void IOBuf::coalesceAndReallocate(size_t newHeadroom, ...@@ -666,10 +672,13 @@ void IOBuf::coalesceAndReallocate(size_t newHeadroom,
IOBuf* current = this; IOBuf* current = this;
size_t remaining = newLength; size_t remaining = newLength;
do { do {
assert(current->length_ <= remaining); if (current->length_ > 0) {
remaining -= current->length_; assert(current->length_ <= remaining);
memcpy(p, current->data_, current->length_); assert(current->data_ != nullptr);
p += current->length_; remaining -= current->length_;
memcpy(p, current->data_, current->length_);
p += current->length_;
}
current = current->next_; current = current->next_;
} while (current != end); } while (current != end);
assert(remaining == 0); assert(remaining == 0);
...@@ -810,7 +819,10 @@ void IOBuf::reserveSlow(uint64_t minHeadroom, uint64_t minTailroom) { ...@@ -810,7 +819,10 @@ void IOBuf::reserveSlow(uint64_t minHeadroom, uint64_t minTailroom) {
throw std::bad_alloc(); throw std::bad_alloc();
} }
newBuffer = static_cast<uint8_t*>(p); newBuffer = static_cast<uint8_t*>(p);
memcpy(newBuffer + minHeadroom, data_, length_); if (length_ > 0) {
assert(data_ != nullptr);
memcpy(newBuffer + minHeadroom, data_, length_);
}
if (sharedInfo()) { if (sharedInfo()) {
freeExtBuffer(); freeExtBuffer();
} }
......
...@@ -1325,3 +1325,16 @@ TEST(IOBuf, Managed) { ...@@ -1325,3 +1325,16 @@ TEST(IOBuf, Managed) {
writableStr(*buf2)[0] = 'x'; writableStr(*buf2)[0] = 'x';
EXPECT_EQ("jelloxorldhelloxorld", toString(*buf1)); EXPECT_EQ("jelloxorldhelloxorld", toString(*buf1));
} }
TEST(IOBuf, CoalesceEmptyBuffers) {
auto b1 = IOBuf::takeOwnership(nullptr, 0);
auto b2 = fromStr("hello");
auto b3 = IOBuf::takeOwnership(nullptr, 0);
b2->appendChain(std::move(b3));
b1->appendChain(std::move(b2));
auto br = b1->coalesce();
EXPECT_TRUE(ByteRange(StringPiece("hello")) == br);
}
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