Commit 57e4ac80 authored by Marshall Cline's avatar Marshall Cline Committed by Facebook Github Bot

Fix "segfault if consumed/moved-out" in Future/SemiFuture/Promise

Summary:
- Fix all cases of "segfault if consumed/moved-out"
  - In Future, SemiFuture, Promise.
  - Those now throw NoState instead.

Reviewed By: yfeldblum

Differential Revision: D7721921

fbshipit-source-id: 76fafcbd30e23d63150196d3d7e404f348b220f2
parent 7e5f29b8
...@@ -194,46 +194,37 @@ T const&& FutureBase<T>::value() const&& { ...@@ -194,46 +194,37 @@ T const&& FutureBase<T>::value() const&& {
template <class T> template <class T>
Try<T>& FutureBase<T>::result() & { Try<T>& FutureBase<T>::result() & {
throwIfInvalid(); return getCore().getTry();
return core_->getTry();
} }
template <class T> template <class T>
Try<T> const& FutureBase<T>::result() const& { Try<T> const& FutureBase<T>::result() const& {
throwIfInvalid(); return getCore().getTry();
return core_->getTry();
} }
template <class T> template <class T>
Try<T>&& FutureBase<T>::result() && { Try<T>&& FutureBase<T>::result() && {
throwIfInvalid(); return std::move(getCore().getTry());
return std::move(core_->getTry());
} }
template <class T> template <class T>
Try<T> const&& FutureBase<T>::result() const&& { Try<T> const&& FutureBase<T>::result() const&& {
throwIfInvalid(); return std::move(getCore().getTry());
return std::move(core_->getTry());
} }
template <class T> template <class T>
bool FutureBase<T>::isReady() const { bool FutureBase<T>::isReady() const {
throwIfInvalid(); return getCore().hasResult();
return core_->ready();
} }
template <class T> template <class T>
bool FutureBase<T>::hasValue() { bool FutureBase<T>::hasValue() {
return core_->getTry().hasValue(); return result().hasValue();
} }
template <class T> template <class T>
bool FutureBase<T>::hasException() { bool FutureBase<T>::hasException() {
return core_->getTry().hasException(); return result().hasException();
} }
template <class T> template <class T>
...@@ -262,14 +253,13 @@ Optional<Try<T>> FutureBase<T>::poll() { ...@@ -262,14 +253,13 @@ Optional<Try<T>> FutureBase<T>::poll() {
template <class T> template <class T>
void FutureBase<T>::raise(exception_wrapper exception) { void FutureBase<T>::raise(exception_wrapper exception) {
core_->raise(std::move(exception)); getCore().raise(std::move(exception));
} }
template <class T> template <class T>
template <class F> template <class F>
void FutureBase<T>::setCallback_(F&& func) { void FutureBase<T>::setCallback_(F&& func) {
throwIfInvalid(); getCore().setCallback(std::forward<F>(func));
core_->setCallback(std::forward<F>(func));
} }
template <class T> template <class T>
...@@ -289,14 +279,12 @@ FutureBase<T>::thenImplementation( ...@@ -289,14 +279,12 @@ FutureBase<T>::thenImplementation(
static_assert(sizeof...(Args) <= 1, "Then must take zero/one argument"); static_assert(sizeof...(Args) <= 1, "Then must take zero/one argument");
typedef typename R::ReturnsFuture::Inner B; typedef typename R::ReturnsFuture::Inner B;
this->throwIfInvalid();
Promise<B> p; Promise<B> p;
p.core_->setInterruptHandlerNoLock(this->core_->getInterruptHandler()); p.core_->setInterruptHandlerNoLock(this->getCore().getInterruptHandler());
// grab the Future now before we lose our handle on the Promise // grab the Future now before we lose our handle on the Promise
auto sf = p.getSemiFuture(); auto sf = p.getSemiFuture();
sf.core_->setExecutor(this->getExecutor()); sf.setExecutor(this->getExecutor());
auto f = Future<B>(sf.core_); auto f = Future<B>(sf.core_);
sf.core_ = nullptr; sf.core_ = nullptr;
...@@ -332,7 +320,6 @@ FutureBase<T>::thenImplementation( ...@@ -332,7 +320,6 @@ FutureBase<T>::thenImplementation(
this->setCallback_( this->setCallback_(
[state = futures::detail::makeCoreCallbackState( [state = futures::detail::makeCoreCallbackState(
std::move(p), std::forward<F>(func))](Try<T>&& t) mutable { std::move(p), std::forward<F>(func))](Try<T>&& t) mutable {
if (!isTry && t.hasException()) { if (!isTry && t.hasException()) {
state.setException(std::move(t.exception())); state.setException(std::move(t.exception()));
} else { } else {
...@@ -368,15 +355,14 @@ FutureBase<T>::thenImplementation( ...@@ -368,15 +355,14 @@ FutureBase<T>::thenImplementation(
futures::detail::argResult<isTry, F, Args...>) { futures::detail::argResult<isTry, F, Args...>) {
static_assert(sizeof...(Args) <= 1, "Then must take zero/one argument"); static_assert(sizeof...(Args) <= 1, "Then must take zero/one argument");
typedef typename R::ReturnsFuture::Inner B; typedef typename R::ReturnsFuture::Inner B;
this->throwIfInvalid();
Promise<B> p; Promise<B> p;
p.core_->setInterruptHandlerNoLock(this->core_->getInterruptHandler()); p.core_->setInterruptHandlerNoLock(this->getCore().getInterruptHandler());
// grab the Future now before we lose our handle on the Promise // grab the Future now before we lose our handle on the Promise
auto sf = p.getSemiFuture(); auto sf = p.getSemiFuture();
auto* e = this->getExecutor(); auto* e = this->getExecutor();
sf.core_->setExecutor(e); sf.setExecutor(e);
auto f = Future<B>(sf.core_); auto f = Future<B>(sf.core_);
sf.core_ = nullptr; sf.core_ = nullptr;
...@@ -637,7 +623,7 @@ SemiFuture<T> SemiFuture<T>::makeEmpty() { ...@@ -637,7 +623,7 @@ SemiFuture<T> SemiFuture<T>::makeEmpty() {
template <class T> template <class T>
typename SemiFuture<T>::DeferredExecutor* SemiFuture<T>::getDeferredExecutor() typename SemiFuture<T>::DeferredExecutor* SemiFuture<T>::getDeferredExecutor()
const { const {
if (auto executor = this->core_->getExecutor()) { if (auto executor = this->getExecutor()) {
assert(dynamic_cast<DeferredExecutor*>(executor) != nullptr); assert(dynamic_cast<DeferredExecutor*>(executor) != nullptr);
return static_cast<DeferredExecutor*>(executor); return static_cast<DeferredExecutor*>(executor);
} }
...@@ -694,7 +680,6 @@ SemiFuture<T>& SemiFuture<T>::operator=(Future<T>&& other) noexcept { ...@@ -694,7 +680,6 @@ SemiFuture<T>& SemiFuture<T>::operator=(Future<T>&& other) noexcept {
template <class T> template <class T>
inline Future<T> SemiFuture<T>::via(Executor* executor, int8_t priority) && { inline Future<T> SemiFuture<T>::via(Executor* executor, int8_t priority) && {
throwIfInvalid();
if (!executor) { if (!executor) {
throwNoExecutor(); throwNoExecutor();
} }
...@@ -884,8 +869,6 @@ typename std:: ...@@ -884,8 +869,6 @@ typename std::
template <class T> template <class T>
inline Future<T> Future<T>::via(Executor* executor, int8_t priority) && { inline Future<T> Future<T>::via(Executor* executor, int8_t priority) && {
this->throwIfInvalid();
this->setExecutor(executor, priority); this->setExecutor(executor, priority);
auto newFuture = Future<T>(this->core_); auto newFuture = Future<T>(this->core_);
...@@ -947,7 +930,7 @@ Future<T>::onError(F&& func) { ...@@ -947,7 +930,7 @@ Future<T>::onError(F&& func) {
"Return type of onError callback must be T or Future<T>"); "Return type of onError callback must be T or Future<T>");
Promise<T> p; Promise<T> p;
p.core_->setInterruptHandlerNoLock(this->core_->getInterruptHandler()); p.core_->setInterruptHandlerNoLock(this->getCore().getInterruptHandler());
auto sf = p.getSemiFuture(); auto sf = p.getSemiFuture();
this->setCallback_( this->setCallback_(
......
...@@ -32,7 +32,6 @@ ...@@ -32,7 +32,6 @@
#include <folly/Utility.h> #include <folly/Utility.h>
#include <folly/executors/DrivableExecutor.h> #include <folly/executors/DrivableExecutor.h>
#include <folly/executors/TimedDrivableExecutor.h> #include <folly/executors/TimedDrivableExecutor.h>
#include <folly/futures/FutureException.h>
#include <folly/futures/Promise.h> #include <folly/futures/Promise.h>
#include <folly/futures/detail/Types.h> #include <folly/futures/detail/Types.h>
...@@ -133,7 +132,7 @@ class FutureBase { ...@@ -133,7 +132,7 @@ class FutureBase {
void setCallback_(F&& func); void setCallback_(F&& func);
bool isActive() { bool isActive() {
return core_->isActive(); return getCore().isActive();
} }
template <class E> template <class E>
...@@ -163,9 +162,30 @@ class FutureBase { ...@@ -163,9 +162,30 @@ class FutureBase {
template <class> template <class>
friend class Future; friend class Future;
using corePtr = futures::detail::Core<T>*; using CoreType = futures::detail::Core<T>;
using corePtr = CoreType*;
// Throws NoState if there is no shared state object; else returns it by ref.
//
// Implementation methods should usually use this instead of `this->core_`.
// The latter should be used only when you need the possibly-null pointer.
CoreType& getCore() {
return getCoreImpl(*this);
}
CoreType const& getCore() const {
return getCoreImpl(*this);
}
template <typename Self>
static decltype(auto) getCoreImpl(Self& self) {
if (!self.core_) {
throwNoState();
}
return *self.core_;
}
// shared core state object // shared core state object
// usually you should use `getCore()` instead of directly accessing `core_`.
corePtr core_; corePtr core_;
explicit FutureBase(corePtr obj) : core_(obj) {} explicit FutureBase(corePtr obj) : core_(obj) {}
...@@ -179,12 +199,12 @@ class FutureBase { ...@@ -179,12 +199,12 @@ class FutureBase {
template <class FutureType> template <class FutureType>
void assign(FutureType&) noexcept; void assign(FutureType&) noexcept;
Executor* getExecutor() { Executor* getExecutor() const {
return core_->getExecutor(); return getCore().getExecutor();
} }
void setExecutor(Executor* x, int8_t priority = Executor::MID_PRI) { void setExecutor(Executor* x, int8_t priority = Executor::MID_PRI) {
core_->setExecutor(x, priority); getCore().setExecutor(x, priority);
} }
// Variant: returns a value // Variant: returns a value
...@@ -740,19 +760,19 @@ class Future : private futures::detail::FutureBase<T> { ...@@ -740,19 +760,19 @@ class Future : private futures::detail::FutureBase<T> {
/// ///
/// Inactive Futures will activate upon destruction. /// Inactive Futures will activate upon destruction.
[[deprecated("do not use")]] Future<T>& activate() & { [[deprecated("do not use")]] Future<T>& activate() & {
this->core_->activate(); this->getCore().activate();
return *this; return *this;
} }
[[deprecated("do not use")]] Future<T>& deactivate() & { [[deprecated("do not use")]] Future<T>& deactivate() & {
this->core_->deactivate(); this->getCore().deactivate();
return *this; return *this;
} }
[[deprecated("do not use")]] Future<T> activate() && { [[deprecated("do not use")]] Future<T> activate() && {
this->core_->activate(); this->getCore().activate();
return std::move(*this); return std::move(*this);
} }
[[deprecated("do not use")]] Future<T> deactivate() && { [[deprecated("do not use")]] Future<T> deactivate() && {
this->core_->deactivate(); this->getCore().deactivate();
return std::move(*this); return std::move(*this);
} }
......
...@@ -50,21 +50,11 @@ Promise<T>& Promise<T>::operator=(Promise<T>&& other) noexcept { ...@@ -50,21 +50,11 @@ Promise<T>& Promise<T>::operator=(Promise<T>&& other) noexcept {
template <class T> template <class T>
void Promise<T>::throwIfFulfilled() { void Promise<T>::throwIfFulfilled() {
if (!core_) { if (getCore().hasResult()) {
throwNoState();
}
if (core_->ready()) {
throwPromiseAlreadySatisfied(); throwPromiseAlreadySatisfied();
} }
} }
template <class T>
void Promise<T>::throwIfRetrieved() {
if (retrieved_) {
throwFutureAlreadyRetrieved();
}
}
template <class T> template <class T>
Promise<T>::Promise(futures::detail::EmptyConstruct) noexcept Promise<T>::Promise(futures::detail::EmptyConstruct) noexcept
: retrieved_(false), core_(nullptr) {} : retrieved_(false), core_(nullptr) {}
...@@ -87,9 +77,11 @@ void Promise<T>::detach() { ...@@ -87,9 +77,11 @@ void Promise<T>::detach() {
template <class T> template <class T>
SemiFuture<T> Promise<T>::getSemiFuture() { SemiFuture<T> Promise<T>::getSemiFuture() {
throwIfRetrieved(); if (retrieved_) {
throwFutureAlreadyRetrieved();
}
retrieved_ = true; retrieved_ = true;
return SemiFuture<T>(core_); return SemiFuture<T>(&getCore());
} }
template <class T> template <class T>
...@@ -120,7 +112,7 @@ void Promise<T>::setException(exception_wrapper ew) { ...@@ -120,7 +112,7 @@ void Promise<T>::setException(exception_wrapper ew) {
template <class T> template <class T>
void Promise<T>::setInterruptHandler( void Promise<T>::setInterruptHandler(
std::function<void(exception_wrapper const&)> fn) { std::function<void(exception_wrapper const&)> fn) {
core_->setInterruptHandler(std::move(fn)); getCore().setInterruptHandler(std::move(fn));
} }
template <class T> template <class T>
......
...@@ -19,6 +19,7 @@ ...@@ -19,6 +19,7 @@
#include <folly/Portability.h> #include <folly/Portability.h>
#include <folly/Try.h> #include <folly/Try.h>
#include <functional> #include <functional>
#include <folly/futures/FutureException.h>
namespace folly { namespace folly {
...@@ -117,7 +118,6 @@ class Promise { ...@@ -117,7 +118,6 @@ class Promise {
bool isFulfilled() const noexcept; bool isFulfilled() const noexcept;
private: private:
typedef typename Future<T>::corePtr corePtr;
template <class> template <class>
friend class futures::detail::FutureBase; friend class futures::detail::FutureBase;
template <class> template <class>
...@@ -130,13 +130,35 @@ class Promise { ...@@ -130,13 +130,35 @@ class Promise {
// Whether the Future has been retrieved (a one-time operation). // Whether the Future has been retrieved (a one-time operation).
bool retrieved_; bool retrieved_;
using CoreType = typename Future<T>::CoreType;
using corePtr = typename Future<T>::corePtr;
// Throws NoState if there is no shared state object; else returns it by ref.
//
// Implementation methods should usually use this instead of `this->core_`.
// The latter should be used only when you need the possibly-null pointer.
CoreType& getCore() {
return getCoreImpl(core_);
}
CoreType const& getCore() const {
return getCoreImpl(core_);
}
template <typename CoreT>
static CoreT& getCoreImpl(CoreT* core) {
if (!core) {
throwNoState();
}
return *core;
}
// shared core state object // shared core state object
// usually you should use `getCore()` instead of directly accessing `core_`.
corePtr core_; corePtr core_;
explicit Promise(futures::detail::EmptyConstruct) noexcept; explicit Promise(futures::detail::EmptyConstruct) noexcept;
void throwIfFulfilled(); void throwIfFulfilled();
void throwIfRetrieved();
void detach(); void detach();
}; };
......
...@@ -146,11 +146,10 @@ class Core final { ...@@ -146,11 +146,10 @@ class Core final {
/// May call from any thread /// May call from any thread
Try<T>& getTry() { Try<T>& getTry() {
if (ready()) { return getTryImpl(*this);
return *result_;
} else {
throwFutureNotReady();
} }
Try<T> const& getTry() const {
return getTryImpl(*this);
} }
/// Call only from Future thread. /// Call only from Future thread.
...@@ -242,7 +241,7 @@ class Core final { ...@@ -242,7 +241,7 @@ class Core final {
} }
/// May call from any thread /// May call from any thread
bool isActive() { return active_.load(std::memory_order_acquire); } bool isActive() const { return active_.load(std::memory_order_acquire); }
/// Call only from Future thread, either before attaching a callback or after /// Call only from Future thread, either before attaching a callback or after
/// the callback has already been invoked, but not concurrently with anything /// the callback has already been invoked, but not concurrently with anything
...@@ -254,7 +253,7 @@ class Core final { ...@@ -254,7 +253,7 @@ class Core final {
priority_ = priority; priority_ = priority;
} }
Executor* getExecutor() { Executor* getExecutor() const {
return executor_; return executor_;
} }
...@@ -413,6 +412,15 @@ class Core final { ...@@ -413,6 +412,15 @@ class Core final {
} }
} }
template <typename Self>
static decltype(auto) getTryImpl(Self& self) {
if (self.ready()) {
return *self.result_;
} else {
throwFutureNotReady();
}
}
folly::Function<void(Try<T>&&)> callback_; folly::Function<void(Try<T>&&)> callback_;
// place result_ next to increase the likelihood that the value will be // place result_ next to increase the likelihood that the value will be
// contained entirely in one cache line // contained entirely in one cache line
......
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