From 1623b6a71fb59d7fc09e87f35286d7ccfa9034e9 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Thu, 19 Jan 2023 18:38:07 +0300 Subject: [PATCH 1/2] Ticker callback should be allowed to be destroyed Mainly, we protect std::function and variant itself from undefined behaviour when the underlying object is destroyed and then re-created with something else inside of it. For example, every captured object would be destroyed. ```cpp Ticker ticker; void oops(Job job) { ticker.once(1.0f, [job = std::move(job)]() { ticker.once(job.next(), do_something_else); job.work(); // ~Job() just happened! }); } ``` Another important case is repeating through once() ```cpp Ticker ticker; void repeat() { ticker.once(1.0f, repeat); puts("tick"); } void setup() { repeat(); } ``` Temporarily moving callback object into the function and returning earlier should solve both cases. --- libraries/Ticker/src/Ticker.cpp | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/libraries/Ticker/src/Ticker.cpp b/libraries/Ticker/src/Ticker.cpp index baac355ca5..9eca8512c8 100644 --- a/libraries/Ticker/src/Ticker.cpp +++ b/libraries/Ticker/src/Ticker.cpp @@ -115,6 +115,10 @@ void Ticker::_static_callback() } } + // it is technically allowed to call either schedule or detach + // *during* callback execution. allow both to happen + auto tmp = std::move(_callback); + std::visit([](auto&& callback) { using T = std::decay_t; if constexpr (std::is_same_v) { @@ -122,7 +126,16 @@ void Ticker::_static_callback() } else if constexpr (std::is_same_v) { callback(); } - }, _callback); + }, tmp); + + // ...and move ourselves back only when object is in a valid state + // * ticker was not detached, zeroing timer pointer + // * nothing else replaced callback variant + if ((_timer == nullptr) || !std::holds_alternative(_callback)) { + return; + } + + _callback = std::move(tmp); if (_repeat) { if (_tick) { From 1261b28c8a2acc213f1fe536945a66db5fd171d6 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Fri, 20 Jan 2023 20:33:07 +0300 Subject: [PATCH 2/2] words --- libraries/Ticker/src/Ticker.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/libraries/Ticker/src/Ticker.cpp b/libraries/Ticker/src/Ticker.cpp index 9eca8512c8..8c45ffed8b 100644 --- a/libraries/Ticker/src/Ticker.cpp +++ b/libraries/Ticker/src/Ticker.cpp @@ -117,7 +117,8 @@ void Ticker::_static_callback() // it is technically allowed to call either schedule or detach // *during* callback execution. allow both to happen - auto tmp = std::move(_callback); + decltype(_callback) tmp; + std::swap(tmp, _callback); std::visit([](auto&& callback) { using T = std::decay_t; @@ -135,7 +136,7 @@ void Ticker::_static_callback() return; } - _callback = std::move(tmp); + std::swap(tmp, _callback); if (_repeat) { if (_tick) {