Skip to content
This repository has been archived by the owner on Jan 7, 2022. It is now read-only.

Commit

Permalink
Fix use-after-move in FileBasedVersionedConfigStore
Browse files Browse the repository at this point in the history 
Summary:
I feel bad for such a hacky fix, but (a) I don't have better simple ideas, (b) it's only used in tests.

Stack trace:
  (gdb) bt
  #0  raise (sig=<optimized out>) at ../sysdeps/unix/sysv/linux/raise.c:50
  #1  0x000000000096052a in facebook::logdevice::handle_fatal_signal (sig=<optimized out>) at logdevice/server/fatalsignal.cpp:58
  #2  <signal handler called>
  #3  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
  #4  0x00007f0b92e3b935 in __GI_abort () at abort.c:90
  #5  0x00007f0b940fc395 in __gnu_cxx::__verbose_terminate_handler () at ../../.././libstdc++-v3/libsupc++/vterminate.cc:95
  #6  0x00007f0b940fa016 in __cxxabiv1::__terminate (handler=<optimized out>) at ../../.././libstdc++-v3/libsupc++/eh_terminate.cc:47
  #7  0x00007f0b940fa061 in std::terminate () at ../../.././libstdc++-v3/libsupc++/eh_terminate.cc:57
  #8  0x00007f0b940fa33b in __cxxabiv1::__cxa_throw (obj=<optimized out>, tinfo=0x26e5800 <typeinfo for std::bad_function_call>, dest=0x415cf0 <std::bad_function_call::~bad_function_call()plt>) at ../../.././libstdc++-v3/libsupc++/eh_throw.cc:93
  #9  0x00000000004b5f92 in folly::throw_exception<std::bad_function_call> (ex=...) at folly/lang/Exception.h:36
  #10 0x00000000004b5f57 in folly::detail::throw_exception_<std::bad_function_call> () at folly/lang/Exception.h:64
  #11 0x0000000000d70019 in folly::throw_exception<std::bad_function_call> () at folly/lang/Exception.h:82
  #12 folly::detail::function::FunctionTraits<void (facebook::logdevice::E, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >)>::uninitCall(folly::detail::function::Data&, facebook::logdevice::E&&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&&) () at folly/Function.h:372
  #13 0x0000000000f023f8 in folly::detail::function::FunctionTraits<void (facebook::logdevice::E, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >)>::operator()(facebook::logdevice::E, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) (this=<optimized out>, args=..., args=...) at folly/Function.h:377
  #14 facebook::logdevice::FileBasedVersionedConfigStore::getConfig(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, folly::Function<void (facebook::logdevice::E, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >)>, folly::Optional<facebook::logdevice::vcs_config_version_t>) const (
      this=<optimized out>, key=..., cb=..., base_version=...) at logdevice/common/FileBasedVersionedConfigStore.cpp:258
  #15 0x0000000000d76019 in facebook::logdevice::configuration::nodes::VersionedNodesConfigurationStore<facebook::logdevice::FileBasedVersionedConfigStore>::getConfig(folly::Function<void (facebook::logdevice::E, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >)>, folly::Optional<facebook::logdevice::vcs_config_version_t>) const (
      this=<optimized out>, cb=..., base_version=...) at logdevice/common/configuration/nodes/NodesConfigurationStore.h:88
  #16 0x0000000000d6de2f in facebook::logdevice::configuration::nodes::ncm::Dependencies::readFromStore (this=0x7f0b91967fa0, should_do_consistent_config_fetch=false) at logdevice/common/configuration/nodes/NodesConfigurationManagerDependencies.cpp:342
  #17 0x0000000000d5e64e in facebook::logdevice::configuration::nodes::NodesConfigurationManager::onHeartBeat (this=0x7f0b90d0df10) at logdevice/common/configuration/nodes/NodesConfigurationManager.cpp:538
  #18 0x0000000000d73073 in facebook::logdevice::configuration::nodes::ncm::Dependencies::scheduleHeartBeat()::$_4::operator()() const (this=<optimized out>) at logdevice/common/configuration/nodes/NodesConfigurationManagerDependencies.cpp:365
  #19 std::_Function_handler<void (), facebook::logdevice::configuration::nodes::ncm::Dependencies::scheduleHeartBeat()::$_4>::_M_invoke(std::_Any_data const&) (__functor=...) at third-party-buck/platform007/build/libgcc/include/c++/trunk/bits/std_function.h:316
  #20 0x0000000000e250a6 in std::function<void ()>::operator()() const (this=<optimized out>) at third-party-buck/platform007/build/libgcc/include/c++/trunk/bits/std_function.h:706
  #21 facebook::logdevice::(anonymous namespace)::WheelTimerDispatchImpl::makeWheelTimerInternalExecutor(facebook::logdevice::Worker*)::$_0::operator()()::{lambda()#1}::operator()() const (this=0x7f0b8bf34070) at logdevice/common/Timer.cpp:123
  #22 folly::detail::function::FunctionTraits<void ()>::callSmall<facebook::logdevice::(anonymous namespace)::WheelTimerDispatchImpl::makeWheelTimerInternalExecutor(facebook::logdevice::Worker*)::$_0::operator()()::{lambda()#1}>(folly::detail::function::Data&) (p=...) at folly/Function.h:361
  #23 0x0000000000e63602 in folly::detail::function::FunctionTraits<void ()>::operator()() (this=<optimized out>) at folly/Function.h:377
  #24 facebook::logdevice::Worker::addWithPriority(folly::Function<void ()>, signed char)::$_9::operator()() (this=0x7f0b8bf34060) at logdevice/common/Worker.cpp:1373
  #25 folly::detail::function::FunctionTraits<void ()>::callBig<facebook::logdevice::Worker::addWithPriority(folly::Function<void ()>, signed char)::$_9>(folly::detail::function::Data&) (p=...) at folly/Function.h:368
  #26 0x00000000010d7e75 in folly::detail::function::FunctionTraits<void ()>::operator()() (this=0x7f0b8bf34060) at folly/Function.h:377
  #27 facebook::logdevice::EventLoopTaskQueue::executeTasks (this=0x7f0b74023000, tokens=<optimized out>) at logdevice/common/EventLoopTaskQueue.cpp:154
  #28 0x00000000010d7699 in facebook::logdevice::EventLoopTaskQueue::haveTasksEventHandler()::$_1::operator()(unsigned int) const (this=<optimized out>, n=1) at logdevice/common/EventLoopTaskQueue.cpp:101
  #29 facebook::logdevice::LifoEventSemImpl<std::atomic>::AsyncWaiter::processBatch<facebook::logdevice::EventLoopTaskQueue::haveTasksEventHandler()::$_1&>(facebook::logdevice::EventLoopTaskQueue::haveTasksEventHandler()::$_1&, unsigned int) (this=<optimized out>, func=..., maxBatchSize=<optimized out>) at logdevice/common/LifoEventSem.h:351
  #30 facebook::logdevice::EventLoopTaskQueue::haveTasksEventHandler (this=0x7f0b74023000) at logdevice/common/EventLoopTaskQueue.cpp:106
  #31 0x0000000001a907f2 in event_process_active_single_queue (base=<optimized out>, activeq=0x7f0b74020010, max_to_process=2147483647, endtime=0x0) at logdevice/external/libevent-2.1.3-alpha/event.c:1449
  #32 0x0000000001a8efae in event_process_active (base=<optimized out>) at logdevice/external/libevent-2.1.3-alpha/event.c:1599
  #33 ld_event_base_loop (base=0x7f0b7401b000, flags=<optimized out>) at logdevice/external/libevent-2.1.3-alpha/event.c:1819
  #34 0x000000000176b5f1 in facebook::logdevice::EvBaseLegacy::loop (this=0x7f0b74000018) at logdevice/common/libevent/EvBaseLegacy.cpp:58
  #35 0x00000000010d5a79 in facebook::logdevice::EventLoop::run (this=<optimized out>) at logdevice/common/EventLoop.cpp:140
  #36 0x00000000016fa223 in std::function<void ()>::operator()() const (this=0x7f0b90d0c520) at third-party-buck/platform007/build/libgcc/include/c++/trunk/bits/std_function.h:706
  #37 facebook::logdevice::thread_func (arg=0x7f0b90d0c520) at logdevice/common/PThread.cpp:9
  #38 0x00007f0b936136b6 in start_thread (arg=0x7f0b74dff700) at pthread_create.c:465
  #39 0x00007f0b92f2cebf in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

