Skip to content
This repository was archived by the owner on Mar 22, 2023. It is now read-only.

Commit 6300980

Browse files
committed
radix_tree: fix upper_bound regression
If the tree contains only a single element smaller than the looked-for key, radix_tree tried to call make_iterator on nullptr. This resulted in segfaults in pmemkv tests. Bug found with the help of UBSAN.
1 parent 8e5eec1 commit 6300980

File tree

2 files changed

+73
-19
lines changed

2 files changed

+73
-19
lines changed

include/libpmemobj++/experimental/radix_tree.hpp

Lines changed: 29 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2307,12 +2307,12 @@ radix_tree<Key, Value, BytesView, MtMode>::internal_bound(const K &k) const
23072307
auto slot = node_d.slot;
23082308
auto prev = node_d.prev;
23092309

2310-
/*
2311-
* n would point to element with key which we are looking for
2312-
* (if it existed). All elements on the left are smaller and all
2313-
* element on the right are bigger.
2314-
*/
23152310
if (!n) {
2311+
/*
2312+
* n would point to element with key which we are
2313+
* looking for (if it existed). All elements on the left
2314+
* are smaller and all element on the right are bigger.
2315+
*/
23162316
assert(prev && !is_leaf(prev));
23172317

23182318
auto target_leaf = next_leaf<node::direction::Forward>(
@@ -2341,22 +2341,31 @@ radix_tree<Key, Value, BytesView, MtMode>::internal_bound(const K &k) const
23412341
} else if (diff == leaf_key.size()) {
23422342
assert(n == leaf);
23432343

2344-
/* Leaf's key is a prefix of the looked-for key. Leaf's
2345-
* key is the biggest key less than the looked-for key.
2346-
* The target node must be the next leaf. Note that we
2347-
* cannot just call const_iterator(leaf,
2348-
* this).try_increment() because some other element with
2349-
* key larger than leaf and smaller than k could be
2350-
* inserted concurrently. */
2351-
auto target_leaf = next_leaf<node::direction::Forward>(
2352-
prev->template make_iterator<
2353-
node::direction::Forward>(slot),
2354-
prev);
2344+
if (!prev) {
2345+
/* There is only one element in the tree and
2346+
* it's smaller. */
2347+
result = cend();
2348+
} else {
2349+
/* Leaf's key is a prefix of the looked-for key.
2350+
* Leaf's key is the biggest key less than the
2351+
* looked-for key. The target node must be the
2352+
* next leaf. Note that we cannot just call
2353+
* const_iterator(leaf, this).try_increment()
2354+
* because some other element with key larger
2355+
* than leaf and smaller than k could be
2356+
* inserted concurrently. */
2357+
auto target_leaf = next_leaf<
2358+
node::direction::Forward>(
2359+
prev->template make_iterator<
2360+
node::direction::Forward>(slot),
2361+
prev);
23552362

2356-
if (!target_leaf.first)
2357-
continue;
2363+
if (!target_leaf.first)
2364+
continue;
23582365

2359-
result = const_iterator(target_leaf.second, this);
2366+
result = const_iterator(target_leaf.second,
2367+
this);
2368+
}
23602369
} else {
23612370
assert(diff < leaf_key.size() && diff < key.size());
23622371

@@ -2396,6 +2405,7 @@ radix_tree<Key, Value, BytesView, MtMode>::internal_bound(const K &k) const
23962405
} else if (slot == &root) {
23972406
result = const_iterator(nullptr, this);
23982407
} else {
2408+
assert(prev);
23992409
assert(static_cast<unsigned char>(key[diff]) >
24002410
static_cast<unsigned char>(
24012411
leaf_key[diff]));

tests/radix_tree/radix_basic.cpp

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -435,6 +435,50 @@ test_find(nvobj::pool<root> &pop)
435435

436436
UT_ASSERTeq(num_allocs(pop), 0);
437437
}
438+
439+
/* Upper bound when there are multiple less elements with common prefix
440+
*/
441+
{
442+
nvobj::transaction::run(pop, [&] {
443+
r->radix_str =
444+
nvobj::make_persistent<container_string>();
445+
});
446+
447+
r->radix_str->try_emplace("in1", "");
448+
r->radix_str->try_emplace("in2", "");
449+
r->radix_str->try_emplace("in3", "");
450+
r->radix_str->try_emplace("in4", "");
451+
452+
auto ub = r->radix_str->upper_bound("in6");
453+
UT_ASSERT(ub == r->radix_str->end());
454+
455+
nvobj::transaction::run(pop, [&] {
456+
nvobj::delete_persistent<container_string>(
457+
r->radix_str);
458+
});
459+
460+
UT_ASSERTeq(num_allocs(pop), 0);
461+
}
462+
463+
/* Upper bound when there is single, less element */
464+
{
465+
nvobj::transaction::run(pop, [&] {
466+
r->radix_str =
467+
nvobj::make_persistent<container_string>();
468+
});
469+
470+
r->radix_str->try_emplace("in", "");
471+
472+
auto ub = r->radix_str->upper_bound("inA");
473+
UT_ASSERT(ub == r->radix_str->end());
474+
475+
nvobj::transaction::run(pop, [&] {
476+
nvobj::delete_persistent<container_string>(
477+
r->radix_str);
478+
});
479+
480+
UT_ASSERTeq(num_allocs(pop), 0);
481+
}
438482
}
439483

440484
const auto compressed_path_len = 4;

0 commit comments

Comments
 (0)