From f7b5123e3fe74dc37f3aed0eb571181efb46dfa9 Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Fri, 25 Sep 2026 12:35:15 -0500 Subject: [PATCH] fix(chain_plugin): return empty for an inverted reverse secondary range The reverse secondary-index scan starts at the upper bound and walks down to begin, the lower bound's position. With upper_bound below lower_bound it started below begin, so its first step down decremented the index's begin() whenever the table's partition is the first in the whole secondary index: undefined behavior, reachable by any API caller against the contract whose account sorts first. Start at begin instead, so an inverted range is empty, as it already is forward and on the primary index. --- plugins/chain_plugin/src/chain_plugin.cpp | 12 ++++++--- tests/get_table_tests.cpp | 30 +++++++++++++++++++++++ 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/plugins/chain_plugin/src/chain_plugin.cpp b/plugins/chain_plugin/src/chain_plugin.cpp index 0ee372f3f2..21f346a203 100644 --- a/plugins/chain_plugin/src/chain_plugin.cpp +++ b/plugins/chain_plugin/src/chain_plugin.cpp @@ -2862,13 +2862,17 @@ read_only::get_table_rows( const read_only::get_table_rows_params& p, const fc:: // next page yields the first row strictly below the last returned // one -- i.e. the first unseen row. Setting `next_key` to the first // unseen row's sk instead would skip that row at every page boundary. + auto begin = sec_idx.lower_bound(boost::make_tuple(p.code, sec_tid, lb_sv)); decltype(sec_idx.end()) itr; - if (has_upper) { - itr = sec_idx.lower_bound(boost::make_tuple(p.code, sec_tid, ub_sv)); - } else { + if (!has_upper) { itr = sec_idx.upper_bound(boost::make_tuple(p.code, sec_tid)); + } else if (ub_sv < lb_sv) { + // An inverted range is empty. Seeking the upper bound would start `itr` below `begin`, and the walk down + // could then step off the front of the index. + itr = begin; + } else { + itr = sec_idx.lower_bound(boost::make_tuple(p.code, sec_tid, ub_sv)); } - auto begin = sec_idx.lower_bound(boost::make_tuple(p.code, sec_tid, lb_sv)); uint32_t count = 0; // Remember the last-returned row's sec_key bytes so the cutoff // branches below can feed them to `emit_secondary_next_key`. diff --git a/tests/get_table_tests.cpp b/tests/get_table_tests.cpp index 7eb8e6956d..5902143716 100644 --- a/tests/get_table_tests.cpp +++ b/tests/get_table_tests.cpp @@ -1818,6 +1818,36 @@ BOOST_FIXTURE_TEST_CASE( get_kv_rows_reverse_pagination_secondary_hex_test, vali } FC_LOG_AND_RETHROW() +// An inverted range (upper_bound below lower_bound) is empty on every path. The reverse secondary scan walks down from +// the upper bound to the lower one, so an inverted range used to start it below its stop; when that table's partition +// comes first in the whole secondary index, the first step went off the front of the index. The account sorts before +// every other code, so its partition does come first. +BOOST_FIXTURE_TEST_CASE( get_kv_rows_inverted_range_test, validating_tester ) try { + setup_secrev(*this, "1secrev"_n); + + std::optional _tracked_votes; + chain_apis::read_only plugin(*(this->control), {}, {}, _tracked_votes, + fc::microseconds::maximum(), fc::microseconds::maximum(), {}); + + for (const std::string index : {"", "byowner"}) { + for (const bool reverse : {false, true}) { + BOOST_TEST_CONTEXT("index='" << index << "' reverse=" << reverse) { + chain_apis::read_only::get_table_rows_params p; + p.code = "1secrev"_n; + p.table = "users"; + p.index_name = index; + p.reverse = reverse; + p.lower_bound = index.empty() ? R"({"id":4})" : R"({"byowner":"u4"})"; + p.upper_bound = index.empty() ? R"({"id":1})" : R"({"byowner":"u1"})"; + + auto page = get_table_rows_kv(plugin, p, fc::time_point::maximum()); + BOOST_CHECK(page.rows.empty()); + BOOST_CHECK(!page.more); + } + } + } +} FC_LOG_AND_RETHROW() + // Test get_table_rows with index_name parameter — full secondary index query BOOST_FIXTURE_TEST_CASE( get_kv_rows_index_name_test, validating_tester ) try { produce_block();