Skip to content

Commit

Permalink
Fix to use after move (#540)
Browse files Browse the repository at this point in the history 
There was a potential use-after-move of the executor in future<future<T>> reduction.
  • Loading branch information
@sean-parent
sean-parent authored Mar 11, 2024
1 parent facbd55 commit 50b7c8d
Show file tree
Hide file tree
Showing 3 changed files with 67 additions and 45 deletions.
2 changes: 1 addition & 1 deletion README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ cmake -S . -B ../BUILD -GNinja -DCMAKE_CXX_STANDARD=17 -DCMAKE_BUILD_TYPE=Releas
If you organize the build directory into subdirectories you can support multiple configurations.

```
cmake -S . -B ../builds/portable -GXcode -DCMAKE_CXX_STANDARD=17 -DBUILD_TESTING=ON -DSTLAB_TASK_SYSTEM=portable
cmake -S . -B ../builds/portable -GXcode -DCMAKE_CXX_STANDARD=17 -DBUILD_TESTING=ON -DSTLAB_TASK_SYSTEM=portable -DCMAKE_OSX_DEPLOYMENT_TARGET=14.4 -DCMAKE_OSX_DEPLOYMENT_TARGET=macosx14.4
```

### Build
Expand Down
74 changes: 54 additions & 20 deletions stlab/concurrency/future.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,15 @@ using packaged_task_from_signature_t = typename packaged_task_from_signature<T>:

/**************************************************************************************************/

template <class T>
struct will_reduce : std::false_type {};

template <class T>
struct will_reduce<future<T>> : std::true_type {};

template <class T>
inline constexpr bool will_reduce_v = will_reduce<T>::value;

template <typename>
struct reduced_;

Expand Down Expand Up @@ -272,22 +281,31 @@ struct shared_base<T, enable_if_copyable<T>> : std::enable_shared_from_this<shar
return recover(std::move(p), _executor, std::forward<F>(f));
}

/*
NOTE : executor cannot be a reference type here. When invoked it could
cause _this_ to be deleted, and the executor passed in may be
this->_executor
*/
template <typename E, typename F>
auto recover(future<T>&& p, E executor, F&& f) {
auto b = package<detail::result_t<F, future<T>>()>(
executor, [_f = std::forward<F>(f), _p = std::move(p)]() mutable {
return std::move(_f)(std::move(_p));
});
using result_type = detail::result_t<F, future<T>>;
constexpr bool will_reduce = detail::will_reduce_v<result_type>;

auto b = package<result_type()>(executor,
[_f = std::forward<F>(f), _p = std::move(p)]() mutable {
return std::move(_f)(std::move(_p));
});

bool ready;
{
std::unique_lock<std::mutex> lock(_mutex);
ready = _ready;
if (!ready) _then.emplace_back(std::move(executor), std::move(b.first));
if (!ready) _then.emplace_back(move_if<!will_reduce>(executor), std::move(b.first));
}
if (ready) executor(std::move(b.first));

return detail::reduce(executor, std::move(b.second));
if (ready) executor(std::move(b.first)); // cannot reference this after here

return detail::reduce(move_if<will_reduce>(executor), std::move(b.second));
}

void _detach() {
Expand Down Expand Up @@ -378,22 +396,30 @@ struct shared_base<T, enable_if_not_copyable<T>> : std::enable_shared_from_this<
return recover(std::move(p), _executor, std::forward<F>(f));
}

/*
NOTE : executor cannot be a reference type here. When invoked it could
cause _this_ to be deleted, and the executor passed in may be
this->_executor
*/
template <typename E, typename F>
auto recover(future<T>&& p, E executor, F&& f) {
auto b = package<detail::result_t<F, future<T>>()>(
executor, [_f = std::forward<F>(f), _p = std::move(p)]() mutable {
return std::move(_f)(std::move(_p));
});
using result_type = detail::result_t<F, future<T>>;
constexpr bool will_reduce = detail::will_reduce_v<result_type>;

auto b = package<result_type()>(executor,
[_f = std::forward<F>(f), _p = std::move(p)]() mutable {
return std::move(_f)(std::move(_p));
});

bool ready;
{
std::unique_lock<std::mutex> lock(_mutex);
ready = _ready;
if (!ready) _then = {std::move(executor), std::move(b.first)};
if (!ready) _then = {move_if<!will_reduce>(executor), std::move(b.first)};
}
if (ready) executor(std::move(b.first));
if (ready) executor(std::move(b.first)); // cannot reference this after here

return detail::reduce(executor, std::move(b.second));
return detail::reduce(move_if<will_reduce>(executor), std::move(b.second));
}