Reviewed By: gdrane

Differential Revision: D17975422

fbshipit-source-id: 499c6aceb577f859e9d4e71006b86f73537221ab
  • Loading branch information
@al13n321 @facebook-github-bot
al13n321 authored and facebook-github-bot committed Oct 23, 2019
1 parent 1a05c83 commit 76ddda5
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 15 deletions.
34 changes: 21 additions & 13 deletions logdevice/common/FileBasedVersionedConfigStore.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ void FileBasedVersionedConfigStore::threadMain() {

void FileBasedVersionedConfigStore::getConfigImpl(
std::string key,
value_callback_t cb,
value_callback_t::SharedProxy cb,
folly::Optional<version_t> base_version) const {
if (shutdown_signaled_.load()) {
cb(E::SHUTDOWN, "");
Expand Down Expand Up @@ -146,7 +146,7 @@ void FileBasedVersionedConfigStore::updateConfigImpl(
std::string value,
version_t new_version,
folly::Optional<version_t> base_version,
write_callback_t cb) {
write_callback_t::SharedProxy cb) {
if (shutdown_signaled_.load()) {
cb(E::SHUTDOWN, {}, "");
return;
Expand Down Expand Up @@ -218,7 +218,9 @@ void FileBasedVersionedConfigStore::updateConfigImpl(
if (base_version.hasValue() && base_version != current_version) {
// version conditional update failed, invoke the callback with the
// version and value that are more recent
cb(E::VERSION_MISMATCH, current_version, std::move(current_value));
cb(E::VERSION_MISMATCH,
std::move(current_version),
std::move(current_value));
return;
}

Expand All @@ -237,7 +239,7 @@ void FileBasedVersionedConfigStore::updateConfigImpl(
return;
}

cb(E::OK, new_version, "");
cb(E::OK, std::move(new_version), "");
}

void FileBasedVersionedConfigStore::getConfig(
Expand All @@ -249,13 +251,18 @@ void FileBasedVersionedConfigStore::getConfig(
return;
}

bool success = task_queue_.writeIfNotFull(
[this, key = std::move(key), cb = std::move(cb), base_version]() mutable {
getConfigImpl(std::move(key), std::move(cb), base_version);
});
auto cb_shared = std::move(cb).asSharedProxy();
bool success = task_queue_.writeIfNotFull([this,
key = std::move(key),
cb_shared = cb_shared,
base_version]() mutable {
getConfigImpl(std::move(key), cb_shared, base_version);
});
if (!success) {
// queue full, report transient error
cb(E::AGAIN, {});
// Queue full, report transient error.
// `func` was not moved out of, so it wasn't destroyed and wasn't called,
// so cb_plain is still alive.
cb_shared(E::AGAIN, {});
return;
}
}
Expand Down Expand Up @@ -325,22 +332,23 @@ void FileBasedVersionedConfigStore::readModifyWriteConfig(
new_version.val());
}

auto cb_shared = std::move(cb).asSharedProxy();
bool success =
task_queue_.writeIfNotFull([this,
key = std::move(key),
value = std::move(write_value),
base_version = std::move(cur_ver),
new_version = std::move(new_version),
cb = std::move(cb)]() mutable {
cb_shared = cb_shared]() mutable {
updateConfigImpl(std::move(key),
std::move(value),
std::move(new_version),
std::move(base_version),
std::move(cb));
cb_shared);
});
if (!success) {
// queue full, report transient error
cb(E::AGAIN, {}, "");
cb_shared(E::AGAIN, {}, "");
return;
}
}
Expand Down
4 changes: 2 additions & 2 deletions logdevice/common/FileBasedVersionedConfigStore.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,13 @@ class FileBasedVersionedConfigStore : public VersionedConfigStore {

// must be executed on the TaskThread
void getConfigImpl(std::string key,
value_callback_t cb,
value_callback_t::SharedProxy cb,
folly::Optional<version_t> base_version) const;
void updateConfigImpl(std::string key,
std::string value,
version_t version,
folly::Optional<version_t> base_version,
write_callback_t cb);
write_callback_t::SharedProxy cb);

std::string getDataFilePath(const std::string& key) const {
return root_path_ + "/" + key;
Expand Down

0 comments on commit 76ddda5

Please sign in to comment.