Skip to content

Commit

Permalink
Fix TMemoryBuffer::ensureCanWrite
Browse files Browse the repository at this point in the history
Summary:
Fix `TMemoryBuffer::ensureCanWrite` for the empty buffer. Previously there was a UB because null pointer wasn't handled properly:

```
thrift/lib/cpp/transport/TBufferTransports.cpp:458:10: runtime error: applying non-zero offset 107271103209344 to null pointer
    #0 0x7f90a738cc31 in apache::thrift::transport::TMemoryBuffer::ensureCanWrite(unsigned int) thrift/lib/cpp/transport/TBufferTransports.cpp:458
    #1 0x7f90a7769b4e in apache::thrift::transport::TMemoryBuffer::getWritePtr(unsigned int) thrift/lib/cpp/transport/TBufferTransports.h:809
    #2 0x7f90a7769ae0 in apache::thrift::async::detail::TFramedACReadState::getReadBuffer(void**, unsigned long*) thrift/lib/cpp/async/TFramedAsyncChannel.cpp:
86
    #3 0x30909b in apache::thrift::async::TStreamAsyncChannel<apache::thrift::async::detail::TFramedACWriteRequest, apache::thrift::async::detail::TFramedACRea
dState>::getReadBuffer(void**, unsigned long*) thrift/lib/cpp/async/TStreamAsyncChannel-inl.h:260
    #4 0x7f90a767028d in folly::AsyncSocket::prepareReadBuffer(void**, unsigned long*) folly/io/async/AsyncSocket.cpp:2517
    #5 0x7f90a7677cf2 in folly::AsyncSocket::processNormalRead() folly/io/async/AsyncSocket.cpp:2869
    #6 0x7f90a767a1f4 in folly::AsyncSocket::handleRead() folly/io/async/AsyncSocket.cpp:2977
    #7 0x7f90a766d89a in folly::AsyncSocket::ioReady(unsigned short) folly/io/async/AsyncSocket.cpp:2398
    #8 0x7f90a76a442a in folly::AsyncSocket::IoHandler::handlerReady(unsigned short) folly/io/async/AsyncSocket.h:1294
    #9 0x7f90a75998d3 in folly::EventHandler::libeventCallback(int, short, void*) folly/io/async/EventHandler.cpp:159
    #10 0x7f90a6943608 in event_process_active /home/engshare/third-party2/libevent/1.4.14b_hphp/src/libevent-1.4.14b-stable/event.c:390:5
    #11 0x7f90a6943608 in event_base_loop /home/engshare/third-party2/libevent/1.4.14b_hphp/src/libevent-1.4.14b-stable/event.c:532:4
    #12 0x7f90a754aa23 in (anonymous namespace)::EventBaseBackend::eb_event_base_loop(int) folly/io/async/EventBase.cpp:74
    #13 0x7f90a7538e16 in folly::EventBase::loopBody(int, bool) folly/io/async/EventBase.cpp:381
    #14 0x7f90a75377f2 in folly::EventBase::loop() folly/io/async/EventBase.cpp:305
    #15 0x33722c in SocketPairTest<apache::thrift::async::TFramedAsyncChannel>::loop(unsigned int) thrift/lib/cpp/test/TAsyncChannelTest.cpp:506
    #16 0x3043a3 in SocketPairTest<apache::thrift::async::TFramedAsyncChannel>::runWithTimeout(unsigned int) thrift/lib/cpp/test/TAsyncChannelTest.cpp:513
    #17 0x3035f9 in SocketPairTest<apache::thrift::async::TFramedAsyncChannel>::run() thrift/lib/cpp/test/TAsyncChannelTest.cpp:509
    #18 0x2f6766 in TAsyncChannelTest_TestMultiSendRecvFramedQueued_Test::TestBody() thrift/lib/cpp/test/TAsyncChannelTest.cpp:729
    #19 0x7f90a6cc9fbf in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char c
onst*) /home/engshare/third-party2/googletest/1.11.0/src/googletest/googletest/src/gtest.cc:2607:27
    #20 0x7f90a6cc9fbf in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char cons
t*) /home/engshare/third-party2/googletest/1.11.0/src/googletest/googletest/src/gtest.cc:2643:52
    #21 0x7f90a6cb9ad5 in testing::Test::Run() /home/engshare/third-party2/googletest/1.11.0/src/googletest/googletest/src/gtest.cc:2682:50
    #22 0x7f90a6cb9ad5 in testing::Test::Run() /home/engshare/third-party2/googletest/1.11.0/src/googletest/googletest/src/gtest.cc:2672:6
    #23 0x7f90a6cb9c64 in testing::TestInfo::Run() /home/engshare/third-party2/googletest/1.11.0/src/googletest/googletest/src/gtest.cc:2861:14
    #24 0x7f90a6cb9c64 in testing::TestInfo::Run() /home/engshare/third-party2/googletest/1.11.0/src/googletest/googletest/src/gtest.cc:2833:6
    #25 0x7f90a6cba321 in testing::TestSuite::Run() /home/engshare/third-party2/googletest/1.11.0/src/googletest/googletest/src/gtest.cc:3015:31
    #26 0x7f90a6cba321 in testing::TestSuite::Run() /home/engshare/third-party2/googletest/1.11.0/src/googletest/googletest/src/gtest.cc:2993:6
    #27 0x7f90a6cbab1e in testing::internal::UnitTestImpl::RunAllTests() /home/engshare/third-party2/googletest/1.11.0/src/googletest/googletest/src/gtest.cc:5
855:47
    #28 0x7f90a6cb9d2c in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl
*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/engshare/third-party2/googletest/1.11.0/src/googletest/googletest/src/gtest.cc:2607:27
    #29 0x7f90a6cb9d2c in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*,
bool (testing::internal::UnitTestImpl::*)(), char const*) /home/engshare/third-party2/googletest/1.11.0/src/googletest/googletest/src/gtest.cc:2643:52
    #30 0x7f90a6cb9d2c in testing::UnitTest::Run() /home/engshare/third-party2/googletest/1.11.0/src/googletest/googletest/src/gtest.cc:5438:55
    #31 0x7f90a75dbae0 in RUN_ALL_TESTS() third-party-buck/platform010/build/googletest/include/gtest/gtest.h:2490
    #32 0x7f90a75db758 in main common/gtest/LightMain.cpp:20
    #33 0x7f90a4e9b656 in __libc_start_call_main /home/engshare/third-party2/glibc/2.34/src/glibc-2.34/csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #34 0x7f90a4e9b717 in __libc_start_main_impl /home/engshare/third-party2/glibc/2.34/src/glibc-2.34/csu/../csu/libc-start.c:409:3
    #35 0x2ecbe0 in _start /home/engshare/third-party2/glibc/2.34/src/glibc-2.34/csu/../sysdeps/x86_64/start.S:116
```