void _detach() {
Expand Down Expand Up @@ -1666,7 +1692,7 @@ auto async(E executor, F&& f, Args&&... args)

executor(std::move(p.first));

return detail::reduce(executor, std::move(p.second));
return detail::reduce(std::move(executor), std::move(p.second));
}

/**************************************************************************************************/
Expand Down Expand Up @@ -1758,22 +1784,30 @@ void shared_base<void>::set_value(F& f, Args&&... args) {

/**************************************************************************************************/

/*
NOTE : executor cannot be a reference type here. When invoked it could
cause _this_ to be deleted, and the executor passed in may be
this->_executor
*/
template <typename E, typename F>
auto shared_base<void>::recover(future<result_type>&& p, E executor, F&& f) {
auto b = package<detail::result_t<F, future<result_type>>()>(
executor, [_f = std::forward<F>(f), _p = std::move(p)]() mutable {
using result_type = detail::result_t<F, future<result_type>>;
constexpr bool will_reduce = detail::will_reduce_v<result_type>;

auto b =
package<result_type()>(executor, [_f = std::forward<F>(f), _p = std::move(p)]() mutable {
return std::move(_f)(std::move(_p));
});

bool ready;
{
std::unique_lock<std::mutex> lock(_mutex);
ready = _ready;
if (!ready) _then.emplace_back(std::move(executor), std::move(b.first));
if (!ready) _then.emplace_back(move_if<!will_reduce>(executor), std::move(b.first));
}
if (ready) executor(std::move(b.first));
if (ready) executor(std::move(b.first)); // cannot reference this after here

return detail::reduce(executor, std::move(b.second));
return detail::reduce(move_if<will_reduce>(executor), std::move(b.second));
}

/**************************************************************************************************/
Expand Down
36 changes: 12 additions & 24 deletions test/future_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,7 @@ BOOST_AUTO_TEST_CASE(async_lambda_arguments) {
BOOST_TEST_MESSAGE("running async lambda argument of type rvalue -> value");

annotate_counters counters;
(void)async(
immediate_executor, [](annotate) {}, annotate(counters));
(void)async(immediate_executor, [](annotate) {}, annotate(counters));
BOOST_REQUIRE(counters.remaining() == 0);
BOOST_REQUIRE(counters._copy_ctor == 0);
}
Expand All @@ -58,8 +57,7 @@ BOOST_AUTO_TEST_CASE(async_lambda_arguments) {

annotate_counters counters;
annotate x(counters);
(void)async(
immediate_executor, [](annotate) {}, x);
(void)async(immediate_executor, [](annotate) {}, x);
BOOST_REQUIRE(counters.remaining() == 1);
BOOST_REQUIRE(counters._copy_ctor == 1);
}
Expand All @@ -69,8 +67,7 @@ BOOST_AUTO_TEST_CASE(async_lambda_arguments) {

annotate_counters counters;
annotate x(counters);
(void)async(
immediate_executor, [](annotate) {}, std::ref(x));
(void)async(immediate_executor, [](annotate) {}, std::ref(x));
BOOST_REQUIRE(counters.remaining() == 1);
BOOST_REQUIRE(counters._copy_ctor == 1);
}
Expand All @@ -80,8 +77,7 @@ BOOST_AUTO_TEST_CASE(async_lambda_arguments) {

annotate_counters counters;
annotate x(counters);
(void)async(
immediate_executor, [](annotate) {}, std::cref(x));
(void)async(immediate_executor, [](annotate) {}, std::cref(x));
BOOST_REQUIRE(counters.remaining() == 1);
BOOST_REQUIRE(counters._copy_ctor == 1);
}
Expand Down Expand Up @@ -113,8 +109,7 @@ BOOST_AUTO_TEST_CASE(async_lambda_arguments) {

annotate_counters counters;
annotate x(counters);
(void)async(
immediate_executor, [](annotate&) {}, std::ref(x));
(void)async(immediate_executor, [](annotate&) {}, std::ref(x));
BOOST_REQUIRE(counters.remaining() == 1);
BOOST_REQUIRE(counters._copy_ctor == 0);
}
Expand All @@ -136,8 +131,7 @@ BOOST_AUTO_TEST_CASE(async_lambda_arguments) {
BOOST_TEST_MESSAGE("running async lambda argument of type rvalue -> const&");

annotate_counters counters;
(void)async(
immediate_executor, [](const annotate&) {}, annotate(counters));
(void)async(immediate_executor, [](const annotate&) {}, annotate(counters));
BOOST_REQUIRE(counters.remaining() == 0);
BOOST_REQUIRE(counters._copy_ctor == 0);
}
Expand All @@ -147,8 +141,7 @@ BOOST_AUTO_TEST_CASE(async_lambda_arguments) {

annotate_counters counters;
annotate x(counters);
(void)async(
immediate_executor, [](const annotate&) {}, x);
(void)async(immediate_executor, [](const annotate&) {}, x);
BOOST_REQUIRE(counters.remaining() == 1);
BOOST_REQUIRE(counters._copy_ctor == 1);
}
Expand All @@ -158,8 +151,7 @@ BOOST_AUTO_TEST_CASE(async_lambda_arguments) {

annotate_counters counters;
annotate x(counters);
(void)async(
immediate_executor, [](const annotate&) {}, std::ref(x));
(void)async(immediate_executor, [](const annotate&) {}, std::ref(x));
BOOST_REQUIRE(counters.remaining() == 1);
BOOST_REQUIRE(counters._copy_ctor == 0);
}
Expand All @@ -169,8 +161,7 @@ BOOST_AUTO_TEST_CASE(async_lambda_arguments) {

annotate_counters counters;
annotate x(counters);
(void)async(
immediate_executor, [](const annotate&) {}, std::cref(x));
(void)async(immediate_executor, [](const annotate&) {}, std::cref(x));
BOOST_REQUIRE(counters.remaining() == 1);
BOOST_REQUIRE(counters._copy_ctor == 0);
}
Expand All @@ -179,8 +170,7 @@ BOOST_AUTO_TEST_CASE(async_lambda_arguments) {
BOOST_TEST_MESSAGE("running async lambda argument of type rvalue -> &&");

annotate_counters counters;
(void)async(
immediate_executor, [](annotate&&) {}, annotate(counters));
(void)async(immediate_executor, [](annotate&&) {}, annotate(counters));
BOOST_REQUIRE(counters.remaining() == 0);
BOOST_REQUIRE(counters._copy_ctor == 0);
}
Expand All @@ -190,8 +180,7 @@ BOOST_AUTO_TEST_CASE(async_lambda_arguments) {

annotate_counters counters;
annotate x(counters);
(void)async(
immediate_executor, [](annotate&&) {}, x);
(void)async(immediate_executor, [](annotate&&) {}, x);
BOOST_REQUIRE(counters.remaining() == 1);
BOOST_REQUIRE(counters._copy_ctor == 1);
}
Expand Down Expand Up @@ -258,8 +247,7 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(future_constructed_minimal_fn_with_parameters,

test_setup setup;
{
auto sut = async(
make_executor<0>(), [](auto x) -> T { return x + T(0); }, T(42));
auto sut = async(make_executor<0>(), [](auto x) -> T { return x + T(0); }, T(42));
BOOST_REQUIRE(sut.valid() == true);
BOOST_REQUIRE(!sut.exception());

Expand Down

0 comments on commit 50b7c8d

Please sign in to comment.