Unverified Commit fe2f4d60 authored by Dominik Charousset's avatar Dominik Charousset Committed by GitHub

Merge pull request #1307

Fix heap-use-after free bug
parents 9d5165d6 f1c65ff8
...@@ -22,6 +22,9 @@ is based on [Keep a Changelog](https://keepachangelog.com). ...@@ -22,6 +22,9 @@ is based on [Keep a Changelog](https://keepachangelog.com).
- Using `allow(...).with(...)` in unit tests without a matching message crashed - Using `allow(...).with(...)` in unit tests without a matching message crashed
the program. By adding a missing NULL-check, `allow` is now always safe to the program. By adding a missing NULL-check, `allow` is now always safe to
use. use.
- Passing a response promise to a run-delayed continuation could result in a
heap-use-after-free if the actor terminates before the action runs. The
destructor of the promise now checks for this case.
### Changed ### Changed
......
...@@ -197,7 +197,7 @@ private: ...@@ -197,7 +197,7 @@ private:
void delegate_impl(abstract_actor* receiver, message msg); void delegate_impl(abstract_actor* receiver, message msg);
mutable size_t ref_count = 1; mutable size_t ref_count = 1;
local_actor* self; weak_actor_ptr weak_self;
strong_actor_ptr source; strong_actor_ptr source;
forwarding_stack stages; forwarding_stack stages;
message_id id; message_id id;
......
...@@ -40,7 +40,7 @@ response_promise::response_promise(local_actor* self, strong_actor_ptr source, ...@@ -40,7 +40,7 @@ response_promise::response_promise(local_actor* self, strong_actor_ptr source,
// anonymous messages since there's nowhere to send the message to anyway. // anonymous messages since there's nowhere to send the message to anyway.
if (requires_response(mid)) { if (requires_response(mid)) {
state_ = make_counted<state>(); state_ = make_counted<state>();
state_->self = self; state_->weak_self = self->ctrl();
state_->source.swap(source); state_->source.swap(source);
state_->stages.swap(stages); state_->stages.swap(stages);
state_->id = mid; state_->id = mid;
...@@ -60,7 +60,7 @@ bool response_promise::async() const noexcept { ...@@ -60,7 +60,7 @@ bool response_promise::async() const noexcept {
} }
bool response_promise::pending() const noexcept { bool response_promise::pending() const noexcept {
return state_ != nullptr && state_->self != nullptr; return state_ != nullptr && state_->weak_self != nullptr;
} }
strong_actor_ptr response_promise::source() const noexcept { strong_actor_ptr response_promise::source() const noexcept {
...@@ -122,7 +122,7 @@ void response_promise::respond_to(local_actor* self, mailbox_element* request, ...@@ -122,7 +122,7 @@ void response_promise::respond_to(local_actor* self, mailbox_element* request,
if (request && requires_response(*request) if (request && requires_response(*request)
&& has_response_receiver(*request)) { && has_response_receiver(*request)) {
state tmp; state tmp;
tmp.self = self; tmp.weak_self = self->ctrl();
tmp.source.swap(request->sender); tmp.source.swap(request->sender);
tmp.stages.swap(request->stages); tmp.stages.swap(request->stages);
tmp.id = request->mid; tmp.id = request->mid;
...@@ -136,7 +136,7 @@ void response_promise::respond_to(local_actor* self, mailbox_element* request, ...@@ -136,7 +136,7 @@ void response_promise::respond_to(local_actor* self, mailbox_element* request,
if (request && requires_response(*request) if (request && requires_response(*request)
&& has_response_receiver(*request)) { && has_response_receiver(*request)) {
state tmp; state tmp;
tmp.self = self; tmp.weak_self = self->ctrl();
tmp.source.swap(request->sender); tmp.source.swap(request->sender);
tmp.stages.swap(request->stages); tmp.stages.swap(request->stages);
tmp.id = request->mid; tmp.id = request->mid;
...@@ -148,18 +148,28 @@ void response_promise::respond_to(local_actor* self, mailbox_element* request, ...@@ -148,18 +148,28 @@ void response_promise::respond_to(local_actor* self, mailbox_element* request,
// -- state -------------------------------------------------------------------- // -- state --------------------------------------------------------------------
response_promise::state::~state() { response_promise::state::~state() {
if (self) { // Note: the state may get destroyed outside of the actor. For example, when
// storing the promise in a run-later continuation. Hence, we can't call
// deliver_impl here since it calls self->context().
if (weak_self && source) {
CAF_LOG_DEBUG("broken promise!"); CAF_LOG_DEBUG("broken promise!");
deliver_impl(make_message(make_error(sec::broken_promise))); auto element = make_mailbox_element(weak_self.lock(), id.response_id(),
no_stages,
make_error(sec::broken_promise));
source->enqueue(std::move(element), nullptr);
} }
} }
void response_promise::state::cancel() { void response_promise::state::cancel() {
self = nullptr; weak_self = nullptr;
} }
void response_promise::state::deliver_impl(message msg) { void response_promise::state::deliver_impl(message msg) {
CAF_LOG_TRACE(CAF_ARG(msg)); CAF_LOG_TRACE(CAF_ARG(msg));
// Even though we are holding a weak pointer, we can access the pointer
// without any additional check here because only the actor itself is allowed
// to call this function.
auto self = static_cast<local_actor*>(weak_self.get()->get());
if (msg.empty() && id.is_async()) { if (msg.empty() && id.is_async()) {
CAF_LOG_DEBUG("drop response: empty response to asynchronous input"); CAF_LOG_DEBUG("drop response: empty response to asynchronous input");
} else if (!stages.empty()) { } else if (!stages.empty()) {
...@@ -178,6 +188,7 @@ void response_promise::state::delegate_impl(abstract_actor* receiver, ...@@ -178,6 +188,7 @@ void response_promise::state::delegate_impl(abstract_actor* receiver,
message msg) { message msg) {
CAF_LOG_TRACE(CAF_ARG(msg)); CAF_LOG_TRACE(CAF_ARG(msg));
if (receiver != nullptr) { if (receiver != nullptr) {
auto self = static_cast<local_actor*>(weak_self.get()->get());
detail::profiled_send(self, std::move(source), receiver, id, detail::profiled_send(self, std::move(source), receiver, id,
std::move(stages), self->context(), std::move(msg)); std::move(stages), self->context(), std::move(msg));
} else { } else {
......
...@@ -9,6 +9,7 @@ ...@@ -9,6 +9,7 @@
#include "core-test.hpp" #include "core-test.hpp"
using namespace caf; using namespace caf;
using namespace std::literals;
namespace { namespace {
...@@ -170,3 +171,25 @@ SCENARIO("response promises allow delegation") { ...@@ -170,3 +171,25 @@ SCENARIO("response promises allow delegation") {
} }
END_FIXTURE_SCOPE() END_FIXTURE_SCOPE()
CAF_TEST("GH-1306 regression") {
actor_system_config cfg;
cfg.set("caf.scheduler.max-threads", 1u);
actor_system sys{cfg};
auto aut = sys.spawn([](caf::event_based_actor* self) -> behavior {
return {
[self](int32_t x) {
auto rp = self->make_response_promise();
self->run_delayed(1h, [rp, x]() mutable { rp.deliver(x + x); });
return rp;
},
};
});
scoped_actor self{sys};
self->send(aut, 21);
self->send_exit(aut, exit_reason::kill);
aut = nullptr;
// Destroying the now obsolete action now destroys the promise. If the promise
// access the self pointer this triggers a heap-use-after-free since the AUT
// has been destroyed at this point.
}
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