Reviewed By: iahs

Differential Revision: D35723595

fbshipit-source-id: 3b0f3104a0e4551a94fcb45f2ccadcca85b699cd
  • Loading branch information
vitaut authored and facebook-github-bot committed Apr 18, 2022
1 parent 5846b97 commit 403d799
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 11 deletions.
20 changes: 10 additions & 10 deletions thrift/lib/cpp/transport/TBufferTransports.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) Facebook, Inc. and its affiliates.
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -416,8 +416,7 @@ void TMemoryBuffer::ensureCanWrite(uint32_t len) {
}

// Allocate into a new pointer so we don't bork ours if it fails.
void* new_buffer = nullptr;
ptrdiff_t offset = 0;
uint8_t* new_buffer = nullptr;
if (copy) {
/* This is not a memory leak, because an observed buffer still owns the
* previous buffer pointer.
Expand All @@ -428,7 +427,7 @@ void TMemoryBuffer::ensureCanWrite(uint32_t len) {
* setIdleWriteBufferLimit, and setIdleReadBufferLimit.
* This will likely solve the issue.
*/
new_buffer = std::malloc(new_size);
new_buffer = static_cast<uint8_t*>(std::malloc(new_size));
if (new_buffer == nullptr) {
throw std::bad_alloc();
}
Expand All @@ -440,7 +439,7 @@ void TMemoryBuffer::ensureCanWrite(uint32_t len) {
memmove(buffer_, rBase_, wBase_ - rBase_);
}
if (new_size > bufferSize_) {
new_buffer = std::realloc(buffer_, new_size);
new_buffer = static_cast<uint8_t*>(std::realloc(buffer_, new_size));
if (new_buffer == nullptr) {
throw std::bad_alloc();
}
Expand All @@ -449,13 +448,14 @@ void TMemoryBuffer::ensureCanWrite(uint32_t len) {
new_buffer = buffer_;
}
}
offset = (uint8_t*)new_buffer - rBase_;
ptrdiff_t rBoundOffset = rBound_ - rBase_;
ptrdiff_t wBaseOffset = wBase_ - rBase_;
bufferSize_ = new_size;

buffer_ = (uint8_t*)new_buffer;
rBase_ += offset;
rBound_ += offset;
wBase_ += offset;
buffer_ = new_buffer;
rBase_ = new_buffer;
rBound_ = new_buffer + rBoundOffset;
wBase_ = new_buffer + wBaseOffset;
wBound_ = buffer_ + bufferSize_;
}

Expand Down
2 changes: 1 addition & 1 deletion thrift/lib/cpp/transport/TBufferTransports.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) Facebook, Inc. and its affiliates.
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down

0 comments on commit 403d799

Please sign in to comment.