Skip to content

Commit

Permalink
core: fix a race in execution stages
Browse files Browse the repository at this point in the history
Execution stage has a threshold value - max_queue_length - after which
a flush will be forced. The flush procedure takes elements from the front
of the queue, processes them and returns. However, the exact order is:
1. Take a reference to the front element of the queue.
2. Launch the queued function.
3. Pop the element from the queue.

Unfortunately, if the applied function (2.) is inlined as ready and it also
triggers a flush (e.g. because the queue is full!), it will enter the
loop again and go to step 1., which will effectively take an element
from the top, which is a moved-from value, which leads to undefined behaviour.

Fix by moving the values and popping the queue front first and then proceeding,
which effectively changes the order above to 1->3->2, thus eliding the race.

Tested against an instantiation of this bug in ScyllaDB's issue
scylladb/scylladb#4856

Message-Id: <d2af4ecf56f51ee5967015e86e172a570d2a5336.1566311882.git.sarna@scylladb.com>
  • Loading branch information
psarna authored and avikivity committed Aug 20, 2019
1 parent b5cac2b commit 4e2e66e
Showing 1 changed file with 3 additions and 1 deletion.
4 changes: 3 additions & 1 deletion include/seastar/core/execution_stage.hh
Original file line number Diff line number Diff line change
Expand Up @@ -241,8 +241,10 @@ private:
virtual void do_flush() noexcept override {
while (!_queue.empty()) {
auto& wi = _queue.front();
futurize<ReturnType>::apply(_function, unwrap(std::move(wi._in))).forward_to(std::move(wi._ready));
auto wi_in = std::move(wi._in);
auto wi_ready = std::move(wi._ready);
_queue.pop_front();
futurize<ReturnType>::apply(_function, unwrap(std::move(wi_in))).forward_to(std::move(wi_ready));
_stats.function_calls_executed++;

if (need_preempt()) {
Expand Down

0 comments on commit 4e2e66e

Please sign in to comment.