Commit bff1bb83 authored by Giuseppe Ottaviano's avatar Giuseppe Ottaviano Committed by Facebook GitHub Bot

Fix race between setProxy() and setCallback()

Summary:
D22371898 (https://github.com/facebook/folly/commit/4981497ad3333ab084e18b2a02a574cbf9438585) introduced a race by attempting to share the storage between `proxy_` and `callback_`: `setProxy()` and `setCallback()` may concurrently try to set both, and there's no good ordering we can use in `setCallback()` to fetch the proxy pointer before constructing the callback.

So revert to the logic pre-D22371898 (https://github.com/facebook/folly/commit/4981497ad3333ab084e18b2a02a574cbf9438585). This means increasing the `Core` footprint by a further 8 bytes, but D22474230 should recover that.

Differential Revision: D22474532

fbshipit-source-id: c63ca6ecd166fb71dcbeddbb2c04eb07494f99ea
parent 1ad7426b
...@@ -336,18 +336,11 @@ void CoreBase::setCallback_( ...@@ -336,18 +336,11 @@ void CoreBase::setCallback_(
std::shared_ptr<folly::RequestContext>&& context, std::shared_ptr<folly::RequestContext>&& context,
futures::detail::InlineContinuation allowInline) { futures::detail::InlineContinuation allowInline) {
DCHECK(!hasCallback()); DCHECK(!hasCallback());
auto state = state_.load(std::memory_order_acquire);
CoreBase* proxy = nullptr;
if (state == State::Proxy) {
// proxy_ and callback_ share the storage, retrieve the pointer before we
// construct callback_.
proxy = proxy_;
}
::new (&callback_) Callback(std::move(callback)); ::new (&callback_) Callback(std::move(callback));
::new (&context_) Context(std::move(context)); ::new (&context_) Context(std::move(context));
auto state = state_.load(std::memory_order_acquire);
State nextState = allowInline == futures::detail::InlineContinuation::permit State nextState = allowInline == futures::detail::InlineContinuation::permit
? State::OnlyCallbackAllowInline ? State::OnlyCallbackAllowInline
: State::OnlyCallback; : State::OnlyCallback;
...@@ -371,7 +364,7 @@ void CoreBase::setCallback_( ...@@ -371,7 +364,7 @@ void CoreBase::setCallback_(
} }
if (state == State::Proxy) { if (state == State::Proxy) {
return proxyCallback(proxy, state); return proxyCallback(state);
} }
terminate_with<std::logic_error>("setCallback unexpected state"); terminate_with<std::logic_error>("setCallback unexpected state");
...@@ -413,6 +406,8 @@ void CoreBase::setResult_(Executor::KeepAlive<>&& completingKA) { ...@@ -413,6 +406,8 @@ void CoreBase::setResult_(Executor::KeepAlive<>&& completingKA) {
void CoreBase::setProxy_(CoreBase* proxy) { void CoreBase::setProxy_(CoreBase* proxy) {
DCHECK(!hasResult()); DCHECK(!hasResult());
proxy_ = proxy;
auto state = state_.load(std::memory_order_acquire); auto state = state_.load(std::memory_order_acquire);
switch (state) { switch (state) {
case State::Start: case State::Start:
...@@ -431,7 +426,7 @@ void CoreBase::setProxy_(CoreBase* proxy) { ...@@ -431,7 +426,7 @@ void CoreBase::setProxy_(CoreBase* proxy) {
case State::OnlyCallback: case State::OnlyCallback:
case State::OnlyCallbackAllowInline: case State::OnlyCallbackAllowInline:
proxyCallback(proxy, state); proxyCallback(state);
break; break;
case State::OnlyResult: case State::OnlyResult:
case State::Proxy: case State::Proxy:
...@@ -441,10 +436,6 @@ void CoreBase::setProxy_(CoreBase* proxy) { ...@@ -441,10 +436,6 @@ void CoreBase::setProxy_(CoreBase* proxy) {
terminate_with<std::logic_error>("setCallback unexpected state"); terminate_with<std::logic_error>("setCallback unexpected state");
} }
// proxy_ and callback_ share the storage, so this needs to be done after
// proxyCallback() is called.
proxy_ = proxy;
detachOne(); detachOne();
} }
...@@ -526,7 +517,7 @@ void CoreBase::doCallback( ...@@ -526,7 +517,7 @@ void CoreBase::doCallback(
} }
} }
void CoreBase::proxyCallback(CoreBase* proxy, State priorState) { void CoreBase::proxyCallback(State priorState) {
// If the state of the core being proxied had a callback that allows inline // If the state of the core being proxied had a callback that allows inline
// execution, maintain this information in the proxy // execution, maintain this information in the proxy
futures::detail::InlineContinuation allowInline = futures::detail::InlineContinuation allowInline =
...@@ -534,9 +525,9 @@ void CoreBase::proxyCallback(CoreBase* proxy, State priorState) { ...@@ -534,9 +525,9 @@ void CoreBase::proxyCallback(CoreBase* proxy, State priorState) {
? futures::detail::InlineContinuation::permit ? futures::detail::InlineContinuation::permit
: futures::detail::InlineContinuation::forbid); : futures::detail::InlineContinuation::forbid);
state_.store(State::Empty, std::memory_order_relaxed); state_.store(State::Empty, std::memory_order_relaxed);
proxy->setExecutor(std::move(executor_)); proxy_->setExecutor(std::move(executor_));
proxy->setCallback_(std::move(callback_), std::move(context_), allowInline); proxy_->setCallback_(std::move(callback_), std::move(context_), allowInline);
proxy->detachFuture(); proxy_->detachFuture();
context_.~Context(); context_.~Context();
callback_.~Callback(); callback_.~Callback();
} }
......
...@@ -433,14 +433,13 @@ class CoreBase { ...@@ -433,14 +433,13 @@ class CoreBase {
void setResult_(Executor::KeepAlive<>&& completingKA); void setResult_(Executor::KeepAlive<>&& completingKA);
void setProxy_(CoreBase* proxy); void setProxy_(CoreBase* proxy);
void doCallback(Executor::KeepAlive<>&& completingKA, State priorState); void doCallback(Executor::KeepAlive<>&& completingKA, State priorState);
void proxyCallback(CoreBase* proxy, State priorState); void proxyCallback(State priorState);
void detachOne() noexcept; void detachOne() noexcept;
void derefCallback() noexcept; void derefCallback() noexcept;
union { union {
CoreBase* proxy_;
Callback callback_; Callback callback_;
}; };
std::atomic<State> state_; std::atomic<State> state_;
...@@ -454,6 +453,7 @@ class CoreBase { ...@@ -454,6 +453,7 @@ class CoreBase {
}; };
std::unique_ptr<exception_wrapper> interrupt_{}; std::unique_ptr<exception_wrapper> interrupt_{};
std::function<void(exception_wrapper const&)> interruptHandler_{nullptr}; std::function<void(exception_wrapper const&)> interruptHandler_{nullptr};
CoreBase* proxy_;
}; };
template <typename T> template <typename T>
......
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