Skip to content

Commit

Permalink
Storage proxy: protect against infinite recursion in query_partition_…
Browse files Browse the repository at this point in the history
…key_range_concurrent

A recent fix to #3767 limited the amount of ranges that
can return from query_ranges_to_vnodes_generator. This with
the combination of a large amount of token ranges can lead to
an infinite recursion. The algorithm multiplies by factor of
2 (actualy a shift left by one)  the amount of requested
tokens in each recursion iteration. As long as the requested
number of ranges is greater than 0, the recursion is implicit,
and each call is scheduled separately since the call is inside
a continuation of a map reduce.
But if the amount of iterations is large enough (~32) the
counter for requested ranges zeros out and from that moment on
two things will happen:
1. The counter will remain 0 forever (0*2 == 0)
2. The map reduce future will be immediately available and this
will result in the continuation being invoked immediately.
The latter causes the recursive call to be a "regular" recursive call
thus, through the stack and not the task queue of the scheduler, and
the former causes this recursion to be infinite.
The combination creates a stack that keeps growing and eventually
overflows resulting in undefined behavior (due to memory overrun).

This patch prevent the problem from happening, it limits the growth of
the concurrency counter beyond twice the last amount of tokens returned
by the query_ranges_to_vnodes_generator.And also makes sure it is not
get stuck at zero.

Testing: * Unit test in dev mode.
         * Modified add 50 dtest that reproduce the problem

Fixes #4944

Signed-off-by: Eliran Sinvani <eliransin@scylladb.com>
Message-Id: <20190922072838.14957-1-eliransin@scylladb.com>
(cherry picked from commit 280715a)
  • Loading branch information
Eliran Sinvani authored and avikivity committed Sep 22, 2019
1 parent d81ac93 commit 37c4be5
Showing 1 changed file with 6 additions and 0 deletions.
6 changes: 6 additions & 0 deletions service/storage_proxy.cc
Expand Up @@ -2930,6 +2930,12 @@ storage_proxy::query_partition_key_range_concurrent(storage_proxy::clock_type::t
dht::partition_range_vector ranges = ranges_to_vnodes(concurrency_factor);
dht::partition_range_vector::iterator i = ranges.begin();

// query_ranges_to_vnodes_generator can return less results than requested. If the number of results
// is small enough or there are a lot of results - concurrentcy_factor which is increased by shifting left can
// eventualy zero out resulting in an infinite recursion. This line makes sure that concurrency factor is never
// get stuck on 0 and never increased too much if the number of results remains small.
concurrency_factor = std::max(size_t(1), ranges.size());

while (i != ranges.end()) {
dht::partition_range& range = *i;
std::vector<gms::inet_address> live_endpoints = get_live_sorted_endpoints(ks, end_token(range));
Expand Down

0 comments on commit 37c4be5

Please sign in to comment.