From a7837aa3c45f8929b323d3f7b2fb2c28d956b76d Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Mon, 31 Aug 2026 11:08:34 -0500 Subject: [PATCH 01/28] fix(kv): reject duplicate primary keys in multi_index::emplace emplace could strand a secondary-index entry. It called kv_set -- an upsert -- and then store_secondaries, an unconditional kv_idx_store, without checking that the primary key was new. Emplacing over an existing key silently overwrote the row and left the previous (sec_key -> pri_key) mapping in place, pointing at a row whose secondary value had since changed, so a later get_index<>().find(old_sec) resolved to a row that did not match the key. The host is not at fault. kv_set is a documented upsert and kv_idx_store a documented insert; on Antelope the duplicate was rejected at the chain layer by db_store_i64, and that guard was lost when the legacy DB was removed. The shim never picked it up. kv::table::emplace already does exactly this check, so the backward-compatibility wrapper was the one missing what the perf-first wrapper already pays for. Demonstrated before fixing, against the real runtime: a throwaway action emplaced pk=1 with secondary "aaa", emplaced pk=1 again with "bbb", then looked up "aaa". Pre-fix it resolved -- the orphan. Post-fix the second emplace aborts and the orphan is unreachable. That probe is now the permanent regression test, asserting the abort. Also templates lower_bound/upper_bound on the primary key type, matching upstream multi_index, which routes through to_raw_key. Taking a bare uint64_t rejected `name` primary keys that compile upstream. Making that work exposed three more sites passing primary_key() straight into pk_to_bytes; they now use the same to_pk_uint64 conversion as every other call site. A name-primary-key test covers both bound forms and pins that the uint64_t form still binds. kv_table::do_insert becomes private. It is explicitly the unchecked path and was public only because of where the access block fell; it has one caller, in the same class, and nothing downstream references it. Deliberately no emplace_unchecked: the host indexes kv_index_object ordered_unique on (code, table_id, sec_key, pri_key), so skipping the check either strands a mapping or trips that constraint -- the hazard being removed here. Also broadens the CLion build-dir ignore to cmake-build-*/ and ignores prequel's local review state. 29/29 ctest including toolchain and integration suites. --- .gitignore | 6 +- .../contracts/sysio/kv_multi_index.hpp | 31 ++++++-- .../sysiolib/contracts/sysio/kv_table.hpp | 10 ++- tests/integration/multi_index_tests.cpp | 5 ++ .../unit/test_contracts/multi_index_tests.cpp | 73 +++++++++++++++++++ 5 files changed, 115 insertions(+), 10 deletions(-) diff --git a/.gitignore b/.gitignore index 010f945d7..d761dcdab 100644 --- a/.gitignore +++ b/.gitignore @@ -41,7 +41,8 @@ compile_commands.json [Bb]uild*/ .ccache/ .vcpkg-binary-cache/ -cmake-build-debug/ +# CLion-style build dirs: cmake-build-debug/, cmake-build-debug-vcpkg/, cmake-build-release/, ... +cmake-build-*/ examples/multi_index_example/build examples/hello/build @@ -67,3 +68,6 @@ tmp/ # oh-my-claudecode runtime state (operational artifacts, never committed) .omc/ + +# prequel local review state (operational artifacts, never committed) +.prequel/ diff --git a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp index 196bd9bf0..02f47b067 100644 --- a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp +++ b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp @@ -312,7 +312,7 @@ class kv_multi_index { using extractor_t = typename Index::secondary_extractor_type; extractor_t ext; auto sec_key = idx.encode_scoped_secondary(ext(obj)); - auto pri_key = idx.pk_to_bytes(obj.primary_key()); + auto pri_key = idx.pk_to_bytes(kv_multi_index::to_pk_uint64(obj.primary_key())); ::kv_idx_store(payer, _sec_tid, pri_key.data, _kv_multi_index_detail::u64_size, sec_key.data(), sec_key.size()); @@ -325,7 +325,7 @@ class kv_multi_index { using extractor_t = typename Index::secondary_extractor_type; extractor_t ext; auto sec_key = idx.encode_scoped_secondary(ext(obj)); - auto pri_key = idx.pk_to_bytes(obj.primary_key()); + auto pri_key = idx.pk_to_bytes(kv_multi_index::to_pk_uint64(obj.primary_key())); ::kv_idx_remove(_sec_tid, pri_key.data, _kv_multi_index_detail::u64_size, sec_key.data(), sec_key.size()); @@ -339,7 +339,7 @@ class kv_multi_index { extractor_t ext; auto old_sec = idx.encode_scoped_secondary(ext(old_obj)); auto new_sec = idx.encode_scoped_secondary(ext(new_obj)); - auto pri_key = idx.pk_to_bytes(old_obj.primary_key()); + auto pri_key = idx.pk_to_bytes(kv_multi_index::to_pk_uint64(old_obj.primary_key())); if (old_sec != new_sec) { ::kv_idx_update(payer, _sec_tid, pri_key.data, _kv_multi_index_detail::u64_size, @@ -592,17 +592,24 @@ class kv_multi_index { return *obj; } - const_iterator lower_bound(uint64_t primary) const { - auto key = make_pk(primary); + /// Templated on the primary key type, matching upstream multi_index, which routes + /// through to_raw_key. Taking a bare uint64_t here rejected the `name` primary keys + /// that compile fine upstream. to_pk_uint64 is the same conversion the rest of this + /// class uses, so a uint64_t argument still binds exactly as before. + template + const_iterator lower_bound(PK primary) const { + auto key = make_pk(to_pk_uint64(primary)); auto prefix = make_prefix(); uint32_t handle = ::kv_it_create(_table_id, _code.value, prefix.data, prefix_size); int32_t status = ::kv_it_lower_bound(handle, key.data, key_size); return const_iterator(this, handle, status == 0); } - const_iterator upper_bound(uint64_t primary) const { - if (primary == std::numeric_limits::max()) return end(); - return lower_bound(primary + 1); + template + const_iterator upper_bound(PK primary) const { + const uint64_t pk = to_pk_uint64(primary); + if (pk == std::numeric_limits::max()) return end(); + return lower_bound(pk + 1); } const_iterator iterator_to(const T& obj) const { @@ -627,6 +634,14 @@ class kv_multi_index { auto key = make_pk(pk); auto value = serialize_row(obj); + // Reject a duplicate primary key, as db_store_i64 did on Antelope. That guard lived + // at the chain layer and was lost with the legacy DB: kv_set is an upsert, so without + // this the row is silently overwritten AND store_secondaries -- an unconditional + // kv_idx_store -- leaves the old (sec_key -> pri_key) mapping behind, pointing at a + // row whose secondary value has changed. kv::table::emplace checks the same way. + sysio::check(!::kv_contains(_table_id, _code.value, key.data, key_size), + "object with the same primary key already exists"); + ::kv_set(_table_id, payer.value, key.data, key_size, value.data(), value.size()); store_secondaries(payer.value, obj); diff --git a/libraries/sysiolib/contracts/sysio/kv_table.hpp b/libraries/sysiolib/contracts/sysio/kv_table.hpp index 74a1bcd27..af47a0e5e 100644 --- a/libraries/sysiolib/contracts/sysio/kv_table.hpp +++ b/libraries/sysiolib/contracts/sysio/kv_table.hpp @@ -435,7 +435,14 @@ class table_impl { sec_ops::update_all(*this, payer, pri.data(), pri.size(), old_val, new_val); } - // Internal insert (no duplicate check — caller must verify) +private: + // Internal insert (no duplicate check — caller must verify). + // + // Private deliberately. Inserting over an existing key here silently overwrites the row + // and, because store_secondaries is an unconditional kv_idx_store, either strands the old + // (sec_key -> pri_key) mapping (when the secondary value changed) or trips the host's + // ordered_unique constraint on (code, table_id, sec_key, pri_key). emplace() pays one + // kv_contains to make that unreachable; there is no supported way to skip it. void do_insert(uint64_t payer, const be_key_stream& k, const K& key, const V& value) { if constexpr (is_fixed_serializable_v) { char vbuf[sizeof(V)]; @@ -448,6 +455,7 @@ class table_impl { store_secondaries(payer, key, value); } +public: // Internal erase used by both primary and secondary erase paths void do_erase(const K& key, const V& value) { remove_secondaries(key, value); diff --git a/tests/integration/multi_index_tests.cpp b/tests/integration/multi_index_tests.cpp index d31d3f81f..8556aa82b 100644 --- a/tests/integration/multi_index_tests.cpp +++ b/tests/integration/multi_index_tests.cpp @@ -35,6 +35,11 @@ BOOST_FIXTURE_TEST_CASE(main_multi_index_tests, TESTER) { try { }; push_action( "testapi"_n, "s1g"_n, "testapi"_n, {} ); // idx64_general + push_action( "testapi"_n, "s1namepk"_n, "testapi"_n, {} ); // name_pk_bounds + + // Duplicate primary key aborts instead of upserting. Without the guard this action + // succeeded and left the previous secondary mapping stranded. + check_failure( "s1dupidx"_n, "object with the same primary key already exists" ); push_action( "testapi"_n, "s1store"_n, "testapi"_n, {} ); // idx64_store_only push_action( "testapi"_n, "s1check"_n, "testapi"_n, {} ); // idx64_check_without_storing push_action( "testapi"_n, "s2g"_n, "testapi"_n, {} ); // idx128_general diff --git a/tests/unit/test_contracts/multi_index_tests.cpp b/tests/unit/test_contracts/multi_index_tests.cpp index c76360ecd..d91946d54 100644 --- a/tests/unit/test_contracts/multi_index_tests.cpp +++ b/tests/unit/test_contracts/multi_index_tests.cpp @@ -342,6 +342,71 @@ namespace _test_multi_index return table; } + // Duplicate primary key must be rejected. + // + // On Antelope the guard was db_store_i64's, at the chain layer, and it was lost when + // the legacy DB was removed. kv_set is an upsert, so without an explicit check the row + // is silently overwritten and store_secondaries -- an unconditional kv_idx_store -- + // strands the previous (sec_key -> pri_key) mapping. Verified against the real runtime: + // before the guard, a lookup of the OLD secondary value still resolved to this row + // after it had been overwritten with a new one. + template + void idx64_duplicate_emplace(sysio::name receiver) + { + typedef record_idx64 record; + sysio::kv_multi_index>> + table(receiver, receiver.value); + auto payer = receiver; + + table.emplace(payer, [&](auto& r) { r.id = 1; r.sec = "aaa"_n.value; }); + + // Changing the secondary value is what made the stale mapping observable. + table.emplace(payer, [&](auto& r) { r.id = 1; r.sec = "bbb"_n.value; }); + } + + // A `name` primary key exercises the templated lower_bound/upper_bound. These took a + // bare uint64_t, so this did not compile, while upstream multi_index accepts it via + // to_raw_key. + struct record_name_pk + { + sysio::name owner; + uint64_t sec; + + sysio::name primary_key() const { return owner; } + uint64_t get_secondary() const { return sec; } + + SYSLIB_SERIALIZE(record_name_pk, (owner)(sec)) + }; + + template + void name_pk_bounds(sysio::name receiver) + { + typedef record_name_pk record; + sysio::kv_multi_index>> + table(receiver, receiver.value); + auto payer = receiver; + + table.emplace(payer, [&](auto& r) { r.owner = "alice"_n; r.sec = 10; }); + table.emplace(payer, [&](auto& r) { r.owner = "bob"_n; r.sec = 20; }); + table.emplace(payer, [&](auto& r) { r.owner = "charlie"_n; r.sec = 30; }); + + // Passing a name, not a uint64_t. + auto lb = table.lower_bound("bob"_n); + sysio::check(lb != table.end() && lb->owner == "bob"_n, + "name_pk_bounds - lower_bound(name) did not land on bob"); + + auto ub = table.upper_bound("bob"_n); + sysio::check(ub != table.end() && ub->owner == "charlie"_n, + "name_pk_bounds - upper_bound(name) did not land on charlie"); + + // The uint64_t form must keep working unchanged. + auto lb_raw = table.lower_bound("bob"_n.value); + sysio::check(lb_raw != table.end() && lb_raw->owner == "bob"_n, + "name_pk_bounds - lower_bound(uint64_t) regressed"); + } + } /// _test_multi_index class [[sysio::contract]] test_multi_index : public sysio::contract @@ -354,6 +419,14 @@ class [[sysio::contract]] test_multi_index : public sysio::contract _test_multi_index::idx64_check_without_storing<"indextable2"_n.value>( get_self() ); } + [[sysio::action("s1namepk")]] void name_pk_bounds() { + _test_multi_index::name_pk_bounds<"namepktable"_n.value>(get_self()); + } + + [[sysio::action("s1dupidx")]] void idx64_duplicate_emplace() { + _test_multi_index::idx64_duplicate_emplace<"duptable1"_n.value>(get_self()); + } + [[sysio::action("s1store")]] void idx64_store_only() { _test_multi_index::idx64_store_only<"indextable1"_n.value>(get_self()); } From 4c4b81adf6031b80f1aafeb878384df7135818b1 Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Mon, 31 Aug 2026 15:34:40 -0500 Subject: [PATCH 02/28] fix(kv): guard multi_index mutation on the receiving account Reads honour the handle's code -- kv_get and kv_contains take a code argument -- but writes do not: kv_set, kv_erase and kv_idx_store have no such parameter and always land on the receiver. So the duplicate-key probe added in this PR could consult one account while the write landed on another, and a handle opened on a foreign account would pass the check and then upsert the receiver's row, leaving its old secondary mapping stranded. That is the corruption this PR set out to remove, reached a different way. The host cannot catch it. Writes take no code, so a contract can never reach another account's namespace and there is nothing for the host to reject; it sees two well-formed calls. table_id derives from the table NAME alone and the key is [scope][pk] with no account component, so the misdirected write lands on the receiver's own row of the same table name. Upstream guards this in the wrapper, and Wire had dropped all three checks. Restored with upstream's messages, since a ported contract may assert on them, at the three points the mutators funnel through -- emplace, modify(const T&) and erase(const T&) -- which covers the iterator overloads and the index-level modify/erase that delegate to them. emplace checks before running the constructor lambda, as upstream does. receiving_account() avoids the host call where it can. The generated dispatcher records the receiver in sysio_contract_name at the top of apply(), so that path is a plain global read; SYSIO_DISPATCH emits its own strong apply() and the native dispatch sets nothing, leaving it 0 -- not a valid account name, so a safe sentinel -- and there we pay the current_receiver intrinsic, as upstream always does. Not cached on the object: a contract may hold a static table, and the receiver differs between an action and a notification handler. The guards are deliberately NOT extended to kv::table, kv::scoped_table or kv::global. Those are new APIs with no upstream behaviour to honour, and kv::global already documents foreign-code handles as read-only by contract. kv::table carried the same hazard undocumented, so it gains the equivalent note. Primary lower_bound/upper_bound go back to concrete overloads on uint64_t plus a name forwarder. Templating them was not source-widening as this PR claimed: lower_bound({42}) cannot deduce from a braced list and &table_type::lower_bound cannot form a pointer to an undeduced template. Two overloads reach exactly what to_pk_uint64 accepts, without the break. New native kv_multi_index_tests covers all of it. The on-chain cases needed ENABLE_INTEGRATION_TESTS, which defaults OFF and is not enabled in CI, so the guard could have been deleted with required CI green. Registered in both tests/unit/CMakeLists.txt and tests/CMakeLists.txt -- missing the second builds the test but never runs it. Each guard case runs twice, once with the dispatcher global set and once with it 0, so both branches of receiving_account() are exercised rather than only the native fallback. Verified by removing the emplace guard: the native test fails with "expect_assert, no assert" on both branches, so it genuinely gates. 30/30 ctest; wire-sysio's contracts rebuild against this CDT with 187 of its own test cases green. --- .../contracts/sysio/kv_multi_index.hpp | 55 ++++-- .../sysiolib/contracts/sysio/kv_table.hpp | 11 ++ tests/CMakeLists.txt | 1 + tests/integration/multi_index_tests.cpp | 4 + tests/unit/CMakeLists.txt | 1 + tests/unit/kv_multi_index_tests.cpp | 181 ++++++++++++++++++ .../unit/test_contracts/multi_index_tests.cpp | 19 ++ 7 files changed, 260 insertions(+), 12 deletions(-) create mode 100644 tests/unit/kv_multi_index_tests.cpp diff --git a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp index 02f47b067..498f12e80 100644 --- a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp +++ b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include @@ -155,6 +156,20 @@ class kv_multi_index { static uint64_t to_pk_uint64(uint64_t pk) { return pk; } static uint64_t to_pk_uint64(name pk) { return pk.value; } + /// The receiving account, avoiding a host call where possible. + /// + /// The generated dispatcher stores the receiver in sysio_contract_name at the top of + /// apply() -- and apply() is re-entered per receiver, so it is correct under notification + /// too -- making this a plain global read. SYSIO_DISPATCH emits its own strong apply() and + /// the native dispatch sets nothing, leaving the global 0, which is not a valid account + /// name and so is a safe "unset" sentinel; there we pay the intrinsic, as upstream always + /// does. Deliberately not cached on the object: a contract may hold a `static` table, and + /// the receiver differs between the initial action and a notification handler. + static name receiving_account() { + const name ctx = current_context_contract(); + return ctx.value ? ctx : current_receiver(); + } + name _code; uint64_t _scope; mutable uint64_t _next_primary_key = 0; @@ -592,26 +607,30 @@ class kv_multi_index { return *obj; } - /// Templated on the primary key type, matching upstream multi_index, which routes - /// through to_raw_key. Taking a bare uint64_t here rejected the `name` primary keys - /// that compile fine upstream. to_pk_uint64 is the same conversion the rest of this - /// class uses, so a uint64_t argument still binds exactly as before. - template - const_iterator lower_bound(PK primary) const { - auto key = make_pk(to_pk_uint64(primary)); + /// Overloads rather than a template on the primary key type. + /// + /// Upstream templates these and routes through to_raw_key, but a member template is not a + /// drop-in for a concrete overload: `lower_bound({42})` cannot deduce from a braced list, + /// and `&table_type::lower_bound` cannot form a pointer to an undeduced template. Both + /// compile against a plain uint64_t parameter. Two overloads cover exactly the types + /// to_pk_uint64 accepts, which is the same reach the template had, without the break. + const_iterator lower_bound(uint64_t primary) const { + auto key = make_pk(primary); auto prefix = make_prefix(); uint32_t handle = ::kv_it_create(_table_id, _code.value, prefix.data, prefix_size); int32_t status = ::kv_it_lower_bound(handle, key.data, key_size); return const_iterator(this, handle, status == 0); } - template - const_iterator upper_bound(PK primary) const { - const uint64_t pk = to_pk_uint64(primary); - if (pk == std::numeric_limits::max()) return end(); - return lower_bound(pk + 1); + const_iterator lower_bound(name primary) const { return lower_bound(to_pk_uint64(primary)); } + + const_iterator upper_bound(uint64_t primary) const { + if (primary == std::numeric_limits::max()) return end(); + return lower_bound(primary + 1); } + const_iterator upper_bound(name primary) const { return upper_bound(to_pk_uint64(primary)); } + const_iterator iterator_to(const T& obj) const { uint64_t pk = to_pk_uint64(obj.primary_key()); check(_items.find(pk) != _items.end(), @@ -627,6 +646,14 @@ class kv_multi_index { template const_iterator emplace(name payer, Lambda&& constructor) { + // Reads honour _code (kv_get/kv_contains take a code argument) but writes do not: + // kv_set and kv_idx_store have no code parameter and always land on the receiver. A + // foreign-code handle would therefore probe one account and write another -- upstream + // rejects that, and a ported contract relying on the abort would otherwise get a silent + // write to its own row. Checked before the constructor runs, so a lambda with side + // effects is not executed on the rejected path. + check(_code == receiving_account(), "cannot create objects in table of another contract"); + T obj; constructor(obj); @@ -667,6 +694,8 @@ class kv_multi_index { template void modify(const T& obj, name payer, Lambda&& updater) { + check(_code == receiving_account(), "cannot modify objects in table of another contract"); + T old_obj = obj; // Cast away const for modification (same pattern as legacy multi_index) auto& mutable_obj = const_cast(obj); @@ -694,6 +723,8 @@ class kv_multi_index { } void erase(const T& obj) { + check(_code == receiving_account(), "cannot erase objects in table of another contract"); + uint64_t pk = to_pk_uint64(obj.primary_key()); auto key = make_pk(pk); diff --git a/libraries/sysiolib/contracts/sysio/kv_table.hpp b/libraries/sysiolib/contracts/sysio/kv_table.hpp index af47a0e5e..dad7d07f8 100644 --- a/libraries/sysiolib/contracts/sysio/kv_table.hpp +++ b/libraries/sysiolib/contracts/sysio/kv_table.hpp @@ -703,6 +703,17 @@ class table_impl { /// Insert a new row. Asserts if the key already exists. Use upsert()/set() /// for insert-or-update semantics. + /// + /// WRITES IGNORE code(), as they do in kv::global. kv_get and kv_contains take a code + /// argument, so reads honour whatever account this handle was constructed with; kv_set, + /// kv_erase and kv_idx_store have no such parameter and always land on the current + /// receiver. A handle opened on a FOREIGN account is therefore read-only in practice -- + /// mutating through one probes their table and writes your own, and because table_id is + /// derived from the table name alone, that write lands on your row of the same name. + /// Nothing detects it at compile time. Construct foreign-code handles for reading only. + /// + /// (sysio::multi_index does guard this, because upstream does and ported contracts rely + /// on the abort; these wrappers have no such compatibility obligation.) void emplace(name payer, const K& key, const V& value, const char* exists_msg = "key already exists") { auto k = make_key(key); sysio::check(!::kv_contains(_table_id, code(), k.data(), k.size()), exists_msg); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 0ed05d0bd..98b2d489c 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -24,6 +24,7 @@ add_unit_test( time_tests ) add_unit_test( varint_tests ) add_unit_test( kv_table_tests ) add_unit_test( kv_cached_tests ) +add_unit_test( kv_multi_index_tests ) add_test( NAME toolchain_tests COMMAND ${CMAKE_BINARY_DIR}/tools/toolchain-tester/toolchain-tester ${CMAKE_SOURCE_DIR}/tests/toolchain --cdt ${CMAKE_BINARY_DIR}/bin --verbose ) set_property(TEST toolchain_tests PROPERTY LABELS toolchain_tests) diff --git a/tests/integration/multi_index_tests.cpp b/tests/integration/multi_index_tests.cpp index 8556aa82b..7ea7e0082 100644 --- a/tests/integration/multi_index_tests.cpp +++ b/tests/integration/multi_index_tests.cpp @@ -37,6 +37,10 @@ BOOST_FIXTURE_TEST_CASE(main_multi_index_tests, TESTER) { try { push_action( "testapi"_n, "s1g"_n, "testapi"_n, {} ); // idx64_general push_action( "testapi"_n, "s1namepk"_n, "testapi"_n, {} ); // name_pk_bounds + // A foreign-code handle cannot mutate: reads honour the handle's code but writes land on + // the receiver, so without the guard this silently wrote the receiver's own row. + check_failure( "s1foreign"_n, "cannot create objects in table of another contract" ); + // Duplicate primary key aborts instead of upserting. Without the guard this action // succeeded and left the previous secondary mapping stranded. check_failure( "s1dupidx"_n, "object with the same primary key already exists" ); diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index d13093af7..80b088adc 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -35,6 +35,7 @@ add_cdt_unit_test(time_tests) add_cdt_unit_test(varint_tests) add_cdt_unit_test(kv_table_tests) add_cdt_unit_test(kv_cached_tests) +add_cdt_unit_test(kv_multi_index_tests) target_compile_options( rope_tests PUBLIC -g ) add_subdirectory(test_contracts) diff --git a/tests/unit/kv_multi_index_tests.cpp b/tests/unit/kv_multi_index_tests.cpp new file mode 100644 index 000000000..2fe15366e --- /dev/null +++ b/tests/unit/kv_multi_index_tests.cpp @@ -0,0 +1,181 @@ +/** + * @file + * @copyright defined in sysio.cdt/LICENSE.txt + * + * Native coverage for sysio::multi_index's mutation guards. + * + * These assertions exist natively, rather than only in tests/integration, because + * ENABLE_INTEGRATION_TESTS defaults OFF and the CI workflow does not enable it -- an + * integration-only regression leaves required CI green when the guard is deleted. The + * equivalent on-chain cases live in tests/integration/multi_index_tests.cpp and remain the + * real-runtime coverage. + * + * Two guards are under test, both restored to match upstream multi_index: + * + * 1. A duplicate primary key aborts. kv_set is an upsert, so without the check the row is + * silently overwritten and store_secondaries strands the previous mapping. + * 2. Mutating through a handle opened on another account aborts. Reads take a `code` + * argument and honour it; kv_set, kv_erase and kv_idx_store have none and always land + * on the receiver. table_id derives from the table NAME alone, so a foreign-code + * mutation probes their table and writes the receiver's row of the same name. + * + * The cases below never reach a write: each aborts first, so the mocked store is seeded + * directly rather than through emplace, and no iterator or secondary-index intrinsics are + * needed. + */ + +#include +#include +#include + +#include +#include +#include + +using namespace sysio; +using namespace sysio::native; + +// The generated dispatcher records the receiver here at the top of apply(); the native +// dispatch does not, which is what the fallback in receiving_account() is for. Driving it +// directly lets both branches be exercised. +extern "C" void sysio_set_contract_name(uint64_t n); + +namespace { + +struct record { + uint64_t id; + uint64_t sec; + + uint64_t primary_key() const { return id; } + uint64_t get_secondary() const { return sec; } + + SYSLIB_SERIALIZE(record, (id)(sec)) +}; + +using table_t = sysio::multi_index<"records"_n, record>; + +constexpr uint32_t records_tid = sysio::kv::compute_table_id("records"_n.value); + +// Mirrors the asymmetry under test: kv_contains honours `code`, writes have no such +// parameter. Only the read side is needed -- every case here aborts before writing. +struct mock_kv { + using row_key = std::tuple; // code, table_id, key + std::map rows; + uint64_t receiver = 0; + uint32_t sets = 0; // must stay 0: a rejected mutation writes nothing + + void reset(uint64_t who) { rows.clear(); receiver = who; sets = 0; } +}; + +mock_kv& store() { static mock_kv inst; return inst; } + +/// The 16-byte primary key multi_index builds: [scope:8B BE][pk:8B BE]. +std::string pk_key(uint64_t scope, uint64_t pk) { + std::string k(16, '\0'); + for (int i = 7; i >= 0; --i) { k[i] = char(scope & 0xFF); scope >>= 8; } + for (int i = 7; i >= 0; --i) { k[8 + i] = char(pk & 0xFF); pk >>= 8; } + return k; +} + +void install_intrinsics() { + intrinsics::set_intrinsic( + []() -> capi_name { return store().receiver; }); + + intrinsics::set_intrinsic( + [](uint32_t table_id, capi_name code, const void* key, uint32_t key_size) -> int32_t { + auto k = std::string(static_cast(key), key_size); + return store().rows.count(mock_kv::row_key{code, table_id, k}) ? 1 : 0; + }); + + // No code parameter: a write always lands on the receiver. Counted, never expected. + intrinsics::set_intrinsic( + [](uint32_t, uint64_t, const void*, uint32_t, const void*, uint32_t) -> int64_t { + ++store().sets; + return 0; + }); +} + +/// Seed the receiver's own table with pk, and install the mocks. +/// @param dispatcher_sets_name mirrors the generated dispatcher recording the receiver in +/// the sysio_contract_name global; false leaves it 0, as SYSIO_DISPATCH and the +/// native dispatch do, exercising the current_receiver() fallback instead. +void arrange(uint64_t receiver, uint64_t scope, uint64_t pk, bool dispatcher_sets_name) { + store().reset(receiver); + store().rows[mock_kv::row_key{receiver, records_tid, pk_key(scope, pk)}] = "row"; + install_intrinsics(); + sysio_set_contract_name(dispatcher_sets_name ? receiver : 0); +} + +} // namespace + +// A duplicate primary key must abort rather than upsert. +SYSIO_TEST_BEGIN(duplicate_primary_key_rejected) + for (bool via_global : {true, false}) { + arrange("alice"_n.value, "alice"_n.value, 1, via_global); + table_t t("alice"_n, "alice"_n.value); + + CHECK_ASSERT( "object with the same primary key already exists", + ([&]() { t.emplace("alice"_n, [](auto& r) { r.id = 1; r.sec = 7; }); }) ) + CHECK_EQUAL( store().sets, 0u ) + } +SYSIO_TEST_END + +// Mutating through a foreign-code handle must abort. The receiver holds pk=1 and the foreign +// account does not, so the duplicate probe alone would pass -- this is precisely the case +// where the old code upserted the receiver's row. +SYSIO_TEST_BEGIN(foreign_code_handle_cannot_mutate) + for (bool via_global : {true, false}) { + arrange("alice"_n.value, "alice"_n.value, 1, via_global); + table_t foreign("bob"_n, "alice"_n.value); + record r{1, 7}; + + CHECK_ASSERT( "cannot create objects in table of another contract", + ([&]() { foreign.emplace("alice"_n, [](auto& o) { o.id = 1; o.sec = 7; }); }) ) + CHECK_ASSERT( "cannot modify objects in table of another contract", + ([&]() { foreign.modify(r, "alice"_n, [](auto& o) { o.sec = 9; }); }) ) + CHECK_ASSERT( "cannot erase objects in table of another contract", + ([&]() { foreign.erase(r); }) ) + + // The whole point: the receiver's row was never touched. + CHECK_EQUAL( store().sets, 0u ) + CHECK_EQUAL( store().rows.count(mock_kv::row_key{"alice"_n.value, records_tid, + pk_key("alice"_n.value, 1)}), 1u ) + } +SYSIO_TEST_END + +// A handle on the receiver's own table is unaffected by the guard. +SYSIO_TEST_BEGIN(own_table_handle_passes_the_guard) + for (bool via_global : {true, false}) { + arrange("alice"_n.value, "alice"_n.value, 1, via_global); + table_t t("alice"_n, "alice"_n.value); + + // pk=2 is absent, so the guard and the duplicate probe both pass and the message, + // if any, is not one of the three rejections. + CHECK_ASSERT( "object with the same primary key already exists", + ([&]() { t.emplace("alice"_n, [](auto& o) { o.id = 1; o.sec = 7; }); }) ) + } +SYSIO_TEST_END + +// The primary bounds stay callable as concrete overloads. A member template would break both +// of these: a braced list cannot be deduced, and a pointer cannot be formed to an undeduced +// template. Compile-time only -- neither expression is evaluated. +SYSIO_TEST_BEGIN(primary_bounds_remain_addressable_overloads) + using itr_t = table_t::const_iterator; + constexpr auto lb = static_cast(&table_t::lower_bound); + constexpr auto ub = static_cast(&table_t::upper_bound); + static_assert(lb != nullptr && ub != nullptr, "primary bounds must be addressable overloads"); + + using braced = decltype(std::declval().lower_bound({42})); + using by_name = decltype(std::declval().lower_bound("alice"_n)); + static_assert(std::is_same_v, "lower_bound must accept a braced initializer"); + static_assert(std::is_same_v, "lower_bound must accept a name"); +SYSIO_TEST_END + +int main(int argc, char* argv[]) { + bool verbose = false; + SYSIO_TEST(duplicate_primary_key_rejected) + SYSIO_TEST(foreign_code_handle_cannot_mutate) + SYSIO_TEST(own_table_handle_passes_the_guard) + SYSIO_TEST(primary_bounds_remain_addressable_overloads) + return has_failed(); +} diff --git a/tests/unit/test_contracts/multi_index_tests.cpp b/tests/unit/test_contracts/multi_index_tests.cpp index d91946d54..6e8a5bad7 100644 --- a/tests/unit/test_contracts/multi_index_tests.cpp +++ b/tests/unit/test_contracts/multi_index_tests.cpp @@ -407,6 +407,21 @@ namespace _test_multi_index "name_pk_bounds - lower_bound(uint64_t) regressed"); } + // Mutating through a handle opened on another account must abort. Reads honour the + // handle's code; kv_set/kv_idx_store do not and always land on the receiver, and table_id + // derives from the table name alone -- so without the guard this writes the receiver's + // own row of the same name. Upstream multi_index rejects it. + template + void foreign_code_mutation(sysio::name receiver) + { + typedef record_idx64 record; + sysio::kv_multi_index>> + foreign("bob"_n, receiver.value); + + foreign.emplace(receiver, [&](auto& r) { r.id = 1; r.sec = 1; }); + } + } /// _test_multi_index class [[sysio::contract]] test_multi_index : public sysio::contract @@ -419,6 +434,10 @@ class [[sysio::contract]] test_multi_index : public sysio::contract _test_multi_index::idx64_check_without_storing<"indextable2"_n.value>( get_self() ); } + [[sysio::action("s1foreign")]] void foreign_code_mutation() { + _test_multi_index::foreign_code_mutation<"foreigntbl"_n.value>(get_self()); + } + [[sysio::action("s1namepk")]] void name_pk_bounds() { _test_multi_index::name_pk_bounds<"namepktable"_n.value>(get_self()); } From 072ea2d20fd3d5d06f2b1858ce72ad38cc57fd52 Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Tue, 1 Sep 2026 06:55:40 -0500 Subject: [PATCH 03/28] fix(kv): accept every primary-key call shape with one non-template parameter The two-overload form from the previous round fixed the braced-initializer break but introduced a narrower one: `auto lower = &table_t::lower_bound;` compiled against the base revision and fails against an overload set with ``. Three call shapes pull in different directions -- a member template breaks both `lower_bound({42})` and the bare member-pointer, two overloads fix the first and still break the second. A single non-template function taking an implicitly-constructible primary_key_arg satisfies all three at once, and reaches exactly the types to_pk_uint64 accepts. Callers never name the type; they pass a uint64_t or a name as before. The test was masking this rather than catching it. Its static_cast selected the uint64_t overload from the set, so it passed even while the bare form did not compile -- the same shape of vacuous assertion as the earlier "name": "hiproto" check, in a new disguise. It now takes the address with no cast, and asserts all three call shapes. Confirmed to have teeth: reverting to two overloads fails the build with exactly the reported `` error on those lines. 30/30 ctest; wire-sysio's contracts rebuild against this CDT. --- .../contracts/sysio/kv_multi_index.hpp | 36 +++++++++++-------- tests/unit/kv_multi_index_tests.cpp | 18 ++++++---- 2 files changed, 33 insertions(+), 21 deletions(-) diff --git a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp index 498f12e80..92534d7c6 100644 --- a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp +++ b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp @@ -607,30 +607,36 @@ class kv_multi_index { return *obj; } - /// Overloads rather than a template on the primary key type. + /// A single non-template parameter that accepts every primary key type. /// - /// Upstream templates these and routes through to_raw_key, but a member template is not a - /// drop-in for a concrete overload: `lower_bound({42})` cannot deduce from a braced list, - /// and `&table_type::lower_bound` cannot form a pointer to an undeduced template. Both - /// compile against a plain uint64_t parameter. Two overloads cover exactly the types - /// to_pk_uint64 accepts, which is the same reach the template had, without the break. - const_iterator lower_bound(uint64_t primary) const { - auto key = make_pk(primary); + /// Three call shapes have to keep working and they pull in different directions. A member + /// TEMPLATE (as upstream uses) breaks `lower_bound({42})`, since a braced list cannot be + /// deduced, and `&table_type::lower_bound`, since no pointer can be formed to an undeduced + /// template. Two concrete OVERLOADS fix the braced case but still break the bare + /// member-pointer, which becomes an overload set. One non-template function taking an + /// implicitly-constructible parameter satisfies all three at once, and it reaches exactly + /// the types to_pk_uint64 accepts. + /// + /// Implicit by design: callers never name this type, they pass a uint64_t or a name. + struct primary_key_arg { + uint64_t value; + constexpr primary_key_arg(uint64_t v) : value(v) {} // NOLINT(google-explicit-constructor) + constexpr primary_key_arg(name n) : value(n.value) {} // NOLINT(google-explicit-constructor) + }; + + const_iterator lower_bound(primary_key_arg primary) const { + auto key = make_pk(primary.value); auto prefix = make_prefix(); uint32_t handle = ::kv_it_create(_table_id, _code.value, prefix.data, prefix_size); int32_t status = ::kv_it_lower_bound(handle, key.data, key_size); return const_iterator(this, handle, status == 0); } - const_iterator lower_bound(name primary) const { return lower_bound(to_pk_uint64(primary)); } - - const_iterator upper_bound(uint64_t primary) const { - if (primary == std::numeric_limits::max()) return end(); - return lower_bound(primary + 1); + const_iterator upper_bound(primary_key_arg primary) const { + if (primary.value == std::numeric_limits::max()) return end(); + return lower_bound(primary.value + 1); } - const_iterator upper_bound(name primary) const { return upper_bound(to_pk_uint64(primary)); } - const_iterator iterator_to(const T& obj) const { uint64_t pk = to_pk_uint64(obj.primary_key()); check(_items.find(pk) != _items.end(), diff --git a/tests/unit/kv_multi_index_tests.cpp b/tests/unit/kv_multi_index_tests.cpp index 2fe15366e..77dbff782 100644 --- a/tests/unit/kv_multi_index_tests.cpp +++ b/tests/unit/kv_multi_index_tests.cpp @@ -159,15 +159,21 @@ SYSIO_TEST_END // The primary bounds stay callable as concrete overloads. A member template would break both // of these: a braced list cannot be deduced, and a pointer cannot be formed to an undeduced // template. Compile-time only -- neither expression is evaluated. -SYSIO_TEST_BEGIN(primary_bounds_remain_addressable_overloads) +SYSIO_TEST_BEGIN(primary_bounds_accept_every_call_shape) using itr_t = table_t::const_iterator; - constexpr auto lb = static_cast(&table_t::lower_bound); - constexpr auto ub = static_cast(&table_t::upper_bound); - static_assert(lb != nullptr && ub != nullptr, "primary bounds must be addressable overloads"); + // Bare address-taking, with NO cast. This is the case a cast would hide: an explicit + // static_cast selects from an overload set and so passes even when the bare form does + // not compile, which is exactly how the earlier two-overload revision looked correct. + constexpr auto lb = &table_t::lower_bound; + constexpr auto ub = &table_t::upper_bound; + static_assert(lb != nullptr && ub != nullptr, "primary bounds must be bare-addressable"); + + using by_u64 = decltype(std::declval().lower_bound(uint64_t{42})); using braced = decltype(std::declval().lower_bound({42})); using by_name = decltype(std::declval().lower_bound("alice"_n)); - static_assert(std::is_same_v, "lower_bound must accept a braced initializer"); + static_assert(std::is_same_v, "lower_bound must accept a uint64_t"); + static_assert(std::is_same_v, "lower_bound must accept a braced initializer"); static_assert(std::is_same_v, "lower_bound must accept a name"); SYSIO_TEST_END @@ -176,6 +182,6 @@ int main(int argc, char* argv[]) { SYSIO_TEST(duplicate_primary_key_rejected) SYSIO_TEST(foreign_code_handle_cannot_mutate) SYSIO_TEST(own_table_handle_passes_the_guard) - SYSIO_TEST(primary_bounds_remain_addressable_overloads) + SYSIO_TEST(primary_bounds_accept_every_call_shape) return has_failed(); } From cb754421e28305dcfba902dbd7399a579f9fbbb5 Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Tue, 1 Sep 2026 09:27:56 -0500 Subject: [PATCH 04/28] fix(kv): keep the primary-key argument domain as wide as its siblings The proxy fixed bare address-taking but narrowed what the bounds accept. With a fixed uint64_t parameter, a caller's key wrapper with operator uint64_t() needs wrapper -> uint64_t -> primary_key_arg, two user-defined conversions, so it is rejected -- while find, get and require_find take uint64_t directly and accept the same type today, and upstream's templated bound accepts it through to_raw_key. The bounds were the only part of the API refusing it. `{}` regressed too: it meant key zero against a plain uint64_t parameter and the proxy had no default constructor. The converting constructor is now a constrained template taking PK by value, so a uint64-convertible type costs one user-defined conversion rather than two, and the proxy is default-constructible at zero. The constraint keeps it from swallowing `name`, which has no implicit uint64_t conversion and so still selects its own overload, and leaves copy construction alone. PK rather than T because T is the row type of the enclosing kv_multi_index. Test gains the two shapes that regressed -- an empty brace and a wrapped key -- alongside the four already covered, plus static asserts that the conversions produce the right value. Confirmed to have teeth: with the narrow proxy restored the build fails on `no matching constructor` for {} and `no viable conversion from wrapped_key`. Also ignores core.* / vgcore.*, which a deliberate-crash test run leaves behind. 30/30 ctest. --- .gitignore | 4 +++ .../contracts/sysio/kv_multi_index.hpp | 24 +++++++++++++-- tests/unit/kv_multi_index_tests.cpp | 29 +++++++++++++++---- 3 files changed, 48 insertions(+), 9 deletions(-) diff --git a/.gitignore b/.gitignore index d761dcdab..a73afd95a 100644 --- a/.gitignore +++ b/.gitignore @@ -71,3 +71,7 @@ tmp/ # prequel local review state (operational artifacts, never committed) .prequel/ + +# Core dumps (deliberate-crash test runs leave these behind) +core.* +vgcore.* diff --git a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp index 92534d7c6..1a11a8bd2 100644 --- a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp +++ b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp @@ -618,10 +618,28 @@ class kv_multi_index { /// the types to_pk_uint64 accepts. /// /// Implicit by design: callers never name this type, they pass a uint64_t or a name. + /// + /// The converting constructor is a CONSTRAINED TEMPLATE, not a fixed `uint64_t` parameter. + /// A fixed one would need `wrapper -> uint64_t -> primary_key_arg` for a user type with + /// `operator uint64_t()`, which is two user-defined conversions and therefore ill-formed -- + /// narrowing the argument domain below `find`, `get` and `require_find`, which take + /// `uint64_t` directly and accept such a type today. Taking `T` by value keeps it to one + /// user-defined conversion. The constraint stops it swallowing `name` (which has no + /// implicit `uint64_t` conversion, so the dedicated overload wins) or unrelated types, + /// and leaves copy construction alone. + /// + /// Default-constructible so `lower_bound({})` still means key zero, as it did when the + /// parameter was a plain `uint64_t`. struct primary_key_arg { - uint64_t value; - constexpr primary_key_arg(uint64_t v) : value(v) {} // NOLINT(google-explicit-constructor) - constexpr primary_key_arg(name n) : value(n.value) {} // NOLINT(google-explicit-constructor) + uint64_t value = 0; + + constexpr primary_key_arg() = default; + constexpr primary_key_arg(name n) : value(n.value) {} // NOLINT(google-explicit-constructor) + + // PK, not T: T is the row type of the enclosing kv_multi_index. + template>> + constexpr primary_key_arg(PK v) // NOLINT(google-explicit-constructor) + : value(static_cast(v)) {} }; const_iterator lower_bound(primary_key_arg primary) const { diff --git a/tests/unit/kv_multi_index_tests.cpp b/tests/unit/kv_multi_index_tests.cpp index 77dbff782..9e4d7d405 100644 --- a/tests/unit/kv_multi_index_tests.cpp +++ b/tests/unit/kv_multi_index_tests.cpp @@ -54,6 +54,13 @@ struct record { using table_t = sysio::multi_index<"records"_n, record>; +/// A caller-supplied key wrapper. find/get/require_find take uint64_t directly and so accept +/// one of these through a single user-defined conversion; the bounds must not be narrower. +struct wrapped_key { + uint64_t v; + constexpr operator uint64_t() const { return v; } // NOLINT(google-explicit-constructor) +}; + constexpr uint32_t records_tid = sysio::kv::compute_table_id("records"_n.value); // Mirrors the asymmetry under test: kv_contains honours `code`, writes have no such @@ -169,12 +176,22 @@ SYSIO_TEST_BEGIN(primary_bounds_accept_every_call_shape) constexpr auto ub = &table_t::upper_bound; static_assert(lb != nullptr && ub != nullptr, "primary bounds must be bare-addressable"); - using by_u64 = decltype(std::declval().lower_bound(uint64_t{42})); - using braced = decltype(std::declval().lower_bound({42})); - using by_name = decltype(std::declval().lower_bound("alice"_n)); - static_assert(std::is_same_v, "lower_bound must accept a uint64_t"); - static_assert(std::is_same_v, "lower_bound must accept a braced initializer"); - static_assert(std::is_same_v, "lower_bound must accept a name"); + using by_u64 = decltype(std::declval().lower_bound(uint64_t{42})); + using braced = decltype(std::declval().lower_bound({42})); + using by_name = decltype(std::declval().lower_bound("alice"_n)); + using empty_br = decltype(std::declval().lower_bound({})); + using wrapped = decltype(std::declval().lower_bound(wrapped_key{7})); + static_assert(std::is_same_v, "lower_bound must accept a uint64_t"); + static_assert(std::is_same_v, "lower_bound must accept a braced initializer"); + static_assert(std::is_same_v, "lower_bound must accept a name"); + // {} meant key zero when the parameter was a plain uint64_t, and must still. + static_assert(std::is_same_v, "lower_bound must accept an empty brace"); + // find/get/require_find accept this through one user-defined conversion; a proxy with a + // fixed uint64_t parameter would need two and reject it, narrowing the bounds below its + // own siblings. + static_assert(std::is_same_v, "lower_bound must accept a uint64-convertible type"); + static_assert(table_t::primary_key_arg{}.value == 0u, "an empty brace must mean key zero"); + static_assert(table_t::primary_key_arg{wrapped_key{7}}.value == 7u, "conversion must preserve the key"); SYSIO_TEST_END int main(int argc, char* argv[]) { From 9b51d2a9b871fae12c27265b6d3a0ff906ea6e24 Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Tue, 1 Sep 2026 10:02:58 -0500 Subject: [PATCH 05/28] fix(kv): match the former uint64_t parameter's conversion semantics exactly The constrained constructor accepted the right set of types but did not convert them the way a plain uint64_t parameter would, in three separate ways. Implicit, not static_cast. A wrapper offering an implicit operator unsigned() returning 1 and an explicit operator uint64_t() returning 2 converts to 1 through a uint64_t parameter; static_cast preferred the exact explicit conversion and stored 2, so lower_bound could seek a different row than find. The argument is now passed to a uint64_t parameter -- copy-initialisation, which considers only implicit conversions -- rather than cast. Note a member initialiser could not do this: `value(x)` is direct-initialisation and would consider the explicit operator too, which is what the first attempt at this fix got wrong. Forwarded, not copied. Taking PK by value copied lvalues, rejecting the noncopyable wrappers the base accepted, and tested a different value category in the constraint than the body then used -- an &&-only conversion passed SFINAE and failed in the body. It now takes PK&& and forwards. Narrowing preserved. Letting the template consume arithmetic and enum arguments bypassed list-initialisation narrowing: the base rejects lower_bound({-1}) and ({1.5}), this accepted and silently converted them. Those types are excluded from the template and reach the uint64_t constructor, where the narrowing rules apply. Tests gain the exact cases: an implicit/explicit dual-conversion wrapper asserting the implicit result, a noncopyable wrapper passed as an lvalue, and a detection trait proving brace-initialisation still rejects a non-constant int and a double while accepting uint64_t. Confirmed to have teeth -- restoring the static_cast and by-value form fails the dual-conversion assertion. Core-dump ignore patterns narrowed to the shapes the kernel actually writes (core_pattern is core.%e.%p) and root-anchored, so core.cpp, core.hpp and tracked headers such as boost/hana/core.hpp stay visible. 30/30 ctest. --- .gitignore | 10 +++-- .../contracts/sysio/kv_multi_index.hpp | 42 ++++++++++++++++-- tests/unit/kv_multi_index_tests.cpp | 43 +++++++++++++++++++ 3 files changed, 88 insertions(+), 7 deletions(-) diff --git a/.gitignore b/.gitignore index a73afd95a..70fdc9ce1 100644 --- a/.gitignore +++ b/.gitignore @@ -72,6 +72,10 @@ tmp/ # prequel local review state (operational artifacts, never committed) .prequel/ -# Core dumps (deliberate-crash test runs leave these behind) -core.* -vgcore.* +# Core dumps. Restricted to the shapes the kernel actually writes here -- core_pattern is +# core.%e.%p -- and root-anchored, so neither a tracked header such as +# libraries/boost/include/boost/hana/core.hpp nor a future core.cpp/core.hpp is hidden. +/core +/core.[0-9]* +/core.*.[0-9]* +/vgcore.[0-9]* diff --git a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp index 1a11a8bd2..eab227945 100644 --- a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp +++ b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp @@ -29,6 +29,7 @@ #include #include #include +#include #include #include @@ -634,12 +635,45 @@ class kv_multi_index { uint64_t value = 0; constexpr primary_key_arg() = default; + + /// Arithmetic and enum arguments arrive through a real `uint64_t` parameter, so + /// list-initialization narrowing still applies: `lower_bound({-1})` and `({1.5})` stay + /// ill-formed exactly as they were against a plain `uint64_t` parameter. Routing them + /// through the template below would have converted them silently. + constexpr primary_key_arg(uint64_t v) : value(v) {} // NOLINT(google-explicit-constructor) + constexpr primary_key_arg(name n) : value(n.value) {} // NOLINT(google-explicit-constructor) - // PK, not T: T is the row type of the enclosing kv_multi_index. - template>> - constexpr primary_key_arg(PK v) // NOLINT(google-explicit-constructor) - : value(static_cast(v)) {} + /// Any other implicitly-uint64-convertible type -- the key wrappers that `find`, `get` + /// and `require_find` already accept. + /// + /// The original expression is forwarded and converted IMPLICITLY, not `static_cast`. + /// A cast would prefer an exact explicit `operator uint64_t()` over an implicit + /// `operator unsigned()`, so a wrapper offering both would seek a different row here + /// than in find(). Copy-initialising the member reproduces the conversion the plain + /// `uint64_t` parameter would have chosen. + /// + /// PK&& rather than by value: taking it by value copied lvalues, rejecting the + /// noncopyable wrappers the base accepted, and tested a different value category in + /// the constraint than the body then used. PK, not T -- T is the enclosing row type. + template, + typename = std::enable_if_t && + !std::is_enum_v && + !std::is_same_v && + !std::is_same_v && + std::is_convertible_v>> + constexpr primary_key_arg(PK&& v) // NOLINT(google-explicit-constructor) + : value(as_key(std::forward(v))) {} + + private: + /// Copy-initialises a uint64_t parameter, which is exactly what the plain `uint64_t` + /// parameter used to do. A member initialiser -- `value(x)` -- is DIRECT-initialisation + /// and would consider `explicit operator uint64_t()`, picking a different conversion + /// than find() for a wrapper offering both. + static constexpr uint64_t as_key(uint64_t v) { return v; } + + public: }; const_iterator lower_bound(primary_key_arg primary) const { diff --git a/tests/unit/kv_multi_index_tests.cpp b/tests/unit/kv_multi_index_tests.cpp index 9e4d7d405..2be121516 100644 --- a/tests/unit/kv_multi_index_tests.cpp +++ b/tests/unit/kv_multi_index_tests.cpp @@ -61,6 +61,32 @@ struct wrapped_key { constexpr operator uint64_t() const { return v; } // NOLINT(google-explicit-constructor) }; +/// Offers both an implicit and an explicit conversion, disagreeing on the key. A plain +/// uint64_t parameter picks the IMPLICIT one; a static_cast picks the explicit exact match. +/// If the bounds disagreed with find() here, they would seek a different row. +struct dual_conversion_key { + constexpr operator unsigned() const { return 1; } // NOLINT(google-explicit-constructor) + constexpr explicit operator uint64_t() const { return 2; } +}; + +/// Noncopyable, as a caller's handle type may be. A by-value template parameter copied the +/// argument and rejected this; the base, taking uint64_t, never copied the wrapper. +struct move_only_key { + move_only_key() = default; + move_only_key(const move_only_key&) = delete; + move_only_key(move_only_key&&) = default; + constexpr operator uint64_t() const { return 5; } // NOLINT(google-explicit-constructor) +}; + +/// Is `primary_key_arg{A}` well-formed? Used to assert that list-initialization narrowing +/// still rejects arithmetic arguments that cannot be represented, as it did when the +/// parameter was a plain uint64_t. +template +struct braces_from : std::false_type {}; +template +struct braces_from()})>> + : std::true_type {}; + constexpr uint32_t records_tid = sysio::kv::compute_table_id("records"_n.value); // Mirrors the asymmetry under test: kv_contains honours `code`, writes have no such @@ -192,6 +218,23 @@ SYSIO_TEST_BEGIN(primary_bounds_accept_every_call_shape) static_assert(std::is_same_v, "lower_bound must accept a uint64-convertible type"); static_assert(table_t::primary_key_arg{}.value == 0u, "an empty brace must mean key zero"); static_assert(table_t::primary_key_arg{wrapped_key{7}}.value == 7u, "conversion must preserve the key"); + + // The conversion chosen must be the one a plain uint64_t parameter would choose: the + // implicit operator, not the explicit exact match a static_cast would prefer. + static_assert(table_t::primary_key_arg{dual_conversion_key{}}.value == 1u, + "an implicit conversion must win over an explicit one, as it does for find()"); + + // Narrowing survives. A non-constant int and a double cannot be represented in a uint64_t + // without narrowing, so brace-initialisation must reject them -- routing arithmetic through + // the template would have silently converted both. + static_assert(braces_from::value, "uint64_t must brace-initialise"); + static_assert(!braces_from::value, "a non-constant int must be rejected as narrowing"); + static_assert(!braces_from::value, "a double must be rejected as narrowing"); + + // A noncopyable wrapper passed as an lvalue: forwarded, never copied. + move_only_key mo; + using by_move_only = decltype(std::declval().lower_bound(mo)); + static_assert(std::is_same_v, "a noncopyable wrapper must be accepted"); SYSIO_TEST_END int main(int argc, char* argv[]) { From 78bd7d613b1366a491fd65e347cef1093e0c6e6f Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Tue, 1 Sep 2026 11:06:16 -0500 Subject: [PATCH 06/28] revert(kv): drop the templated primary bounds and their proxy lower_bound/upper_bound go back to taking a plain uint64_t, and primary_key_arg is deleted. Templating the bounds so a `name` primary key could be passed directly was never needed by the duplicate-key and receiver guards this PR exists for -- it was an opportunistic convenience. It broke source compatibility for `lower_bound({42})` and `&table_type::lower_bound`, and the proxy type introduced to restore those could not reproduce a `uint64_t` parameter's conversion semantics exactly: copy- versus direct-initialisation, value category, and list-init narrowing each pulled in a different direction, and closing one gap reopened another. Callers with a `name` primary key pass `.value`, exactly as they did before. Kept: the to_pk_uint64 calls in store/remove/update_secondaries, which are an independent fix -- pk_to_bytes takes uint64_t, so a `name` primary key combined with a secondary index did not compile at all. The contract-side test is retargeted accordingly: name_pk_bounds becomes name_pk_secondaries and now exercises the path to_pk_uint64 actually fixed (secondary lookup, modify rewriting the mapping, erase removing it) rather than the bounds' argument types. --- .../contracts/sysio/kv_multi_index.hpp | 78 ++---------------- tests/integration/multi_index_tests.cpp | 2 +- tests/unit/kv_multi_index_tests.cpp | 82 ------------------- .../unit/test_contracts/multi_index_tests.cpp | 46 +++++++---- 4 files changed, 37 insertions(+), 171 deletions(-) diff --git a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp index eab227945..b97510e97 100644 --- a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp +++ b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp @@ -608,85 +608,17 @@ class kv_multi_index { return *obj; } - /// A single non-template parameter that accepts every primary key type. - /// - /// Three call shapes have to keep working and they pull in different directions. A member - /// TEMPLATE (as upstream uses) breaks `lower_bound({42})`, since a braced list cannot be - /// deduced, and `&table_type::lower_bound`, since no pointer can be formed to an undeduced - /// template. Two concrete OVERLOADS fix the braced case but still break the bare - /// member-pointer, which becomes an overload set. One non-template function taking an - /// implicitly-constructible parameter satisfies all three at once, and it reaches exactly - /// the types to_pk_uint64 accepts. - /// - /// Implicit by design: callers never name this type, they pass a uint64_t or a name. - /// - /// The converting constructor is a CONSTRAINED TEMPLATE, not a fixed `uint64_t` parameter. - /// A fixed one would need `wrapper -> uint64_t -> primary_key_arg` for a user type with - /// `operator uint64_t()`, which is two user-defined conversions and therefore ill-formed -- - /// narrowing the argument domain below `find`, `get` and `require_find`, which take - /// `uint64_t` directly and accept such a type today. Taking `T` by value keeps it to one - /// user-defined conversion. The constraint stops it swallowing `name` (which has no - /// implicit `uint64_t` conversion, so the dedicated overload wins) or unrelated types, - /// and leaves copy construction alone. - /// - /// Default-constructible so `lower_bound({})` still means key zero, as it did when the - /// parameter was a plain `uint64_t`. - struct primary_key_arg { - uint64_t value = 0; - - constexpr primary_key_arg() = default; - - /// Arithmetic and enum arguments arrive through a real `uint64_t` parameter, so - /// list-initialization narrowing still applies: `lower_bound({-1})` and `({1.5})` stay - /// ill-formed exactly as they were against a plain `uint64_t` parameter. Routing them - /// through the template below would have converted them silently. - constexpr primary_key_arg(uint64_t v) : value(v) {} // NOLINT(google-explicit-constructor) - - constexpr primary_key_arg(name n) : value(n.value) {} // NOLINT(google-explicit-constructor) - - /// Any other implicitly-uint64-convertible type -- the key wrappers that `find`, `get` - /// and `require_find` already accept. - /// - /// The original expression is forwarded and converted IMPLICITLY, not `static_cast`. - /// A cast would prefer an exact explicit `operator uint64_t()` over an implicit - /// `operator unsigned()`, so a wrapper offering both would seek a different row here - /// than in find(). Copy-initialising the member reproduces the conversion the plain - /// `uint64_t` parameter would have chosen. - /// - /// PK&& rather than by value: taking it by value copied lvalues, rejecting the - /// noncopyable wrappers the base accepted, and tested a different value category in - /// the constraint than the body then used. PK, not T -- T is the enclosing row type. - template, - typename = std::enable_if_t && - !std::is_enum_v && - !std::is_same_v && - !std::is_same_v && - std::is_convertible_v>> - constexpr primary_key_arg(PK&& v) // NOLINT(google-explicit-constructor) - : value(as_key(std::forward(v))) {} - - private: - /// Copy-initialises a uint64_t parameter, which is exactly what the plain `uint64_t` - /// parameter used to do. A member initialiser -- `value(x)` -- is DIRECT-initialisation - /// and would consider `explicit operator uint64_t()`, picking a different conversion - /// than find() for a wrapper offering both. - static constexpr uint64_t as_key(uint64_t v) { return v; } - - public: - }; - - const_iterator lower_bound(primary_key_arg primary) const { - auto key = make_pk(primary.value); + const_iterator lower_bound(uint64_t primary) const { + auto key = make_pk(primary); auto prefix = make_prefix(); uint32_t handle = ::kv_it_create(_table_id, _code.value, prefix.data, prefix_size); int32_t status = ::kv_it_lower_bound(handle, key.data, key_size); return const_iterator(this, handle, status == 0); } - const_iterator upper_bound(primary_key_arg primary) const { - if (primary.value == std::numeric_limits::max()) return end(); - return lower_bound(primary.value + 1); + const_iterator upper_bound(uint64_t primary) const { + if (primary == std::numeric_limits::max()) return end(); + return lower_bound(primary + 1); } const_iterator iterator_to(const T& obj) const { diff --git a/tests/integration/multi_index_tests.cpp b/tests/integration/multi_index_tests.cpp index 7ea7e0082..ec0e0f246 100644 --- a/tests/integration/multi_index_tests.cpp +++ b/tests/integration/multi_index_tests.cpp @@ -35,7 +35,7 @@ BOOST_FIXTURE_TEST_CASE(main_multi_index_tests, TESTER) { try { }; push_action( "testapi"_n, "s1g"_n, "testapi"_n, {} ); // idx64_general - push_action( "testapi"_n, "s1namepk"_n, "testapi"_n, {} ); // name_pk_bounds + push_action( "testapi"_n, "s1namepk"_n, "testapi"_n, {} ); // name_pk_secondaries // A foreign-code handle cannot mutate: reads honour the handle's code but writes land on // the receiver, so without the guard this silently wrote the receiver's own row. diff --git a/tests/unit/kv_multi_index_tests.cpp b/tests/unit/kv_multi_index_tests.cpp index 2be121516..696028b45 100644 --- a/tests/unit/kv_multi_index_tests.cpp +++ b/tests/unit/kv_multi_index_tests.cpp @@ -54,39 +54,6 @@ struct record { using table_t = sysio::multi_index<"records"_n, record>; -/// A caller-supplied key wrapper. find/get/require_find take uint64_t directly and so accept -/// one of these through a single user-defined conversion; the bounds must not be narrower. -struct wrapped_key { - uint64_t v; - constexpr operator uint64_t() const { return v; } // NOLINT(google-explicit-constructor) -}; - -/// Offers both an implicit and an explicit conversion, disagreeing on the key. A plain -/// uint64_t parameter picks the IMPLICIT one; a static_cast picks the explicit exact match. -/// If the bounds disagreed with find() here, they would seek a different row. -struct dual_conversion_key { - constexpr operator unsigned() const { return 1; } // NOLINT(google-explicit-constructor) - constexpr explicit operator uint64_t() const { return 2; } -}; - -/// Noncopyable, as a caller's handle type may be. A by-value template parameter copied the -/// argument and rejected this; the base, taking uint64_t, never copied the wrapper. -struct move_only_key { - move_only_key() = default; - move_only_key(const move_only_key&) = delete; - move_only_key(move_only_key&&) = default; - constexpr operator uint64_t() const { return 5; } // NOLINT(google-explicit-constructor) -}; - -/// Is `primary_key_arg{A}` well-formed? Used to assert that list-initialization narrowing -/// still rejects arithmetic arguments that cannot be represented, as it did when the -/// parameter was a plain uint64_t. -template -struct braces_from : std::false_type {}; -template -struct braces_from()})>> - : std::true_type {}; - constexpr uint32_t records_tid = sysio::kv::compute_table_id("records"_n.value); // Mirrors the asymmetry under test: kv_contains honours `code`, writes have no such @@ -189,59 +156,10 @@ SYSIO_TEST_BEGIN(own_table_handle_passes_the_guard) } SYSIO_TEST_END -// The primary bounds stay callable as concrete overloads. A member template would break both -// of these: a braced list cannot be deduced, and a pointer cannot be formed to an undeduced -// template. Compile-time only -- neither expression is evaluated. -SYSIO_TEST_BEGIN(primary_bounds_accept_every_call_shape) - using itr_t = table_t::const_iterator; - - // Bare address-taking, with NO cast. This is the case a cast would hide: an explicit - // static_cast selects from an overload set and so passes even when the bare form does - // not compile, which is exactly how the earlier two-overload revision looked correct. - constexpr auto lb = &table_t::lower_bound; - constexpr auto ub = &table_t::upper_bound; - static_assert(lb != nullptr && ub != nullptr, "primary bounds must be bare-addressable"); - - using by_u64 = decltype(std::declval().lower_bound(uint64_t{42})); - using braced = decltype(std::declval().lower_bound({42})); - using by_name = decltype(std::declval().lower_bound("alice"_n)); - using empty_br = decltype(std::declval().lower_bound({})); - using wrapped = decltype(std::declval().lower_bound(wrapped_key{7})); - static_assert(std::is_same_v, "lower_bound must accept a uint64_t"); - static_assert(std::is_same_v, "lower_bound must accept a braced initializer"); - static_assert(std::is_same_v, "lower_bound must accept a name"); - // {} meant key zero when the parameter was a plain uint64_t, and must still. - static_assert(std::is_same_v, "lower_bound must accept an empty brace"); - // find/get/require_find accept this through one user-defined conversion; a proxy with a - // fixed uint64_t parameter would need two and reject it, narrowing the bounds below its - // own siblings. - static_assert(std::is_same_v, "lower_bound must accept a uint64-convertible type"); - static_assert(table_t::primary_key_arg{}.value == 0u, "an empty brace must mean key zero"); - static_assert(table_t::primary_key_arg{wrapped_key{7}}.value == 7u, "conversion must preserve the key"); - - // The conversion chosen must be the one a plain uint64_t parameter would choose: the - // implicit operator, not the explicit exact match a static_cast would prefer. - static_assert(table_t::primary_key_arg{dual_conversion_key{}}.value == 1u, - "an implicit conversion must win over an explicit one, as it does for find()"); - - // Narrowing survives. A non-constant int and a double cannot be represented in a uint64_t - // without narrowing, so brace-initialisation must reject them -- routing arithmetic through - // the template would have silently converted both. - static_assert(braces_from::value, "uint64_t must brace-initialise"); - static_assert(!braces_from::value, "a non-constant int must be rejected as narrowing"); - static_assert(!braces_from::value, "a double must be rejected as narrowing"); - - // A noncopyable wrapper passed as an lvalue: forwarded, never copied. - move_only_key mo; - using by_move_only = decltype(std::declval().lower_bound(mo)); - static_assert(std::is_same_v, "a noncopyable wrapper must be accepted"); -SYSIO_TEST_END - int main(int argc, char* argv[]) { bool verbose = false; SYSIO_TEST(duplicate_primary_key_rejected) SYSIO_TEST(foreign_code_handle_cannot_mutate) SYSIO_TEST(own_table_handle_passes_the_guard) - SYSIO_TEST(primary_bounds_accept_every_call_shape) return has_failed(); } diff --git a/tests/unit/test_contracts/multi_index_tests.cpp b/tests/unit/test_contracts/multi_index_tests.cpp index 6e8a5bad7..49d3a82b3 100644 --- a/tests/unit/test_contracts/multi_index_tests.cpp +++ b/tests/unit/test_contracts/multi_index_tests.cpp @@ -365,9 +365,9 @@ namespace _test_multi_index table.emplace(payer, [&](auto& r) { r.id = 1; r.sec = "bbb"_n.value; }); } - // A `name` primary key exercises the templated lower_bound/upper_bound. These took a - // bare uint64_t, so this did not compile, while upstream multi_index accepts it via - // to_raw_key. + // A `name` primary key alongside a secondary index. store/remove/update_secondaries feed + // primary_key() straight to pk_to_bytes(uint64_t), so before to_pk_uint64 was applied + // there this combination did not compile at all. struct record_name_pk { sysio::name owner; @@ -380,7 +380,7 @@ namespace _test_multi_index }; template - void name_pk_bounds(sysio::name receiver) + void name_pk_secondaries(sysio::name receiver) { typedef record_name_pk record; sysio::kv_multi_indexowner == "bob"_n, - "name_pk_bounds - lower_bound(name) did not land on bob"); + "name_pk_secondaries - lower_bound did not land on bob"); - auto ub = table.upper_bound("bob"_n); + auto ub = table.upper_bound("bob"_n.value); sysio::check(ub != table.end() && ub->owner == "charlie"_n, - "name_pk_bounds - upper_bound(name) did not land on charlie"); + "name_pk_secondaries - upper_bound did not land on charlie"); - // The uint64_t form must keep working unchanged. - auto lb_raw = table.lower_bound("bob"_n.value); - sysio::check(lb_raw != table.end() && lb_raw->owner == "bob"_n, - "name_pk_bounds - lower_bound(uint64_t) regressed"); + // The secondary index must resolve back to the name-keyed row: this is the path + // to_pk_uint64 fixed. modify() rewrites the mapping, erase() removes it. + auto sec = table.template get_index<"bysecondary"_n>(); + auto sitr = sec.find(20); + sysio::check(sitr != sec.end() && sitr->owner == "bob"_n, + "name_pk_secondaries - secondary lookup did not resolve to bob"); + + table.modify(*sitr, payer, [&](auto& r) { r.sec = 25; }); + sysio::check(sec.find(20) == sec.end(), + "name_pk_secondaries - modify left the old secondary mapping behind"); + auto moved = sec.find(25); + sysio::check(moved != sec.end() && moved->owner == "bob"_n, + "name_pk_secondaries - modify did not install the new secondary mapping"); + + table.erase(*moved); + sysio::check(sec.find(25) == sec.end(), + "name_pk_secondaries - erase left the secondary mapping behind"); + sysio::check(table.find("bob"_n.value) == table.end(), + "name_pk_secondaries - erase did not remove the primary row"); } // Mutating through a handle opened on another account must abort. Reads honour the @@ -438,8 +454,8 @@ class [[sysio::contract]] test_multi_index : public sysio::contract _test_multi_index::foreign_code_mutation<"foreigntbl"_n.value>(get_self()); } - [[sysio::action("s1namepk")]] void name_pk_bounds() { - _test_multi_index::name_pk_bounds<"namepktable"_n.value>(get_self()); + [[sysio::action("s1namepk")]] void name_pk_secondaries() { + _test_multi_index::name_pk_secondaries<"namepktable"_n.value>(get_self()); } [[sysio::action("s1dupidx")]] void idx64_duplicate_emplace() { From 1668490e8a7d89beb93edafabcb803350b3192a0 Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Tue, 1 Sep 2026 11:23:13 -0500 Subject: [PATCH 07/28] feat(kv): accept a name at multi_index's primary bounds lower_bound and upper_bound gain a one-line `name` overload delegating to the uint64_t one -- the exact shape find, require_find and get have used in this class all along (kv_multi_index.hpp:585-605). This is the third attempt at `name` bounds and the first that changes nothing else. A member template could not deduce `lower_bound({42})`. The primary_key_arg proxy that replaced it restored the braced form but could not reproduce a uint64_t parameter's conversion semantics: it silently accepted `lower_bound({w})` for a `w` converting to a narrower type, which a real uint64_t parameter rejects as narrowing. Plain overloads keep every conversion the base performed, because the uint64_t parameter is still a uint64_t parameter. `name`'s uint64_t constructor is explicit, so `name` is never a viable candidate for a braced integer and the braced forms stay unambiguous. The one behaviour change is that `&table::lower_bound` is now an overload set, so the bare address cannot be taken. That has always been true of `&table::find`, `&table::get` and `&table::require_find` for the same reason; the bounds were the only primary accessors where it worked. A named static_cast still resolves either overload, and the test uses that form. Verified: the new assertions fail without the overloads -- a build against a header carrying only the uint64_t form fails on the static_cast with "to 'itr_t (table_t::*)(name) const' is not allowed" and on the contract-side call with "no viable conversion from 'sysio::name' to 'uint64_t'". ctest 30/30 and the multi_index integration suite 24/24 pass with them. --- .../contracts/sysio/kv_multi_index.hpp | 13 ++++++ tests/unit/kv_multi_index_tests.cpp | 42 +++++++++++++++++++ .../unit/test_contracts/multi_index_tests.cpp | 16 ++++--- 3 files changed, 65 insertions(+), 6 deletions(-) diff --git a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp index b97510e97..7a28ab631 100644 --- a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp +++ b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp @@ -608,6 +608,18 @@ class kv_multi_index { return *obj; } + /// Matches the two-overload shape find/require_find/get already use above: a one-line + /// `name` form delegating to the `uint64_t` one. Overloads rather than a template or a + /// converting-proxy parameter -- both were tried and both changed the argument's meaning. + /// A template cannot deduce `lower_bound({42})`; a proxy accepts `lower_bound({w})` for a + /// `w` converting to a narrower type, which a real `uint64_t` parameter rejects as + /// narrowing. Two overloads keep every conversion the base performed, unchanged. + /// + /// Like its three siblings, this makes `&table::lower_bound` an overload set, so the bare + /// address can no longer be taken -- as has always been true of `&table::find`, + /// `&table::get` and `&table::require_find`. Named overloads still resolve: + /// `static_cast(&table::lower_bound)`. + const_iterator lower_bound(name primary) const { return lower_bound(primary.value); } const_iterator lower_bound(uint64_t primary) const { auto key = make_pk(primary); auto prefix = make_prefix(); @@ -616,6 +628,7 @@ class kv_multi_index { return const_iterator(this, handle, status == 0); } + const_iterator upper_bound(name primary) const { return upper_bound(primary.value); } const_iterator upper_bound(uint64_t primary) const { if (primary == std::numeric_limits::max()) return end(); return lower_bound(primary + 1); diff --git a/tests/unit/kv_multi_index_tests.cpp b/tests/unit/kv_multi_index_tests.cpp index 696028b45..a89fa8edd 100644 --- a/tests/unit/kv_multi_index_tests.cpp +++ b/tests/unit/kv_multi_index_tests.cpp @@ -29,6 +29,8 @@ #include #include +#include +#include #include #include @@ -54,6 +56,13 @@ struct record { using table_t = sysio::multi_index<"records"_n, record>; +/// A caller's key wrapper. find/get/require_find take uint64_t and so accept one of these +/// through a single user-defined conversion; the bounds must not be narrower than they are. +struct wrapped_key { + uint64_t v; + constexpr operator uint64_t() const { return v; } // NOLINT(google-explicit-constructor) +}; + constexpr uint32_t records_tid = sysio::kv::compute_table_id("records"_n.value); // Mirrors the asymmetry under test: kv_contains honours `code`, writes have no such @@ -156,10 +165,43 @@ SYSIO_TEST_BEGIN(own_table_handle_passes_the_guard) } SYSIO_TEST_END +// The primary bounds take a `name` as well as a uint64_t, matching the two-overload shape +// find/require_find/get have always used. Compile-time only -- nothing here is evaluated. +SYSIO_TEST_BEGIN(primary_bounds_accept_uint64_and_name) + using itr_t = table_t::const_iterator; + + // Pin BOTH overloads by exact signature. A named static_cast resolves an overload set, so + // these fail to compile if either parameter type changes -- which is what would happen if + // the uint64_t parameter were ever swapped for a converting proxy again. That also + // demonstrates the documented escape hatch for taking a member pointer. + constexpr auto lb_u64 = static_cast(&table_t::lower_bound); + constexpr auto lb_name = static_cast(&table_t::lower_bound); + constexpr auto ub_u64 = static_cast(&table_t::upper_bound); + constexpr auto ub_name = static_cast(&table_t::upper_bound); + static_assert(lb_u64 && lb_name && ub_u64 && ub_name, "both bound overloads must exist"); + + // A real uint64_t parameter, so every conversion the base performed is unchanged. The + // braced forms in particular must stay unambiguous: name's uint64_t constructor is + // explicit, so name is never viable for a braced integer. + // declval, not a dereferenced null: these appear only in unevaluated operands, and the + // test body itself must stay well-defined at run time. +#define LB(expr) decltype(std::declval().lower_bound expr) +#define UB(expr) decltype(std::declval().upper_bound expr) + static_assert(std::is_same_v, "uint64_t"); + static_assert(std::is_same_v, "a name"); + static_assert(std::is_same_v, "braced literal"); + static_assert(std::is_same_v, "empty brace, key zero"); + static_assert(std::is_same_v, "uint64-convertible wrapper"); + static_assert(std::is_same_v, "a name"); +#undef LB +#undef UB +SYSIO_TEST_END + int main(int argc, char* argv[]) { bool verbose = false; SYSIO_TEST(duplicate_primary_key_rejected) SYSIO_TEST(foreign_code_handle_cannot_mutate) SYSIO_TEST(own_table_handle_passes_the_guard) + SYSIO_TEST(primary_bounds_accept_uint64_and_name) return has_failed(); } diff --git a/tests/unit/test_contracts/multi_index_tests.cpp b/tests/unit/test_contracts/multi_index_tests.cpp index 49d3a82b3..614baa658 100644 --- a/tests/unit/test_contracts/multi_index_tests.cpp +++ b/tests/unit/test_contracts/multi_index_tests.cpp @@ -392,15 +392,19 @@ namespace _test_multi_index table.emplace(payer, [&](auto& r) { r.owner = "bob"_n; r.sec = 20; }); table.emplace(payer, [&](auto& r) { r.owner = "charlie"_n; r.sec = 30; }); - // The bounds take a uint64_t, as they always have; a name primary key is passed - // through .value, exactly as before this change. - auto lb = table.lower_bound("bob"_n.value); + // A name goes straight to the bounds, as it already did to find/get/require_find. + auto lb = table.lower_bound("bob"_n); sysio::check(lb != table.end() && lb->owner == "bob"_n, - "name_pk_secondaries - lower_bound did not land on bob"); + "name_pk_secondaries - lower_bound(name) did not land on bob"); - auto ub = table.upper_bound("bob"_n.value); + auto ub = table.upper_bound("bob"_n); sysio::check(ub != table.end() && ub->owner == "charlie"_n, - "name_pk_secondaries - upper_bound did not land on charlie"); + "name_pk_secondaries - upper_bound(name) did not land on charlie"); + + // The uint64_t form is unchanged, and must agree with the name form. + auto lb_raw = table.lower_bound("bob"_n.value); + sysio::check(lb_raw != table.end() && lb_raw->owner == "bob"_n, + "name_pk_secondaries - lower_bound(uint64_t) regressed"); // The secondary index must resolve back to the name-keyed row: this is the path // to_pk_uint64 fixed. modify() rewrites the mapping, erase() removes it. From 6957c30241dcf999cb4946dafa5e9c55c6527e59 Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Tue, 1 Sep 2026 11:36:30 -0500 Subject: [PATCH 08/28] test(kv): give the receiving-account and success paths real teeth Three gaps found in pre-push review, all in the coverage this PR added. arrange() pointed both the sysio_contract_name global AND the mocked current_receiver at the same account, so the two branches of receiving_account() could not be told apart: deleting the global fast path outright left the whole suite green. The mock now returns a decoy account whenever the global is set, so a guard that consulted the intrinsic instead of the global reaches the wrong answer and the case fails. own_table_handle_passes_the_guard was a byte-identical copy of duplicate_primary_key_rejected -- its comment said pk=2 but the lambda wrote id=1 and it asserted the duplicate message. Nothing in the suite required a mutation to SUCCEED, so a guard that rejected everything satisfied every case. It now emplaces an absent key and asserts the row lands. That needs the iterator read path, since emplace returns find(pk); kv_it_key/kv_it_value are served from the mock store rather than stubbed, so the returned iterator is genuinely valid. The bounds doc claimed "two overloads keep every conversion the base performed, unchanged". False: a wrapper convertible to both uint64_t and name is now ambiguous, where against the single uint64_t parameter it chose the uint64_t conversion. find/get/require_find have always been ambiguous for such a type, so the overloads are consistent with their siblings rather than novel -- but it is a source break, so the claim is narrowed to name both costs and a dual_key case pins it. Verified by mutation. Deleting the duplicate check fails only duplicate_primary_key_rejected; deleting the three receiver guards fails only foreign_code_handle_cannot_mutate; deleting the dispatcher-global fast path fails duplicate_primary_key_rejected and own_table_handle_passes_the_guard. ctest 30/30, multi_index integration 24/24. Also from the same review: - context.hpp declared sysio_contract_name without the volatile its definition in sysiolib.cpp carries. Differing cv-qualification on one entity is ill-formed NDR; it linked only because extern "C" names carry no type and no TU saw both. This PR is the header's first consumer. - kv_table's do_insert comment claimed "no supported way to skip it", but store_secondaries, remove_secondaries, update_secondaries and do_erase are all still public and reach the same stranded mapping. Says what is true. --- .../contracts/sysio/kv_multi_index.hpp | 22 ++-- .../sysiolib/contracts/sysio/kv_table.hpp | 5 +- libraries/sysiolib/core/sysio/context.hpp | 7 +- tests/unit/kv_multi_index_tests.cpp | 115 +++++++++++++++--- 4 files changed, 124 insertions(+), 25 deletions(-) diff --git a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp index 7a28ab631..10a21ee4d 100644 --- a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp +++ b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp @@ -613,12 +613,20 @@ class kv_multi_index { /// converting-proxy parameter -- both were tried and both changed the argument's meaning. /// A template cannot deduce `lower_bound({42})`; a proxy accepts `lower_bound({w})` for a /// `w` converting to a narrower type, which a real `uint64_t` parameter rejects as - /// narrowing. Two overloads keep every conversion the base performed, unchanged. + /// narrowing. Here the parameter is still a `uint64_t`, so that conversion is the base's. /// - /// Like its three siblings, this makes `&table::lower_bound` an overload set, so the bare - /// address can no longer be taken -- as has always been true of `&table::find`, - /// `&table::get` and `&table::require_find`. Named overloads still resolve: - /// `static_cast(&table::lower_bound)`. + /// This adopts the sibling shape INCLUDING its two costs, neither of which is new to the + /// class but both of which are new to the bounds: + /// + /// - `&table::lower_bound` is now an overload set, so the bare address cannot be taken, + /// exactly as for `&table::find`, `&table::get` and `&table::require_find`. A named + /// cast still resolves either one: + /// `static_cast(&table::lower_bound)`. + /// - a wrapper convertible to BOTH `uint64_t` and `name` becomes ambiguous, where + /// against the single `uint64_t` parameter it selected the `uint64_t` conversion. + /// `find`/`get`/`require_find` have always been ambiguous for such a type, so this + /// makes the bounds consistent rather than introducing a new rule; it is called out + /// because it is a source break, and it is pinned by test. const_iterator lower_bound(name primary) const { return lower_bound(primary.value); } const_iterator lower_bound(uint64_t primary) const { auto key = make_pk(primary); @@ -669,8 +677,8 @@ class kv_multi_index { // this the row is silently overwritten AND store_secondaries -- an unconditional // kv_idx_store -- leaves the old (sec_key -> pri_key) mapping behind, pointing at a // row whose secondary value has changed. kv::table::emplace checks the same way. - sysio::check(!::kv_contains(_table_id, _code.value, key.data, key_size), - "object with the same primary key already exists"); + check(!::kv_contains(_table_id, _code.value, key.data, key_size), + "object with the same primary key already exists"); ::kv_set(_table_id, payer.value, key.data, key_size, value.data(), value.size()); store_secondaries(payer.value, obj); diff --git a/libraries/sysiolib/contracts/sysio/kv_table.hpp b/libraries/sysiolib/contracts/sysio/kv_table.hpp index dad7d07f8..edec8d1e7 100644 --- a/libraries/sysiolib/contracts/sysio/kv_table.hpp +++ b/libraries/sysiolib/contracts/sysio/kv_table.hpp @@ -442,7 +442,10 @@ class table_impl { // and, because store_secondaries is an unconditional kv_idx_store, either strands the old // (sec_key -> pri_key) mapping (when the secondary value changed) or trips the host's // ordered_unique constraint on (code, table_id, sec_key, pri_key). emplace() pays one - // kv_contains to make that unreachable; there is no supported way to skip it. + // kv_contains so no PUBLIC path reaches an unguarded insert. This seals do_insert only: + // store_secondaries/remove_secondaries/update_secondaries and do_erase below stay public, + // and calling store_secondaries directly still strands a mapping the same way. Sealing + // those is a wider change than this fix. void do_insert(uint64_t payer, const be_key_stream& k, const K& key, const V& value) { if constexpr (is_fixed_serializable_v) { char vbuf[sizeof(V)]; diff --git a/libraries/sysiolib/core/sysio/context.hpp b/libraries/sysiolib/core/sysio/context.hpp index c50845763..27b5ea131 100644 --- a/libraries/sysiolib/core/sysio/context.hpp +++ b/libraries/sysiolib/core/sysio/context.hpp @@ -4,8 +4,11 @@ namespace sysio { namespace internal_use_do_not_use { - extern "C" uint64_t sysio_contract_name; + /// volatile MUST match the definition in sysiolib.cpp -- differing cv-qualification on + /// the same entity is ill-formed (no diagnostic required). It links today only because + /// extern "C" names carry no type and no translation unit sees both spellings. + extern "C" volatile uint64_t sysio_contract_name; } - inline name current_context_contract() { return name{internal_use_do_not_use::sysio_contract_name}; } + inline name current_context_contract() { return name{uint64_t{internal_use_do_not_use::sysio_contract_name}}; } } diff --git a/tests/unit/kv_multi_index_tests.cpp b/tests/unit/kv_multi_index_tests.cpp index a89fa8edd..c318aebdc 100644 --- a/tests/unit/kv_multi_index_tests.cpp +++ b/tests/unit/kv_multi_index_tests.cpp @@ -31,6 +31,7 @@ #include #include #include +#include #include #include @@ -63,6 +64,21 @@ struct wrapped_key { constexpr operator uint64_t() const { return v; } // NOLINT(google-explicit-constructor) }; +/// Convertible to BOTH parameter types. Against a single uint64_t parameter this selected the +/// uint64_t conversion; against the overload pair it is ambiguous. Pinned so the documented +/// cost stays a documented cost rather than being rediscovered as a surprise. +struct dual_key { + constexpr operator uint64_t() const { return 3; } // NOLINT(google-explicit-constructor) + operator name() const { return "alice"_n; } // NOLINT(google-explicit-constructor) +}; + +/// Is `t.lower_bound(A)` well-formed? +template +struct callable_with : std::false_type {}; +template +struct callable_with().lower_bound(std::declval()))>> + : std::true_type {}; + constexpr uint32_t records_tid = sysio::kv::compute_table_id("records"_n.value); // Mirrors the asymmetry under test: kv_contains honours `code`, writes have no such @@ -71,9 +87,10 @@ struct mock_kv { using row_key = std::tuple; // code, table_id, key std::map rows; uint64_t receiver = 0; - uint32_t sets = 0; // must stay 0: a rejected mutation writes nothing + uint32_t sets = 0; // 0 unless a case expects the write to be allowed + std::string it_key; // key the one live iterator was positioned at - void reset(uint64_t who) { rows.clear(); receiver = who; sets = 0; } + void reset(uint64_t who) { rows.clear(); receiver = who; sets = 0; it_key.clear(); } }; mock_kv& store() { static mock_kv inst; return inst; } @@ -96,23 +113,75 @@ void install_intrinsics() { return store().rows.count(mock_kv::row_key{code, table_id, k}) ? 1 : 0; }); - // No code parameter: a write always lands on the receiver. Counted, never expected. + // No code parameter: a write always lands on the receiver, never on the handle's code. + // Recorded under store().receiver so a misdirected write is observable as a row under the + // wrong account, not merely as a count. intrinsics::set_intrinsic( - [](uint32_t, uint64_t, const void*, uint32_t, const void*, uint32_t) -> int64_t { + [](uint32_t table_id, uint64_t, const void* key, uint32_t key_size, + const void* val, uint32_t val_size) -> int64_t { ++store().sets; + store().rows[mock_kv::row_key{store().receiver, table_id, + std::string(static_cast(key), key_size)}] = + std::string(static_cast(val), val_size); + return 0; + }); + + // emplace() returns find(pk), so a write that is allowed through walks the iterator path. + // Exactly one iterator is ever live in these cases, so remembering the key it was + // positioned at is enough to serve a real key/value pair rather than a stub -- the + // returned iterator is genuinely valid, not merely non-crashing. + intrinsics::set_intrinsic( + [](uint32_t, capi_name, const void*, uint32_t) -> uint32_t { return 1; }); + intrinsics::set_intrinsic([](uint32_t) {}); + intrinsics::set_intrinsic([](uint32_t) -> int32_t { return 0; }); + intrinsics::set_intrinsic( + [](uint32_t, const void* key, uint32_t key_size) -> int32_t { + store().it_key.assign(static_cast(key), key_size); return 0; }); + + // Serve from the store, so a key or value the contract never wrote cannot be read back. + auto serve = [](const std::string& src, uint32_t offset, void* dest, uint32_t dest_size, + uint32_t* actual_size) -> int32_t { + if (offset > src.size()) return -1; + *actual_size = static_cast(src.size() - offset); + const uint32_t n = *actual_size < dest_size ? *actual_size : dest_size; + std::memcpy(dest, src.data() + offset, n); + return 0; + }; + intrinsics::set_intrinsic( + [serve](uint32_t, uint32_t off, void* d, uint32_t ds, uint32_t* as) -> int32_t { + return serve(store().it_key, off, d, ds, as); + }); + intrinsics::set_intrinsic( + [serve](uint32_t, uint32_t off, void* d, uint32_t ds, uint32_t* as) -> int32_t { + auto it = store().rows.find(mock_kv::row_key{store().receiver, records_tid, + store().it_key}); + if (it == store().rows.end()) return -1; + return serve(it->second, off, d, ds, as); + }); } -/// Seed the receiver's own table with pk, and install the mocks. +/// The account whose table is under test is never this one. When the dispatcher-global path +/// is being exercised, the current_receiver intrinsic is pointed here instead, so the two +/// branches of receiving_account() cannot return the same answer. +constexpr uint64_t decoy_receiver = "carol"_n.value; + +/// Seed `owner`'s table with pk, and install the mocks. +/// /// @param dispatcher_sets_name mirrors the generated dispatcher recording the receiver in -/// the sysio_contract_name global; false leaves it 0, as SYSIO_DISPATCH and the -/// native dispatch do, exercising the current_receiver() fallback instead. -void arrange(uint64_t receiver, uint64_t scope, uint64_t pk, bool dispatcher_sets_name) { - store().reset(receiver); - store().rows[mock_kv::row_key{receiver, records_tid, pk_key(scope, pk)}] = "row"; +/// the sysio_contract_name global. When true, the mocked current_receiver +/// deliberately returns decoy_receiver rather than `owner`: a guard that consulted +/// the intrinsic instead of the global would then get the wrong account and the case +/// would fail. Pointing both at `owner` -- as this did originally -- makes the two +/// branches indistinguishable, and deleting the global fast path leaves the suite +/// green. When false the global is 0, as SYSIO_DISPATCH and the native dispatch leave +/// it, and the intrinsic is the only source. +void arrange(uint64_t owner, uint64_t scope, uint64_t pk, bool dispatcher_sets_name) { + store().reset(dispatcher_sets_name ? decoy_receiver : owner); + store().rows[mock_kv::row_key{owner, records_tid, pk_key(scope, pk)}] = "row"; install_intrinsics(); - sysio_set_contract_name(dispatcher_sets_name ? receiver : 0); + sysio_set_contract_name(dispatcher_sets_name ? owner : 0); } } // namespace @@ -158,10 +227,13 @@ SYSIO_TEST_BEGIN(own_table_handle_passes_the_guard) arrange("alice"_n.value, "alice"_n.value, 1, via_global); table_t t("alice"_n, "alice"_n.value); - // pk=2 is absent, so the guard and the duplicate probe both pass and the message, - // if any, is not one of the three rejections. - CHECK_ASSERT( "object with the same primary key already exists", - ([&]() { t.emplace("alice"_n, [](auto& o) { o.id = 1; o.sec = 7; }); }) ) + // pk=2 is absent, so neither the receiver guard nor the duplicate probe fires and the + // write goes through. Without a case that SUCCEEDS, a guard that rejected every + // mutation would satisfy the entire suite. + t.emplace("alice"_n, [](auto& o) { o.id = 2; o.sec = 7; }); + CHECK_EQUAL( store().sets, 1u ) + CHECK_EQUAL( store().rows.count(mock_kv::row_key{store().receiver, records_tid, + pk_key("alice"_n.value, 2)}), 1u ) } SYSIO_TEST_END @@ -189,10 +261,23 @@ SYSIO_TEST_BEGIN(primary_bounds_accept_uint64_and_name) #define UB(expr) decltype(std::declval().upper_bound expr) static_assert(std::is_same_v, "uint64_t"); static_assert(std::is_same_v, "a name"); + // The braced-literal case is the point of the assertion, so the diagnostic it provokes is + // suppressed rather than avoided. +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wbraced-scalar-init" static_assert(std::is_same_v, "braced literal"); +#pragma clang diagnostic pop static_assert(std::is_same_v, "empty brace, key zero"); static_assert(std::is_same_v, "uint64-convertible wrapper"); static_assert(std::is_same_v, "a name"); + + // The documented costs. A dual-convertible wrapper is ambiguous here, as it already was + // for find/get/require_find -- consistent with the siblings, but a source break against + // the single uint64_t parameter, so it is pinned rather than left to be rediscovered. + static_assert(callable_with::value && callable_with::value && + callable_with::value, "the accepted domain must stay callable"); + static_assert(!callable_with::value, + "a uint64_t-and-name-convertible wrapper is ambiguous, as it is for find()"); #undef LB #undef UB SYSIO_TEST_END From 4092b36810a59f73aa8ef078df757d858c485bad Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Tue, 1 Sep 2026 12:55:15 -0500 Subject: [PATCH 09/28] test(kv): assert the receiver's row by value, not by key count Pre-push review found the foreign-code case could not detect the corruption its own comment described. It asserted `rows.count({alice, tid, pk_key(alice,1)}) == 1`, but a misdirected emplace OVERWRITES the row under that same key, so the count is 1 either way. The property was carried entirely by the `sets == 0` line above it. Now compares the stored value: with the emplace guard deleted and the sets assertion removed, the case fails with `store().rows.at(seeded) != std::string("row")`. The positive case gets the same treatment -- the row it writes must be non-empty and must not be the seeded placeholder, so a write that landed with the wrong key or wrong contents is not read as success. Also documents why emplace's closing find() returns end() on the global-path iteration: the write lands under the decoy receiver while the handle's code is alice, so kv_contains short-circuits. That asymmetry is the point of the decoy, but the previous comment implied both iterations behaved alike. The review reported kv_it_value as dead code; it is not. Removing it aborts three of the four cases with "unsupported intrinsic" -- emplace's closing find() builds an iterator whose load_current() reads the key and then the value. Kept, with a comment saying what reaches it. ctest 30/30, multi_index integration 24/24. --- tests/unit/kv_multi_index_tests.cpp | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/tests/unit/kv_multi_index_tests.cpp b/tests/unit/kv_multi_index_tests.cpp index c318aebdc..50f30d929 100644 --- a/tests/unit/kv_multi_index_tests.cpp +++ b/tests/unit/kv_multi_index_tests.cpp @@ -153,6 +153,9 @@ void install_intrinsics() { [serve](uint32_t, uint32_t off, void* d, uint32_t ds, uint32_t* as) -> int32_t { return serve(store().it_key, off, d, ds, as); }); + // Reached: emplace's closing find() constructs an iterator, whose load_current() reads the + // key and then the value. Serving from the store rather than stubbing means a row that + // landed under the wrong key cannot be read back as if it were correct. intrinsics::set_intrinsic( [serve](uint32_t, uint32_t off, void* d, uint32_t ds, uint32_t* as) -> int32_t { auto it = store().rows.find(mock_kv::row_key{store().receiver, records_tid, @@ -214,10 +217,14 @@ SYSIO_TEST_BEGIN(foreign_code_handle_cannot_mutate) CHECK_ASSERT( "cannot erase objects in table of another contract", ([&]() { foreign.erase(r); }) ) - // The whole point: the receiver's row was never touched. + // The whole point: the receiver's row was never touched. Its VALUE is what carries + // that -- a misdirected emplace overwrites the row under the same key, so a count() + // of 1 would hold either way and prove nothing. CHECK_EQUAL( store().sets, 0u ) - CHECK_EQUAL( store().rows.count(mock_kv::row_key{"alice"_n.value, records_tid, - pk_key("alice"_n.value, 1)}), 1u ) + const auto seeded = mock_kv::row_key{"alice"_n.value, records_tid, + pk_key("alice"_n.value, 1)}; + CHECK_EQUAL( store().rows.count(seeded), 1u ) + CHECK_EQUAL( store().rows.at(seeded), std::string("row") ) } SYSIO_TEST_END @@ -230,10 +237,20 @@ SYSIO_TEST_BEGIN(own_table_handle_passes_the_guard) // pk=2 is absent, so neither the receiver guard nor the duplicate probe fires and the // write goes through. Without a case that SUCCEEDS, a guard that rejected every // mutation would satisfy the entire suite. + // + // The write lands under store().receiver, which the mock deliberately makes the decoy + // account on the global-path iteration -- that is the asymmetry under test, and it is + // also why emplace's closing find() returns end() there: it probes _code, which the + // decoy is not. Nothing here depends on the returned iterator. t.emplace("alice"_n, [](auto& o) { o.id = 2; o.sec = 7; }); CHECK_EQUAL( store().sets, 1u ) - CHECK_EQUAL( store().rows.count(mock_kv::row_key{store().receiver, records_tid, - pk_key("alice"_n.value, 2)}), 1u ) + const auto written = mock_kv::row_key{store().receiver, records_tid, + pk_key("alice"_n.value, 2)}; + CHECK_EQUAL( store().rows.count(written), 1u ) + // Not merely present: the row must be the one this emplace serialized, so a write + // that landed with the wrong key or wrong contents is not mistaken for success. + CHECK_EQUAL( store().rows.at(written).empty(), false ) + CHECK_EQUAL( store().rows.at(written) == std::string("row"), false ) } SYSIO_TEST_END From 31961f3c30ba00b4c8acc778bad83548414030d0 Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Tue, 1 Sep 2026 14:08:27 -0500 Subject: [PATCH 10/28] docs(kv): stop calling the shim a drop-in replacement in the header The same overstatement the docs already dropped. Names the three real divergences instead: deleted postfix iterator operators, uint64_t/name bound overloads rather than upstream's member template, and the trivially-copyable secondary-key constraint. --- libraries/sysiolib/contracts/sysio/kv_multi_index.hpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp index 10a21ee4d..832904f5f 100644 --- a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp +++ b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp @@ -142,7 +142,11 @@ namespace _kv_multi_index_detail { } // namespace _kv_multi_index_detail // Uses sysio::indexed_by and sysio::const_mem_fun from the standard CDT headers. -// This class is a drop-in replacement: just change multi_index -> kv_multi_index. +// +// Source-compatible with the EOSIO multi_index, not identical to it. The known divergences, +// all documented in docs/kv-multi-index.md: the postfix iterator operators are deleted (a copy +// duplicates a host-side handle), the primary bounds are uint64_t/name overloads rather than a +// member template, and a secondary key must be trivially copyable. template class kv_multi_index { From 7f1f900261e051d366a26f442e5caf55ef77c584 Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Tue, 1 Sep 2026 14:32:23 -0500 Subject: [PATCH 11/28] test(kv): pin that the dispatcher records the receiver, not the code Final review round found that nothing in the tree pins the one fact receiving_account() depends on. Changing cdt-codegen.cpp:83 from sysio_set_contract_name(r) to (c) leaves ctest 30/30 and the integration suite 24/24 green: every in-tree action is self-sent, so r == c, and the native test drives the global directly rather than through apply(). The divergence appears only under notification, on chain -- and the one downstream contract that would catch it, wire-sysio's ram_restrictions_test, is not rebuilt by that repo's CI (SYSIO_BUILD_TEST_CONTRACTS: "OFF"). dispatch_receiver_tests.sh inspects the emitted dispatch text, which no other test looks at: it asserts the call passes `r`, and that it precedes any action dispatch. Registered under unit_tests so it runs in required CI. Verified to gate: with the argument changed to `c` it fails and everything else still passes. Also from the same review: - The header comment added in the previous commit cited docs/kv-multi-index.md for the divergences, but this PR touches no docs -- those edits are on the #111 branch, and on THIS branch that file still calls the shim a drop-in replacement. The divergences are listed inline instead, and the comment now records that sysio::multi_index and sysio::singleton are both aliases of this template, so the new guards reach the singleton surface too. - The foreign-code case's `rows.count(seeded) == 1` cannot fail: the mock installs no kv_erase and kv_set only assigns, so nothing can reduce the count. The value comparison beside it is what carries the property; the comment said otherwise and now says which is which. --- .../contracts/sysio/kv_multi_index.hpp | 12 ++- tests/CMakeLists.txt | 4 + tests/unit/dispatch_receiver_tests.sh | 76 +++++++++++++++++++ tests/unit/kv_multi_index_tests.cpp | 7 +- 4 files changed, 92 insertions(+), 7 deletions(-) create mode 100755 tests/unit/dispatch_receiver_tests.sh diff --git a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp index 832904f5f..3e2306a97 100644 --- a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp +++ b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp @@ -143,10 +143,14 @@ namespace _kv_multi_index_detail { // Uses sysio::indexed_by and sysio::const_mem_fun from the standard CDT headers. // -// Source-compatible with the EOSIO multi_index, not identical to it. The known divergences, -// all documented in docs/kv-multi-index.md: the postfix iterator operators are deleted (a copy -// duplicates a host-side handle), the primary bounds are uint64_t/name overloads rather than a -// member template, and a secondary key must be trivially copyable. +// Source-compatible with the EOSIO multi_index, not identical to it. The known divergences: +// the postfix iterator operators are deleted (a copy duplicates a host-side handle), the +// primary bounds are uint64_t/name overloads rather than a member template, a secondary key +// must be trivially copyable, and -- as of this change -- emplace rejects a duplicate primary +// key while emplace/modify/erase reject a handle opened on another account. +// +// sysio::multi_index and sysio::singleton are both aliases of this template, so every one of +// those applies to them too. template class kv_multi_index { diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 98b2d489c..48f8401fe 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -37,6 +37,10 @@ configure_file(${CMAKE_CURRENT_SOURCE_DIR}/unit/abi_version_tests.sh ${CMAKE_BIN add_test(NAME abi_version_tests COMMAND ${CMAKE_BINARY_DIR}/tests/unit/abi_version_tests.sh "${CMAKE_BINARY_DIR}" WORKING_DIRECTORY ${CMAKE_BINARY_DIR}) set_property(TEST abi_version_tests PROPERTY LABELS unit_tests) +configure_file(${CMAKE_CURRENT_SOURCE_DIR}/unit/dispatch_receiver_tests.sh ${CMAKE_BINARY_DIR}/tests/unit/dispatch_receiver_tests.sh COPYONLY) +add_test(NAME dispatch_receiver_tests COMMAND ${CMAKE_BINARY_DIR}/tests/unit/dispatch_receiver_tests.sh "${CMAKE_BINARY_DIR}" WORKING_DIRECTORY ${CMAKE_BINARY_DIR}) +set_property(TEST dispatch_receiver_tests PROPERTY LABELS unit_tests) + configure_file(${CMAKE_CURRENT_SOURCE_DIR}/unit/multidir_contract_tests.sh ${CMAKE_BINARY_DIR}/tests/unit/multidir_contract_tests.sh COPYONLY) add_test(NAME multidir_contract_tests COMMAND ${CMAKE_BINARY_DIR}/tests/unit/multidir_contract_tests.sh "${CMAKE_BINARY_DIR}" WORKING_DIRECTORY ${CMAKE_BINARY_DIR}) set_property(TEST multidir_contract_tests PROPERTY LABELS unit_tests) diff --git a/tests/unit/dispatch_receiver_tests.sh b/tests/unit/dispatch_receiver_tests.sh new file mode 100755 index 000000000..18c1df534 --- /dev/null +++ b/tests/unit/dispatch_receiver_tests.sh @@ -0,0 +1,76 @@ +#!/bin/bash +# The generated apply() must record the RECEIVER in sysio_contract_name. +# +# multi_index's receiving_account() reads that global and falls back to the current_receiver +# intrinsic only when it is 0, so the guards on emplace/modify/erase are only correct if the +# dispatcher stores `r` (the receiver) rather than `c` (the code). Every in-tree action is +# self-sent, so r == c and the entire unit + integration suite stays green if that argument is +# changed -- the divergence appears only under notification, on chain. This pins it at the +# source: the emitted dispatch text, which no other test inspects. +# +# Usage: dispatch_receiver_tests.sh +set -euo pipefail + +BUILD_DIR="$1" +CDT_CPP="${BUILD_DIR}/bin/cdt-cpp" +PASS=0 +FAIL=0 +pass() { echo " PASS: $1"; PASS=$((PASS + 1)); } +fail() { echo " FAIL: $1"; FAIL=$((FAIL + 1)); } + +WORK="$(mktemp -d)" +trap 'rm -rf "$WORK"' EXIT + +cat > "${WORK}/c.cpp" <<'EOF' +#include +class [[sysio::contract("dispatchrcv")]] dispatchrcv : public sysio::contract { +public: + using contract::contract; + [[sysio::action]] void go() {} + [[sysio::on_notify("sysio.token::transfer")]] void onxfer(sysio::name from, sysio::name to) {} +}; +EOF + +echo "=== Dispatch Receiver Tests ===" + +if ! ( cd "$WORK" && "$CDT_CPP" -abigen -abigen_output=c.abi -contract=dispatchrcv \ + -o c.wasm c.cpp ) > "${WORK}/build.log" 2>&1; then + fail "contract builds" + sed 's/^/ /' "${WORK}/build.log" + echo "Results: ${PASS} passed, ${FAIL} failed" + exit 1 +fi +pass "contract builds" + +DISPATCH="$(find "$WORK" -name '*.dispatch.cpp' | head -1)" +if [ -z "$DISPATCH" ]; then + fail "a dispatch.cpp was generated" + echo "Results: ${PASS} passed, ${FAIL} failed" + exit 1 +fi +pass "a dispatch.cpp was generated" + +# apply(uint64_t r, uint64_t c, uint64_t a): the receiver is the FIRST parameter. +if grep -qE 'sysio_set_contract_name\(\s*r\s*\)' "$DISPATCH"; then + pass "apply() records the receiver, not the code" +else + fail "apply() records the receiver, not the code" + echo " expected: sysio_set_contract_name(r)" + grep -n "sysio_set_contract_name" "$DISPATCH" | sed 's/^/ got: /' || echo " (no call at all)" +fi + +# It must run before any action is dispatched, or a guard could read a stale value. +line_set="$(grep -n 'sysio_set_contract_name(' "$DISPATCH" | tail -1 | cut -d: -f1 || true)" +line_apply="$(grep -n 'void apply' "$DISPATCH" | head -1 | cut -d: -f1 || true)" +line_first_action="$(grep -nE 'sysio_wasm_action|executed|action_wrapper|::go' "$DISPATCH" | awk -F: -v s="${line_set:-0}" '$1 > s {print $1; exit}' || true)" +if [ -n "$line_set" ] && [ -n "$line_apply" ] && [ "$line_set" -gt "$line_apply" ] \ + && { [ -z "$line_first_action" ] || [ "$line_set" -lt "$line_first_action" ]; }; then + pass "the receiver is recorded before any action runs" +else + fail "the receiver is recorded before any action runs" + echo " apply at ${line_apply:-?}, set at ${line_set:-?}, first action at ${line_first_action:-none}" +fi + +echo "" +echo "Results: ${PASS} passed, ${FAIL} failed" +[ "$FAIL" -eq 0 ] diff --git a/tests/unit/kv_multi_index_tests.cpp b/tests/unit/kv_multi_index_tests.cpp index 50f30d929..70ffbcf2b 100644 --- a/tests/unit/kv_multi_index_tests.cpp +++ b/tests/unit/kv_multi_index_tests.cpp @@ -217,9 +217,10 @@ SYSIO_TEST_BEGIN(foreign_code_handle_cannot_mutate) CHECK_ASSERT( "cannot erase objects in table of another contract", ([&]() { foreign.erase(r); }) ) - // The whole point: the receiver's row was never touched. Its VALUE is what carries - // that -- a misdirected emplace overwrites the row under the same key, so a count() - // of 1 would hold either way and prove nothing. + // The whole point: the receiver's row was never touched. The VALUE comparison is what + // carries that -- a misdirected emplace overwrites the row under the same key, so the + // count() below cannot fall to 0 and proves nothing on its own (the mock installs no + // kv_erase, and kv_set only assigns). It is kept as a precondition for the .at(). CHECK_EQUAL( store().sets, 0u ) const auto seeded = mock_kv::row_key{"alice"_n.value, records_tid, pk_key("alice"_n.value, 1)}; From 4e8248d27ae67dca0ebc83bb1ba6cb7178b5c1b1 Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Wed, 2 Sep 2026 09:31:41 -0500 Subject: [PATCH 12/28] test(kv): make the ordering assertion real, and cover allowed modify/erase Three review comments, all correct. The dispatch ordering assertion was tautological. It grepped for `sysio_wasm_action|executed|action_wrapper|::go`, none of which the generator emits -- the real call sites are `pre_dispatch(`, `__sysio_action_*(` and `__sysio_notify_*(` -- so the "first dispatch" line came back empty and the test treated empty as success. The awk filter also searched only after the setter's own line, which made any match later by construction. Moving sysio_set_contract_name below every handler would have passed. It now locates the first real call site after apply(), requires it to be non-empty, requires exactly one setter inside apply(), and asserts the setter precedes it. Verified by moving the emission below all dispatch: the test fails with "setter at line 28, first dispatch at line 14". The positive native case proved only that emplace is ALLOWED. modify and erase were covered in the rejecting direction by the foreign-code case, but their successful paths ran only in the opt-in integration suite -- so an inverted or unconditional guard on either left every required test green. own_table_handle_can_modify_and_erase exercises both on an owned handle, with a kv_erase mock, asserting the row is rewritten (not merely present, and not the seeded placeholder) and then removed. Verified: inverting either guard fails this case and nothing else. The header comment claimed sysio::singleton is an alias of this template. It is not -- it aliases kv_singleton, which holds a kv_multi_index as a private member and exposes only get/set/remove/get_or_create, so it inherits the mutation guards but none of the iterator/bounds/secondary-key divergences. The comment also lumped those two groups together, when they are opposite in kind: the divergences are permanent source breaks against upstream, while the duplicate and receiver rejections RESTORE upstream behaviour and are a change only against earlier Wire CDT. Split accordingly, and "source-compatible" dropped since the same comment documents the breaks. The reverse-iterator carve-out is recorded there too. ctest 31/31, multi_index integration 24/24. --- .../contracts/sysio/kv_multi_index.hpp | 31 ++++++++++--- tests/unit/dispatch_receiver_tests.sh | 36 +++++++++++---- tests/unit/kv_multi_index_tests.cpp | 45 ++++++++++++++++++- 3 files changed, 95 insertions(+), 17 deletions(-) diff --git a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp index 3e2306a97..cb315d214 100644 --- a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp +++ b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp @@ -143,14 +143,31 @@ namespace _kv_multi_index_detail { // Uses sysio::indexed_by and sysio::const_mem_fun from the standard CDT headers. // -// Source-compatible with the EOSIO multi_index, not identical to it. The known divergences: -// the postfix iterator operators are deleted (a copy duplicates a host-side handle), the -// primary bounds are uint64_t/name overloads rather than a member template, a secondary key -// must be trivially copyable, and -- as of this change -- emplace rejects a duplicate primary -// key while emplace/modify/erase reject a handle opened on another account. +// A shim for the EOSIO multi_index over a different store. Nearly all contract code carries +// over, but two distinct things are worth keeping apart. // -// sysio::multi_index and sysio::singleton are both aliases of this template, so every one of -// those applies to them too. +// WHERE THIS DIVERGES FROM UPSTREAM -- permanent, and each is a source break against upstream +// code: +// - the postfix iterator operators are deleted, because copying a KV iterator duplicates a +// host-side handle. Note rbegin()/rend() hand back a std::reverse_iterator, whose postfix +// operators are the adaptor's and are NOT deleted, so reverse loops compile silently; +// - the primary bounds are uint64_t/name overloads rather than upstream's member template, +// so &table::lower_bound cannot be taken bare and a wrapper convertible to both is +// ambiguous (see the note at the bounds themselves); +// - a secondary key must be trivially copyable, enforced by a static_assert in +// secondary_index_view -- so it fires at get_index<...>(), not at declaration. +// +// WHERE THIS CHANGED TO MATCH UPSTREAM -- as of this commit, and a behaviour change only +// against EARLIER WIRE CDT, not against upstream: +// - emplace rejects a duplicate primary key; +// - emplace/modify/erase reject a handle whose code is not the receiving account. +// +// sysio::multi_index is a direct alias of this template, so all of the above applies to it. +// sysio::singleton is NOT: it aliases kv_singleton, which holds a kv_multi_index as a PRIVATE +// member and exposes only get/set/remove/get_or_create. Its mutators funnel through the ones +// above, so it inherits the two behaviour changes -- a singleton handle constructed on another +// account is now read-only -- but none of the divergences, which are not reachable through its +// API. template class kv_multi_index { diff --git a/tests/unit/dispatch_receiver_tests.sh b/tests/unit/dispatch_receiver_tests.sh index 18c1df534..6610dd40d 100755 --- a/tests/unit/dispatch_receiver_tests.sh +++ b/tests/unit/dispatch_receiver_tests.sh @@ -59,16 +59,34 @@ else grep -n "sysio_set_contract_name" "$DISPATCH" | sed 's/^/ got: /' || echo " (no call at all)" fi -# It must run before any action is dispatched, or a guard could read a stale value. -line_set="$(grep -n 'sysio_set_contract_name(' "$DISPATCH" | tail -1 | cut -d: -f1 || true)" -line_apply="$(grep -n 'void apply' "$DISPATCH" | head -1 | cut -d: -f1 || true)" -line_first_action="$(grep -nE 'sysio_wasm_action|executed|action_wrapper|::go' "$DISPATCH" | awk -F: -v s="${line_set:-0}" '$1 > s {print $1; exit}' || true)" -if [ -n "$line_set" ] && [ -n "$line_apply" ] && [ "$line_set" -gt "$line_apply" ] \ - && { [ -z "$line_first_action" ] || [ "$line_set" -lt "$line_first_action" ]; }; then - pass "the receiver is recorded before any action runs" +# It must run before anything is dispatched, or a guard could read a stale value. +# +# Match the names the generator actually emits -- `pre_dispatch(`, `__sysio_action_*(` and +# `__sysio_notify_*(`. An earlier version of this test grepped for names that appear nowhere in +# the output, so the "first dispatch" line came back empty and the comparison was skipped as a +# pass: moving the setter below every handler would have satisfied it. The search deliberately +# does NOT start from the setter's line, which would make any match tautologically later. +apply_line="$(grep -nE '^\s*(__attribute__.*)?void apply\(' "$DISPATCH" | head -1 | cut -d: -f1 || true)" +first_dispatch="$(awk -v a="${apply_line:-0}" \ + 'NR > a && /(pre_dispatch\(|__sysio_(action|notify)_[A-Za-z0-9_]*\()/ { print NR; exit }' "$DISPATCH")" +setter_lines="$(grep -nE 'sysio_set_contract_name\(' "$DISPATCH" | awk -F: -v a="${apply_line:-0}" '$1 > a {print $1}')" +setter_count="$(printf '%s\n' "$setter_lines" | grep -c . || true)" + +if [ -z "$apply_line" ]; then + fail "apply() is defined in the generated dispatch" +elif [ -z "$first_dispatch" ]; then + fail "the generated apply() dispatches to a handler" + echo " no pre_dispatch/__sysio_action_*/__sysio_notify_* call found after line ${apply_line}" + sed 's/^/ /' "$DISPATCH" +elif [ "$setter_count" -ne 1 ]; then + fail "apply() records the receiver exactly once" + echo " found ${setter_count} call(s) inside apply(), expected 1" +elif [ "$setter_lines" -lt "$first_dispatch" ]; then + pass "the receiver is recorded before anything is dispatched" else - fail "the receiver is recorded before any action runs" - echo " apply at ${line_apply:-?}, set at ${line_set:-?}, first action at ${line_first_action:-none}" + fail "the receiver is recorded before anything is dispatched" + echo " setter at line ${setter_lines}, first dispatch at line ${first_dispatch}" + sed 's/^/ /' "$DISPATCH" fi echo "" diff --git a/tests/unit/kv_multi_index_tests.cpp b/tests/unit/kv_multi_index_tests.cpp index 70ffbcf2b..c47967137 100644 --- a/tests/unit/kv_multi_index_tests.cpp +++ b/tests/unit/kv_multi_index_tests.cpp @@ -89,8 +89,9 @@ struct mock_kv { uint64_t receiver = 0; uint32_t sets = 0; // 0 unless a case expects the write to be allowed std::string it_key; // key the one live iterator was positioned at + uint32_t erases = 0; - void reset(uint64_t who) { rows.clear(); receiver = who; sets = 0; it_key.clear(); } + void reset(uint64_t who) { rows.clear(); receiver = who; sets = 0; erases = 0; it_key.clear(); } }; mock_kv& store() { static mock_kv inst; return inst; } @@ -126,6 +127,15 @@ void install_intrinsics() { return 0; }); + // No code parameter here either: an erase always lands on the receiver. + intrinsics::set_intrinsic( + [](uint32_t table_id, const void* key, uint32_t key_size) -> int64_t { + ++store().erases; + store().rows.erase(mock_kv::row_key{store().receiver, table_id, + std::string(static_cast(key), key_size)}); + return 0; + }); + // emplace() returns find(pk), so a write that is allowed through walks the iterator path. // Exactly one iterator is ever live in these cases, so remembering the key it was // positioned at is enough to serve a real key/value pair rather than a stub -- the @@ -255,6 +265,38 @@ SYSIO_TEST_BEGIN(own_table_handle_passes_the_guard) } SYSIO_TEST_END +// modify and erase must also SUCCEED on an owned handle. Without these, an inverted or +// unconditional guard on either would satisfy every required test: the foreign-code case proves +// only that they reject, and their allowed paths ran solely in the opt-in integration suite. +SYSIO_TEST_BEGIN(own_table_handle_can_modify_and_erase) + for (bool via_global : {true, false}) { + arrange("alice"_n.value, "alice"_n.value, 1, via_global); + table_t t("alice"_n, "alice"_n.value); + + // The mock seeds the row under the table's OWNER; writes land under the receiver, which + // on the global-path iteration is the decoy. Address each by the account that holds it. + const auto owned = mock_kv::row_key{"alice"_n.value, records_tid, pk_key("alice"_n.value, 1)}; + const auto written = mock_kv::row_key{store().receiver, records_tid, pk_key("alice"_n.value, 1)}; + + record r{1, 7}; + t.modify(r, "alice"_n, [](auto& o) { o.sec = 9; }); + CHECK_EQUAL( store().sets, 1u ) + // Serialized, not the seeded placeholder -- so a modify that wrote nothing, or wrote the + // wrong key, is not read as success. + CHECK_EQUAL( store().rows.count(written), 1u ) + CHECK_EQUAL( store().rows.at(written) == std::string("row"), false ) + + // erase() removes the row it addresses. It is keyed the same way kv_set is, so it lands + // on the receiver too. + t.erase(r); + CHECK_EQUAL( store().erases, 1u ) + CHECK_EQUAL( store().rows.count(written), 0u ) + // The owner's seeded row is untouched on the decoy iteration, gone on the fallback one + // where receiver == owner -- either way the erase hit exactly the namespace it wrote to. + CHECK_EQUAL( store().rows.count(owned), via_global ? 1u : 0u ) + } +SYSIO_TEST_END + // The primary bounds take a `name` as well as a uint64_t, matching the two-overload shape // find/require_find/get have always used. Compile-time only -- nothing here is evaluated. SYSIO_TEST_BEGIN(primary_bounds_accept_uint64_and_name) @@ -305,6 +347,7 @@ int main(int argc, char* argv[]) { SYSIO_TEST(duplicate_primary_key_rejected) SYSIO_TEST(foreign_code_handle_cannot_mutate) SYSIO_TEST(own_table_handle_passes_the_guard) + SYSIO_TEST(own_table_handle_can_modify_and_erase) SYSIO_TEST(primary_bounds_accept_uint64_and_name) return has_failed(); } From c52cc9d3157981bc73b206c27c4e20d577215d89 Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Wed, 2 Sep 2026 09:37:56 -0500 Subject: [PATCH 13/28] docs(kv): describe the class, not the changes made to it The header comments had drifted into narrating their own diff -- "as of this commit", "a behaviour change only against earlier Wire CDT", "both were tried", "is now an overload set", "a wider change than this fix". A header should say what the class does today; the history belongs in the log. Dropped the WHERE-THIS-CHANGED section entirely. The duplicate-key and receiver rejections are stated once as what the mutators do, with each guard explained where it stands. The bounds comment now describes the overload pair and its two consequences directly, rather than which alternatives were attempted and rejected. The duplicate-key guard leads with the mechanism -- kv_set upserts, store_secondaries is unconditional -- and mentions db_store_i64 only as the reason the wrapper carries the check the KV intrinsics do not. kv_table's do_insert note states which members are sealed and which are not. No behaviour change; comments only. ctest 31/31. --- .../contracts/sysio/kv_multi_index.hpp | 64 +++++++++---------- .../sysiolib/contracts/sysio/kv_table.hpp | 9 +-- 2 files changed, 34 insertions(+), 39 deletions(-) diff --git a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp index cb315d214..1a40b044a 100644 --- a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp +++ b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp @@ -144,12 +144,10 @@ namespace _kv_multi_index_detail { // Uses sysio::indexed_by and sysio::const_mem_fun from the standard CDT headers. // // A shim for the EOSIO multi_index over a different store. Nearly all contract code carries -// over, but two distinct things are worth keeping apart. +// over; these are the places it does not, each a source break against upstream code: // -// WHERE THIS DIVERGES FROM UPSTREAM -- permanent, and each is a source break against upstream -// code: // - the postfix iterator operators are deleted, because copying a KV iterator duplicates a -// host-side handle. Note rbegin()/rend() hand back a std::reverse_iterator, whose postfix +// host-side handle. rbegin()/rend() hand back a std::reverse_iterator, whose postfix // operators are the adaptor's and are NOT deleted, so reverse loops compile silently; // - the primary bounds are uint64_t/name overloads rather than upstream's member template, // so &table::lower_bound cannot be taken bare and a wrapper convertible to both is @@ -157,17 +155,14 @@ namespace _kv_multi_index_detail { // - a secondary key must be trivially copyable, enforced by a static_assert in // secondary_index_view -- so it fires at get_index<...>(), not at declaration. // -// WHERE THIS CHANGED TO MATCH UPSTREAM -- as of this commit, and a behaviour change only -// against EARLIER WIRE CDT, not against upstream: -// - emplace rejects a duplicate primary key; -// - emplace/modify/erase reject a handle whose code is not the receiving account. +// The mutators reject a duplicate primary key and a handle whose code is not the receiving +// account, matching upstream. Each guard is documented where it stands. // -// sysio::multi_index is a direct alias of this template, so all of the above applies to it. -// sysio::singleton is NOT: it aliases kv_singleton, which holds a kv_multi_index as a PRIVATE -// member and exposes only get/set/remove/get_or_create. Its mutators funnel through the ones -// above, so it inherits the two behaviour changes -- a singleton handle constructed on another -// account is now read-only -- but none of the divergences, which are not reachable through its -// API. +// sysio::multi_index is a direct alias of this template. sysio::singleton is not: it aliases +// kv_singleton, which holds a kv_multi_index as a PRIVATE member and exposes only +// get/set/remove/get_or_create, so it is bound by the mutators' guards -- a singleton handle +// on another account is read-only -- but not by the divergences above, which its API does not +// reach. template class kv_multi_index { @@ -633,25 +628,23 @@ class kv_multi_index { return *obj; } - /// Matches the two-overload shape find/require_find/get already use above: a one-line - /// `name` form delegating to the `uint64_t` one. Overloads rather than a template or a - /// converting-proxy parameter -- both were tried and both changed the argument's meaning. - /// A template cannot deduce `lower_bound({42})`; a proxy accepts `lower_bound({w})` for a - /// `w` converting to a narrower type, which a real `uint64_t` parameter rejects as - /// narrowing. Here the parameter is still a `uint64_t`, so that conversion is the base's. + /// Two concrete overloads, the same shape find/require_find/get use above: a one-line + /// `name` form delegating to the `uint64_t` one. /// - /// This adopts the sibling shape INCLUDING its two costs, neither of which is new to the - /// class but both of which are new to the bounds: + /// Concrete overloads rather than a template or a converting-proxy parameter, because both + /// of those change what the argument means. A template cannot deduce `lower_bound({42})`; + /// a proxy accepts `lower_bound({w})` for a `w` converting to a narrower type, which a real + /// `uint64_t` parameter rejects as narrowing. The parameter here is a `uint64_t`, so every + /// conversion is the one a `uint64_t` parameter performs. /// - /// - `&table::lower_bound` is now an overload set, so the bare address cannot be taken, - /// exactly as for `&table::find`, `&table::get` and `&table::require_find`. A named - /// cast still resolves either one: + /// Two consequences of the overload pair, both shared with the three siblings above: + /// + /// - `&table::lower_bound` is an overload set, so the bare address cannot be taken. A + /// named cast resolves either one: /// `static_cast(&table::lower_bound)`. - /// - a wrapper convertible to BOTH `uint64_t` and `name` becomes ambiguous, where - /// against the single `uint64_t` parameter it selected the `uint64_t` conversion. - /// `find`/`get`/`require_find` have always been ambiguous for such a type, so this - /// makes the bounds consistent rather than introducing a new rule; it is called out - /// because it is a source break, and it is pinned by test. + /// - a wrapper convertible to BOTH `uint64_t` and `name` is ambiguous. + /// + /// Both are pinned by test. const_iterator lower_bound(name primary) const { return lower_bound(primary.value); } const_iterator lower_bound(uint64_t primary) const { auto key = make_pk(primary); @@ -697,11 +690,12 @@ class kv_multi_index { auto key = make_pk(pk); auto value = serialize_row(obj); - // Reject a duplicate primary key, as db_store_i64 did on Antelope. That guard lived - // at the chain layer and was lost with the legacy DB: kv_set is an upsert, so without - // this the row is silently overwritten AND store_secondaries -- an unconditional - // kv_idx_store -- leaves the old (sec_key -> pri_key) mapping behind, pointing at a - // row whose secondary value has changed. kv::table::emplace checks the same way. + // Reject a duplicate primary key. Nothing below this point will: kv_set is an upsert, + // so the row would be silently overwritten, and store_secondaries is an unconditional + // kv_idx_store, so the old (sec_key -> pri_key) mapping would survive and point at a + // row whose secondary value has changed. On Antelope db_store_i64 rejected duplicates + // at the chain layer; the KV intrinsics do not, so the wrapper must. + // kv::table::emplace checks the same way. check(!::kv_contains(_table_id, _code.value, key.data, key_size), "object with the same primary key already exists"); diff --git a/libraries/sysiolib/contracts/sysio/kv_table.hpp b/libraries/sysiolib/contracts/sysio/kv_table.hpp index edec8d1e7..c3f72df65 100644 --- a/libraries/sysiolib/contracts/sysio/kv_table.hpp +++ b/libraries/sysiolib/contracts/sysio/kv_table.hpp @@ -442,10 +442,11 @@ class table_impl { // and, because store_secondaries is an unconditional kv_idx_store, either strands the old // (sec_key -> pri_key) mapping (when the secondary value changed) or trips the host's // ordered_unique constraint on (code, table_id, sec_key, pri_key). emplace() pays one - // kv_contains so no PUBLIC path reaches an unguarded insert. This seals do_insert only: - // store_secondaries/remove_secondaries/update_secondaries and do_erase below stay public, - // and calling store_secondaries directly still strands a mapping the same way. Sealing - // those is a wider change than this fix. + // kv_contains so no PUBLIC path reaches an unguarded insert. + // + // Only do_insert is sealed. store_secondaries, remove_secondaries, update_secondaries and + // do_erase below are public, and calling store_secondaries directly strands a mapping the + // same way -- treat them as internal. void do_insert(uint64_t payer, const be_key_stream& k, const K& key, const V& value) { if constexpr (is_fixed_serializable_v) { char vbuf[sizeof(V)]; From 8576030cf8a3ed99df08ab0d5ee8cabdb765b2fd Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Wed, 2 Sep 2026 10:05:34 -0500 Subject: [PATCH 14/28] test(kv): require the receiver setter to dominate both dispatch branches Lexical ordering was not enough, as the review showed with a working counterexample: void apply(uint64_t r, uint64_t c, uint64_t a) { if (c == r) { sysio_set_contract_name(r); __sysio_action_...(r, c); } else { __sysio_notify_...(r, c); } } That has exactly one setter, passes the exact-`r` check, and precedes the first textual handler -- while every notification runs with the global still 0, which is precisely the case receiving_account()'s fast path depends on. The assertion is now structural: the setter must be the first executable statement in apply(), before pre_dispatch and before the `if (c == r)` split, so it dominates both branches. Reproduced the counterexample by emitting the setter inside that branch; the test now fails with "first statement after apply() is: if (c == r) {" and passed before. Comment corrections from the same review: - The file header still advertised a "Drop-in replacement" with the "Same template API" while the block above the class documents three source breaks. Both cannot be true. - kv_singleton was described as exposing only get/set/remove/get_or_create; it also has exists, try_get and get_or_default. Naming a closed list dates badly, so it now states the distinction that actually matters: single-row accessors and mutators, not the table's iterators, bounds or secondary-index API. - Three test comments predated the success cases and had become false -- that every case aborts before writing, that only the read side of the mock is needed, and that no kv_erase is installed. They now distinguish the rejecting cases from the allowing ones. ctest 31/31, multi_index integration 24/24. --- .../contracts/sysio/kv_multi_index.hpp | 13 ++--- tests/unit/dispatch_receiver_tests.sh | 47 ++++++++++--------- tests/unit/kv_multi_index_tests.cpp | 20 +++++--- 3 files changed, 46 insertions(+), 34 deletions(-) diff --git a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp index 1a40b044a..278c5e964 100644 --- a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp +++ b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp @@ -2,8 +2,9 @@ /** * KV-backed multi_index emulation layer. * - * Drop-in replacement for sysio::multi_index that uses KV intrinsics instead - * of legacy db_*_i64 intrinsics. Same template API, different backend. + * A shim for sysio::multi_index that uses KV intrinsics instead of the legacy db_*_i64 + * ones. The template API is the same in almost every respect; the places it is not are + * listed above the class. * * Key encoding: [scope: 8B BE][primary_key: 8B BE] = 16 bytes. * Table name is encoded in table_id (DJB2 hash of raw template parameter), @@ -159,10 +160,10 @@ namespace _kv_multi_index_detail { // account, matching upstream. Each guard is documented where it stands. // // sysio::multi_index is a direct alias of this template. sysio::singleton is not: it aliases -// kv_singleton, which holds a kv_multi_index as a PRIVATE member and exposes only -// get/set/remove/get_or_create, so it is bound by the mutators' guards -- a singleton handle -// on another account is read-only -- but not by the divergences above, which its API does not -// reach. +// kv_singleton, which holds a kv_multi_index as a PRIVATE member. Its surface is single-row +// accessors and mutators -- not the table's iterators, bounds or secondary-index API -- so it +// is bound by the mutators' guards, and a singleton handle on another account is read-only, +// but none of the divergences above are reachable through it. template class kv_multi_index { diff --git a/tests/unit/dispatch_receiver_tests.sh b/tests/unit/dispatch_receiver_tests.sh index 6610dd40d..d8b5bb4ba 100755 --- a/tests/unit/dispatch_receiver_tests.sh +++ b/tests/unit/dispatch_receiver_tests.sh @@ -59,33 +59,38 @@ else grep -n "sysio_set_contract_name" "$DISPATCH" | sed 's/^/ got: /' || echo " (no call at all)" fi -# It must run before anything is dispatched, or a guard could read a stale value. +# It must be the FIRST executable statement in apply(), not merely the first textually. # -# Match the names the generator actually emits -- `pre_dispatch(`, `__sysio_action_*(` and -# `__sysio_notify_*(`. An earlier version of this test grepped for names that appear nowhere in -# the output, so the "first dispatch" line came back empty and the comparison was skipped as a -# pass: moving the setter below every handler would have satisfied it. The search deliberately -# does NOT start from the setter's line, which would make any match tautologically later. +# Lexical ordering alone is not enough. An apply() shaped as +# +# void apply(uint64_t r, uint64_t c, uint64_t a) { +# if (c == r) { sysio_set_contract_name(r); __sysio_action_...(r, c); } +# else { __sysio_notify_...(r, c); } +# } +# +# has exactly one setter, passes the exact-`r` check, and puts the setter before the first +# textual handler -- while every NOTIFICATION runs with the global still 0. The only assertion +# that rules that out is structural: the setter must sit at the top of the function body, +# before `pre_dispatch` and before the `if (c == r)` split, so it dominates both branches. apply_line="$(grep -nE '^\s*(__attribute__.*)?void apply\(' "$DISPATCH" | head -1 | cut -d: -f1 || true)" -first_dispatch="$(awk -v a="${apply_line:-0}" \ - 'NR > a && /(pre_dispatch\(|__sysio_(action|notify)_[A-Za-z0-9_]*\()/ { print NR; exit }' "$DISPATCH")" -setter_lines="$(grep -nE 'sysio_set_contract_name\(' "$DISPATCH" | awk -F: -v a="${apply_line:-0}" '$1 > a {print $1}')" -setter_count="$(printf '%s\n' "$setter_lines" | grep -c . || true)" +setter_count="$(grep -cE 'sysio_set_contract_name\(' "$DISPATCH" || true)" +# The first non-blank, non-comment line after the function's opening brace. +first_stmt="$(awk -v a="${apply_line:-0}" \ + 'NR > a && $0 !~ /^[[:space:]]*$/ && $0 !~ /^[[:space:]]*\/\// { print; exit }' "$DISPATCH" \ + | sed 's/^[[:space:]]*//; s/[[:space:]]*$//')" if [ -z "$apply_line" ]; then fail "apply() is defined in the generated dispatch" -elif [ -z "$first_dispatch" ]; then - fail "the generated apply() dispatches to a handler" - echo " no pre_dispatch/__sysio_action_*/__sysio_notify_* call found after line ${apply_line}" - sed 's/^/ /' "$DISPATCH" -elif [ "$setter_count" -ne 1 ]; then - fail "apply() records the receiver exactly once" - echo " found ${setter_count} call(s) inside apply(), expected 1" -elif [ "$setter_lines" -lt "$first_dispatch" ]; then - pass "the receiver is recorded before anything is dispatched" +elif [ "$setter_count" -ne 2 ]; then + # one declaration in the extern "C" block, one call inside apply() + fail "the dispatch declares and calls the setter exactly once each" + echo " found ${setter_count} occurrence(s), expected 2" + grep -n "sysio_set_contract_name" "$DISPATCH" | sed 's/^/ /' +elif [ "$first_stmt" = "sysio_set_contract_name(r);" ]; then + pass "the receiver is recorded as apply()'s first statement, before any branch" else - fail "the receiver is recorded before anything is dispatched" - echo " setter at line ${setter_lines}, first dispatch at line ${first_dispatch}" + fail "the receiver is recorded as apply()'s first statement, before any branch" + echo " first statement after apply() is: ${first_stmt}" sed 's/^/ /' "$DISPATCH" fi diff --git a/tests/unit/kv_multi_index_tests.cpp b/tests/unit/kv_multi_index_tests.cpp index c47967137..1a72c52ce 100644 --- a/tests/unit/kv_multi_index_tests.cpp +++ b/tests/unit/kv_multi_index_tests.cpp @@ -19,9 +19,14 @@ * on the receiver. table_id derives from the table NAME alone, so a foreign-code * mutation probes their table and writes the receiver's row of the same name. * - * The cases below never reach a write: each aborts first, so the mocked store is seeded - * directly rather than through emplace, and no iterator or secondary-index intrinsics are - * needed. + * The cases come in two kinds. The REJECTING ones abort before any write, so their store is + * seeded directly rather than through emplace. The ALLOWING ones -- an owned handle doing + * emplace, modify and erase -- run the mutation through, so the mock also serves kv_set, + * kv_erase and the iterator reads that emplace's closing find() performs. Both kinds matter: + * without the allowing ones, a guard that rejected everything would satisfy the suite. + * + * No secondary-index intrinsic is needed: the table under test declares no indices, so + * store/remove/update_secondaries fold to nothing. */ #include @@ -81,8 +86,9 @@ struct callable_with().lowe constexpr uint32_t records_tid = sysio::kv::compute_table_id("records"_n.value); -// Mirrors the asymmetry under test: kv_contains honours `code`, writes have no such -// parameter. Only the read side is needed -- every case here aborts before writing. +// Mirrors the asymmetry under test: kv_contains and kv_get honour `code`, while kv_set and +// kv_erase have no such parameter and always land on store().receiver. Rows are therefore +// keyed by the account that holds them, which is how a misdirected write is made visible. struct mock_kv { using row_key = std::tuple; // code, table_id, key std::map rows; @@ -229,8 +235,8 @@ SYSIO_TEST_BEGIN(foreign_code_handle_cannot_mutate) // The whole point: the receiver's row was never touched. The VALUE comparison is what // carries that -- a misdirected emplace overwrites the row under the same key, so the - // count() below cannot fall to 0 and proves nothing on its own (the mock installs no - // kv_erase, and kv_set only assigns). It is kept as a precondition for the .at(). + // count() below would still be 1 and proves nothing on its own here, where nothing + // erases. It is kept as a precondition for the .at(). CHECK_EQUAL( store().sets, 0u ) const auto seeded = mock_kv::row_key{"alice"_n.value, records_tid, pk_key("alice"_n.value, 1)}; From 746eda00ee859e0979ccfd7f5d179df44786fa61 Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Wed, 2 Sep 2026 10:23:20 -0500 Subject: [PATCH 15/28] test(kv): read apply()'s body from the brace, not the following line The dominance check started at the line after the signature, so anything following the opening brace on that line was invisible: void apply(uint64_t r, uint64_t c, uint64_t a) { if (c == r) { sysio_set_contract_name(r); Setter count is 2, the first statement on its own line is the setter, and the test passed -- while the notification branch still runs with the global at 0. The body is now taken from immediately after the opening brace, including any suffix on the signature line, comments stripped and lines joined, and the first semicolon-terminated statement must be the setter. Verified against both evasion shapes. Emitting the branch on the signature line fails with "first statement after apply() is: if (c == r) { sysio_set_contract_name(r);", and the earlier shape -- setter nested inside the c == r branch -- still fails. The unmodified generator passes. ctest 31/31. --- tests/unit/dispatch_receiver_tests.sh | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/tests/unit/dispatch_receiver_tests.sh b/tests/unit/dispatch_receiver_tests.sh index d8b5bb4ba..d534f11d1 100755 --- a/tests/unit/dispatch_receiver_tests.sh +++ b/tests/unit/dispatch_receiver_tests.sh @@ -74,10 +74,17 @@ fi # before `pre_dispatch` and before the `if (c == r)` split, so it dominates both branches. apply_line="$(grep -nE '^\s*(__attribute__.*)?void apply\(' "$DISPATCH" | head -1 | cut -d: -f1 || true)" setter_count="$(grep -cE 'sysio_set_contract_name\(' "$DISPATCH" || true)" -# The first non-blank, non-comment line after the function's opening brace. -first_stmt="$(awk -v a="${apply_line:-0}" \ - 'NR > a && $0 !~ /^[[:space:]]*$/ && $0 !~ /^[[:space:]]*\/\// { print; exit }' "$DISPATCH" \ - | sed 's/^[[:space:]]*//; s/[[:space:]]*$//')" +# The function body, starting immediately after the opening brace -- INCLUDING any text that +# follows it on the signature line. Reading from the next line down would miss +# `void apply(...) { if (c == r) {`, which puts a branch ahead of the setter while leaving the +# setter as the first thing on its own line. +body="$(awk -v a="${apply_line:-0}" ' + NR < a { next } + NR == a { sub(/^[^{]*\{/, "") } + { print } +' "$DISPATCH" | sed 's://.*::' | tr '\n' ' ' | sed 's/[[:space:]][[:space:]]*/ /g; s/^ //')" +# The first statement is everything up to and including the first semicolon. +first_stmt="${body%%;*};" if [ -z "$apply_line" ]; then fail "apply() is defined in the generated dispatch" From 42ab46eaf4851ba38fd0e210cbedca92c54eca50 Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Wed, 2 Sep 2026 10:50:55 -0500 Subject: [PATCH 16/28] test(kv): count setter occurrences, not matching lines `grep -c` counts matching LINES, so two calls on one line read as one: sysio_set_contract_name(r); sysio_set_contract_name(c); That gave the expected count of 2 (declaration line plus this one), and the first-statement check stopped at the first semicolon and saw the `r` call -- while the second call overwrote the global before either dispatch branch ran. Counts occurrences now, allowing whitespace before the paren, so the exactly-once assertion means what its message says. Verified against all three evasion shapes, each of which passed some earlier version of this test: the branch on the signature line, the setter nested inside `if (c == r)`, and now the double call. All three fail; the unmodified generator passes. ctest 31/31. --- tests/unit/dispatch_receiver_tests.sh | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/unit/dispatch_receiver_tests.sh b/tests/unit/dispatch_receiver_tests.sh index d534f11d1..97ad02259 100755 --- a/tests/unit/dispatch_receiver_tests.sh +++ b/tests/unit/dispatch_receiver_tests.sh @@ -73,7 +73,11 @@ fi # that rules that out is structural: the setter must sit at the top of the function body, # before `pre_dispatch` and before the `if (c == r)` split, so it dominates both branches. apply_line="$(grep -nE '^\s*(__attribute__.*)?void apply\(' "$DISPATCH" | head -1 | cut -d: -f1 || true)" -setter_count="$(grep -cE 'sysio_set_contract_name\(' "$DISPATCH" || true)" +# -o | wc -l counts OCCURRENCES. `grep -c` counts matching LINES, which let +# `sysio_set_contract_name(r); sysio_set_contract_name(c);` on one line read as a single +# call: the count came to the expected 2, the first statement was still the `r` call, and the +# second call silently overwrote the global before either branch ran. +setter_count="$(grep -oE 'sysio_set_contract_name[[:space:]]*\(' "$DISPATCH" | wc -l)" # The function body, starting immediately after the opening brace -- INCLUDING any text that # follows it on the signature line. Reading from the next line down would miss # `void apply(...) { if (c == r) {`, which puts a branch ahead of the setter while leaving the From a09ca51fa385ce3430f7a9e3659f3e870bf07f56 Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Wed, 2 Sep 2026 12:17:50 -0500 Subject: [PATCH 17/28] test(kv): test the dispatch checker, not just the dispatch Fifth evasion of this assertion: `sysio_set_contract_name/**/(c)` slips past a grep for `identifier[[:space:]]*(`, so the count stayed at the expected 2 and the first statement was still the `r` call, while `c` overwrote the global before dispatch. Patching the pattern again would have been the fifth fix of the same shape, so the structure changes instead. - Comments are stripped before anything is counted or ordered, which retires the whole class of lexical hiding rather than this one spelling. - The count is of the IDENTIFIER, not of a call spelling, so any occurrence beyond the declaration and the single call fails regardless of what follows it. - The checker is now a function over a dispatch file, and the test runs it against a table of crafted counterexamples as well as against the real generated output. Every shape that defeated an earlier revision is a row: the code passed instead of the receiver, the setter nested in `if (c == r)`, two calls on one line, the comment-split call, no setter at all, and the setter after dispatch. A positive control -- a space before the paren -- keeps the checker from passing by rejecting everything. A new evasion is now one row in that table rather than a review round. Also fixes a hazard in this file: the real-dispatch call was not `|| true` guarded, so under `set -e` a genuine failure aborted the script mid-run instead of reporting FAIL. Found by mutating the generator to emit `(c)` and seeing the output truncate rather than fail. Verified: all six counterexamples rejected, the positive control accepted, and mutating the generator to `(c)` fails end to end with "first statement of apply() is: sysio_set_contract_name(c);". ctest 31/31, integration 24/24. --- tests/unit/dispatch_receiver_tests.sh | 203 +++++++++++++++++++------- 1 file changed, 149 insertions(+), 54 deletions(-) diff --git a/tests/unit/dispatch_receiver_tests.sh b/tests/unit/dispatch_receiver_tests.sh index 97ad02259..c900ce10e 100755 --- a/tests/unit/dispatch_receiver_tests.sh +++ b/tests/unit/dispatch_receiver_tests.sh @@ -3,10 +3,18 @@ # # multi_index's receiving_account() reads that global and falls back to the current_receiver # intrinsic only when it is 0, so the guards on emplace/modify/erase are only correct if the -# dispatcher stores `r` (the receiver) rather than `c` (the code). Every in-tree action is -# self-sent, so r == c and the entire unit + integration suite stays green if that argument is -# changed -- the divergence appears only under notification, on chain. This pins it at the -# source: the emitted dispatch text, which no other test inspects. +# dispatcher stores `r` (the receiver) rather than `c` (the code), exactly once, before any +# dispatch. Every in-tree action is self-sent, so r == c and the whole unit + integration suite +# stays green if that is broken -- the divergence appears only under notification, on chain. +# This pins it at the source: the emitted dispatch text, which no other test inspects. +# +# The checker is a function over a dispatch FILE, and it is exercised twice: against the real +# generated dispatch, and against a table of crafted counterexamples that must each be +# rejected. Earlier revisions of this test were defeated four times in review -- by handler +# names it did not match, by a branch on the signature line, by two calls on one line, and by a +# comment between the identifier and its paren -- because each fix pattern-matched the last +# evasion. Checking the checker is what stops that: a new evasion is one row below, not a round +# trip. # # Usage: dispatch_receiver_tests.sh set -euo pipefail @@ -21,6 +29,87 @@ fail() { echo " FAIL: $1"; FAIL=$((FAIL + 1)); } WORK="$(mktemp -d)" trap 'rm -rf "$WORK"' EXIT +# Strip comments before anything is counted or ordered. Lexical tricks that hide a second call +# -- `name/**/(c)` -- stop working once the text the checker sees has no comments in it. +strip_comments() { + python3 - "$1" <<'PYEOF' +import re, sys +src = open(sys.argv[1]).read() +out, i, n = [], 0, len(src) +while i < n: + c = src[i] + if c == '"' or c == "'": # string / char literal: copy verbatim + q = c; out.append(c); i += 1 + while i < n: + out.append(src[i]) + if src[i] == '\\': + i += 2 + if i <= n: out.append(src[i-1]) + continue + if src[i] == q: i += 1; break + i += 1 + continue + if src.startswith('//', i): + while i < n and src[i] != '\n': i += 1 + continue + if src.startswith('/*', i): + j = src.find('*/', i + 2) + out.append(' ') # a comment is whitespace, not nothing + i = (j + 2) if j != -1 else n + continue + out.append(c); i += 1 +print(''.join(out)) +PYEOF +} + +# Decide whether one dispatch file satisfies the contract. Echoes OK, or a reason. +check_dispatch() { + local file="$1" clean + clean="$(mktemp)" + strip_comments "$file" > "$clean" + + # Every occurrence of the identifier, however spelled. Two are expected: the declaration + # in the extern "C" block and the single call inside apply(). Counting matching LINES, or + # only `identifier(`, both let a second call hide. + local occurrences + occurrences="$(grep -owE 'sysio_set_contract_name' "$clean" | wc -l)" + if [ "$occurrences" -ne 2 ]; then + echo "expected 2 occurrences of sysio_set_contract_name (1 declaration + 1 call), found ${occurrences}" + rm -f "$clean"; return 1 + fi + + local apply_line + apply_line="$(grep -nE '^[[:space:]]*(__attribute__.*)?void apply\(' "$clean" | head -1 | cut -d: -f1 || true)" + if [ -z "$apply_line" ]; then + echo "no apply() definition found" + rm -f "$clean"; return 1 + fi + + # The body from immediately after the opening brace, INCLUDING any suffix on the signature + # line, joined into one line. Reading from the next line down misses + # `void apply(...) { if (c == r) {`. + local body first_stmt + body="$(awk -v a="$apply_line" ' + NR < a { next } + NR == a { sub(/^[^{]*\{/, "") } + { print } + ' "$clean" | tr '\n' ' ' | sed 's/[[:space:]][[:space:]]*/ /g; s/^ //')" + first_stmt="${body%%;*};" + rm -f "$clean" + + # Normalise spacing so `set (r)` and `set(r)` compare alike. + local normalised + normalised="$(printf '%s' "$first_stmt" | tr -d ' ')" + if [ "$normalised" != "sysio_set_contract_name(r);" ]; then + echo "first statement of apply() is: ${first_stmt}" + return 1 + fi + echo OK +} + +echo "=== Dispatch Receiver Tests ===" + +# --- 1. the real generated dispatch -------------------------------------------------------- cat > "${WORK}/c.cpp" <<'EOF' #include class [[sysio::contract("dispatchrcv")]] dispatchrcv : public sysio::contract { @@ -31,8 +120,6 @@ public: }; EOF -echo "=== Dispatch Receiver Tests ===" - if ! ( cd "$WORK" && "$CDT_CPP" -abigen -abigen_output=c.abi -contract=dispatchrcv \ -o c.wasm c.cpp ) > "${WORK}/build.log" 2>&1; then fail "contract builds" @@ -50,59 +137,67 @@ if [ -z "$DISPATCH" ]; then fi pass "a dispatch.cpp was generated" -# apply(uint64_t r, uint64_t c, uint64_t a): the receiver is the FIRST parameter. -if grep -qE 'sysio_set_contract_name\(\s*r\s*\)' "$DISPATCH"; then - pass "apply() records the receiver, not the code" +# `|| true`: check_dispatch returns non-zero on rejection, and under `set -e` the assignment +# would take that status and abort the script -- turning a real failure into a truncated run +# instead of a reported one. +verdict="$(check_dispatch "$DISPATCH" || true)" +if [ "$verdict" = OK ]; then + pass "the generated apply() records the receiver once, before any dispatch" else - fail "apply() records the receiver, not the code" - echo " expected: sysio_set_contract_name(r)" - grep -n "sysio_set_contract_name" "$DISPATCH" | sed 's/^/ got: /' || echo " (no call at all)" + fail "the generated apply() records the receiver once, before any dispatch" + echo " ${verdict}" + sed 's/^/ /' "$DISPATCH" fi -# It must be the FIRST executable statement in apply(), not merely the first textually. -# -# Lexical ordering alone is not enough. An apply() shaped as +# --- 2. the checker itself ----------------------------------------------------------------- # -# void apply(uint64_t r, uint64_t c, uint64_t a) { -# if (c == r) { sysio_set_contract_name(r); __sysio_action_...(r, c); } -# else { __sysio_notify_...(r, c); } -# } -# -# has exactly one setter, passes the exact-`r` check, and puts the setter before the first -# textual handler -- while every NOTIFICATION runs with the global still 0. The only assertion -# that rules that out is structural: the setter must sit at the top of the function body, -# before `pre_dispatch` and before the `if (c == r)` split, so it dominates both branches. -apply_line="$(grep -nE '^\s*(__attribute__.*)?void apply\(' "$DISPATCH" | head -1 | cut -d: -f1 || true)" -# -o | wc -l counts OCCURRENCES. `grep -c` counts matching LINES, which let -# `sysio_set_contract_name(r); sysio_set_contract_name(c);` on one line read as a single -# call: the count came to the expected 2, the first statement was still the `r` call, and the -# second call silently overwrote the global before either branch ran. -setter_count="$(grep -oE 'sysio_set_contract_name[[:space:]]*\(' "$DISPATCH" | wc -l)" -# The function body, starting immediately after the opening brace -- INCLUDING any text that -# follows it on the signature line. Reading from the next line down would miss -# `void apply(...) { if (c == r) {`, which puts a branch ahead of the setter while leaving the -# setter as the first thing on its own line. -body="$(awk -v a="${apply_line:-0}" ' - NR < a { next } - NR == a { sub(/^[^{]*\{/, "") } - { print } -' "$DISPATCH" | sed 's://.*::' | tr '\n' ' ' | sed 's/[[:space:]][[:space:]]*/ /g; s/^ //')" -# The first statement is everything up to and including the first semicolon. -first_stmt="${body%%;*};" - -if [ -z "$apply_line" ]; then - fail "apply() is defined in the generated dispatch" -elif [ "$setter_count" -ne 2 ]; then - # one declaration in the extern "C" block, one call inside apply() - fail "the dispatch declares and calls the setter exactly once each" - echo " found ${setter_count} occurrence(s), expected 2" - grep -n "sysio_set_contract_name" "$DISPATCH" | sed 's/^/ /' -elif [ "$first_stmt" = "sysio_set_contract_name(r);" ]; then - pass "the receiver is recorded as apply()'s first statement, before any branch" +# Each of these compiles and each breaks the contract. Every one defeated some earlier +# revision of this test, so they are kept as regressions on the CHECKER. +mkbad() { # $1=name $2=apply-body + cat > "${WORK}/bad_$1.cpp" < Date: Wed, 2 Sep 2026 13:09:36 -0500 Subject: [PATCH 18/28] test(kv): splice before lexing, and make the signature-line fixture bite Two findings, both correct. The lexer ran on physical source, but phase 2 removes backslash-newline pairs before comments and tokenization, so sysio_set_contract_name(r); sysio_set_contract_\ name(c); compiles as a second call while the checker saw only two occurrences. Normalisation now follows the front end's order: splice, then replace comments with a space. Confirmed the fixture compiles under clang -std=c++17, and that removing the splice step fails it. The signature_line counterexample did not pin what it claimed. mkbad always emitted a newline after the opening brace, so no fixture ever placed text on the signature line -- and the first version I wrote closed its branch on that same line, which the checker rejects either way. Reverting the body extraction to skip the apply line left it passing. mkbad now appends the body directly after the brace, and the fixture opens its branch on the signature line with the setter first on the line below -- the shape that discriminates: read the body from the brace and the first statement is `if (c == r) {`; read it from the next line and the setter looks first. Ablating the extraction now fails signature_line, where before it failed only after_dispatch, by accident. Eight counterexamples plus a positive control. Not a full preprocessor: it does not expand macros or paste tokens, which the generated dispatch does not use, and the comment says so. ctest 31/31. --- tests/unit/dispatch_receiver_tests.sh | 56 ++++++++++++++++++++------- 1 file changed, 42 insertions(+), 14 deletions(-) diff --git a/tests/unit/dispatch_receiver_tests.sh b/tests/unit/dispatch_receiver_tests.sh index c900ce10e..623d7af7d 100755 --- a/tests/unit/dispatch_receiver_tests.sh +++ b/tests/unit/dispatch_receiver_tests.sh @@ -29,12 +29,17 @@ fail() { echo " FAIL: $1"; FAIL=$((FAIL + 1)); } WORK="$(mktemp -d)" trap 'rm -rf "$WORK"' EXIT -# Strip comments before anything is counted or ordered. Lexical tricks that hide a second call -# -- `name/**/(c)` -- stop working once the text the checker sees has no comments in it. -strip_comments() { +# Normalise the source the way the front end does, in the same order: phase 2 splices +# backslash-newline pairs, then phase 3 replaces comments with a space. Doing either on +# physical lines lets a second call hide -- `name/**/(c)` behind a comment, or +# `sysio_set_contract_\name(c)` behind a splice, both of which compile as an ordinary +# call. This is not a full preprocessor: it does not expand macros or paste tokens, which the +# generated dispatch does not use. +normalise_source() { python3 - "$1" <<'PYEOF' import re, sys src = open(sys.argv[1]).read() +src = src.replace('\\\n', '') # phase 2: line splicing, before anything else out, i, n = [], 0, len(src) while i < n: c = src[i] @@ -66,7 +71,7 @@ PYEOF check_dispatch() { local file="$1" clean clean="$(mktemp)" - strip_comments "$file" > "$clean" + normalise_source "$file" > "$clean" # Every occurrence of the identifier, however spelled. Two are expected: the declaration # in the extern "C" block and the single call inside apply(). Counting matching LINES, or @@ -153,32 +158,54 @@ fi # # Each of these compiles and each breaks the contract. Every one defeated some earlier # revision of this test, so they are kept as regressions on the CHECKER. -mkbad() { # $1=name $2=apply-body +# $2 is appended directly after the opening brace, so a fixture can put text on the SIGNATURE +# line by starting without a newline. mkbad used to emit one unconditionally, which meant no +# counterexample ever exercised that escape even though the header claimed one did. +mkbad() { # $1=name $2=apply-body (leading newline optional) cat > "${WORK}/bad_$1.cpp" < Date: Wed, 2 Sep 2026 13:44:17 -0500 Subject: [PATCH 19/28] test(kv): let clang tokenize the dispatch, and compile every counterexample Stop hand-rolling a C++ lexer. Four revisions of that normaliser were each defeated by a different lexical form -- a comment between the identifier and its paren, a backslash-newline splice, a backslash followed by spaces then a newline, and a raw string whose contents look like a comment (`R"d(" // )d"`, where the hand-rolled parser treats the opening quote as an ordinary string, the // as a comment, and erases the real call after it). Each fix was another narrow normalisation, which leaves the same class of false accepts. check_dispatch now preprocesses with the bundled clang++ -E -P, which implements phases 1-4 exactly as the compiler that builds the contract does. #include lines are dropped first; the dispatch defines no macros, so nothing in it depends on them, and this avoids needing the header tree resolved. Confirmed against the raw-string case: clang reports three occurrences of the identifier where the hand-rolled parser reported two. Two lexical counterexamples added for the forms that motivated this -- backslash-whitespace-newline and the raw string. Separately: the fixtures did not compile. mkbad emitted `uint64_t` with no , so `unknown type name 'uint64_t'` -- and nothing checked, so a malformed fixture would have counted as a successful rejection forever. The files are self-contained now, and every negative fixture and the positive control is syntax-checked before its verdict is trusted. Verified by corrupting a fixture: it now reports "counterexample compiles: missing_entirely" rather than passing. Ten counterexamples plus the positive control. Mutating the generator to emit (c) still fails end to end. ctest 31/31. --- tests/unit/dispatch_receiver_tests.sh | 77 ++++++++++++++------------- 1 file changed, 41 insertions(+), 36 deletions(-) diff --git a/tests/unit/dispatch_receiver_tests.sh b/tests/unit/dispatch_receiver_tests.sh index 623d7af7d..789684809 100755 --- a/tests/unit/dispatch_receiver_tests.sh +++ b/tests/unit/dispatch_receiver_tests.sh @@ -21,6 +21,7 @@ set -euo pipefail BUILD_DIR="$1" CDT_CPP="${BUILD_DIR}/bin/cdt-cpp" +CLANGXX="${BUILD_DIR}/bin/clang++" PASS=0 FAIL=0 pass() { echo " PASS: $1"; PASS=$((PASS + 1)); } @@ -29,42 +30,23 @@ fail() { echo " FAIL: $1"; FAIL=$((FAIL + 1)); } WORK="$(mktemp -d)" trap 'rm -rf "$WORK"' EXIT -# Normalise the source the way the front end does, in the same order: phase 2 splices -# backslash-newline pairs, then phase 3 replaces comments with a space. Doing either on -# physical lines lets a second call hide -- `name/**/(c)` behind a comment, or -# `sysio_set_contract_\name(c)` behind a splice, both of which compile as an ordinary -# call. This is not a full preprocessor: it does not expand macros or paste tokens, which the -# generated dispatch does not use. +# Normalise with the REAL preprocessor rather than a hand-rolled lexer. +# +# Four hand-written revisions of this were each defeated by a different lexical form -- a +# comment between the identifier and its paren, a backslash-newline splice, a backslash +# followed by spaces then a newline, and a raw string whose contents look like a comment. Every +# fix was another narrow normalisation, which leaves the same class of false accepts. Clang +# already implements phases 1-4 exactly as the compiler that builds the contract does, so it +# decides what a token is. +# +# #include lines are dropped first: the dispatch defines no macros, so nothing it contains +# depends on them, and this keeps the check from needing the whole header tree resolved. normalise_source() { - python3 - "$1" <<'PYEOF' -import re, sys -src = open(sys.argv[1]).read() -src = src.replace('\\\n', '') # phase 2: line splicing, before anything else -out, i, n = [], 0, len(src) -while i < n: - c = src[i] - if c == '"' or c == "'": # string / char literal: copy verbatim - q = c; out.append(c); i += 1 - while i < n: - out.append(src[i]) - if src[i] == '\\': - i += 2 - if i <= n: out.append(src[i-1]) - continue - if src[i] == q: i += 1; break - i += 1 - continue - if src.startswith('//', i): - while i < n and src[i] != '\n': i += 1 - continue - if src.startswith('/*', i): - j = src.find('*/', i + 2) - out.append(' ') # a comment is whitespace, not nothing - i = (j + 2) if j != -1 else n - continue - out.append(c); i += 1 -print(''.join(out)) -PYEOF + sed '/^[[:space:]]*#[[:space:]]*include/d' "$1" > "${1}.noinc" 2>/dev/null || return 1 + "$CLANGXX" -std=c++17 -E -P -nostdinc -nostdinc++ -x c++ "${1}.noinc" 2>/dev/null + local rc=$? + rm -f "${1}.noinc" + return $rc } # Decide whether one dispatch file satisfies the contract. Echoes OK, or a reason. @@ -163,6 +145,7 @@ fi # counterexample ever exercised that escape even though the header claimed one did. mkbad() { # $1=name $2=apply-body (leading newline optional) cat > "${WORK}/bad_$1.cpp" < extern "C" { void sysio_set_contract_name(uint64_t n); void __sysio_action_go_x(uint64_t r, uint64_t c); @@ -199,13 +182,32 @@ mkbad spliced_call ' sysio_set_contract_name(r); sysio_set_contract_\ name(c); if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' +# A backslash followed by horizontal whitespace before the newline. Clang splices it (with a +# warning), so this is a second call; a normaliser matching only an adjacent backslash-newline +# does not see it. +mkbad spliced_ws ' + sysio_set_contract_name(r); sysio_set_contract_\ +name(c); + if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' +# A raw string whose contents look like a comment. A hand-rolled lexer treats the opening quote +# as an ordinary string and the // inside it as a comment, erasing the real call after it. +mkbad raw_string_comment ' + sysio_set_contract_name(r); + const char* s = R"d(" // )d"; sysio_set_contract_name(c); (void)s; + if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' mkbad missing_entirely ' if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' mkbad after_dispatch ' if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); } sysio_set_contract_name(r);' for bad in code_not_receiver inside_branch signature_line double_call comment_split \ - spliced_call missing_entirely after_dispatch; do + spliced_call spliced_ws raw_string_comment missing_entirely after_dispatch; do + if ! "$CLANGXX" -std=c++17 -fsyntax-only -Wno-comment "${WORK}/bad_${bad}.cpp" \ + > "${WORK}/bad_${bad}.log" 2>&1; then + fail "counterexample compiles: ${bad}" + sed 's/^/ /' "${WORK}/bad_${bad}.log" + continue + fi verdict="$(check_dispatch "${WORK}/bad_${bad}.cpp" || true)" if [ "$verdict" = OK ]; then fail "the checker rejects: ${bad}" @@ -220,6 +222,9 @@ done mkbad spaced_ok ' sysio_set_contract_name (r); if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' +if ! "$CLANGXX" -std=c++17 -fsyntax-only "${WORK}/bad_spaced_ok.cpp" > /dev/null 2>&1; then + fail "the positive control compiles" +fi verdict="$(check_dispatch "${WORK}/bad_spaced_ok.cpp" || true)" if [ "$verdict" = OK ]; then pass "the checker accepts: a space before the paren" From fb742548da9d79aeba59e2871183f8c6adf97dec Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Wed, 2 Sep 2026 14:22:35 -0500 Subject: [PATCH 20/28] test(kv): make the counterexamples freestanding The macOS job failed all eleven compile checks with `'cstdint' file not found`. The check uses the bundled clang++, which targets WebAssembly and ships no host standard library, so `` resolved on Linux only by accident of the platform's include search path. The fixtures need the type, not the header: `typedef unsigned long long uint64_t;`. Both compile checks now pass -nostdinc -nostdinc++, so the requirement is enforced rather than left to luck -- passing on Linux with those flags is what demonstrates the macOS case, since they remove exactly what that platform lacks. I added the include last round in response to review, verified it on Linux, and did not consider that the compiler being used has no host headers at all. ctest 31/31; the suite passes with the fixtures compiled freestanding. --- tests/unit/dispatch_receiver_tests.sh | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/tests/unit/dispatch_receiver_tests.sh b/tests/unit/dispatch_receiver_tests.sh index 789684809..45478592e 100755 --- a/tests/unit/dispatch_receiver_tests.sh +++ b/tests/unit/dispatch_receiver_tests.sh @@ -145,7 +145,7 @@ fi # counterexample ever exercised that escape even though the header claimed one did. mkbad() { # $1=name $2=apply-body (leading newline optional) cat > "${WORK}/bad_$1.cpp" < +typedef unsigned long long uint64_t; // not : see the -nostdinc note below extern "C" { void sysio_set_contract_name(uint64_t n); void __sysio_action_go_x(uint64_t r, uint64_t c); @@ -202,7 +202,11 @@ mkbad after_dispatch ' if (c == r) { __sysio_action_go_x(r, c); } else { _ for bad in code_not_receiver inside_branch signature_line double_call comment_split \ spliced_call spliced_ws raw_string_comment missing_entirely after_dispatch; do - if ! "$CLANGXX" -std=c++17 -fsyntax-only -Wno-comment "${WORK}/bad_${bad}.cpp" \ + # -nostdinc/-nostdinc++: the bundled clang++ targets WebAssembly and carries no host + # standard library, so any #include would resolve only by accident of the platform's + # search path. The fixtures are self-contained; this makes that a requirement, not luck. + if ! "$CLANGXX" -std=c++17 -fsyntax-only -nostdinc -nostdinc++ -Wno-comment \ + "${WORK}/bad_${bad}.cpp" \ > "${WORK}/bad_${bad}.log" 2>&1; then fail "counterexample compiles: ${bad}" sed 's/^/ /' "${WORK}/bad_${bad}.log" @@ -222,7 +226,8 @@ done mkbad spaced_ok ' sysio_set_contract_name (r); if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' -if ! "$CLANGXX" -std=c++17 -fsyntax-only "${WORK}/bad_spaced_ok.cpp" > /dev/null 2>&1; then +if ! "$CLANGXX" -std=c++17 -fsyntax-only -nostdinc -nostdinc++ \ + "${WORK}/bad_spaced_ok.cpp" > /dev/null 2>&1; then fail "the positive control compiles" fi verdict="$(check_dispatch "${WORK}/bad_spaced_ok.cpp" || true)" From 7e5a89ca2dd94b292bc80cc0c103466776194ae4 Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Wed, 2 Sep 2026 16:01:58 -0500 Subject: [PATCH 21/28] test(kv): preprocess and compile through the CDT driver The checker used the bundled clang++ with the HOST target and deleted #include lines first. Both changed the translation unit away from the one that ships: - the host target evaluates `#ifdef __wasm__` the wrong way, so a second setter call guarded on it was invisible. Reproduced: host preprocessing counts 2 occurrences and the checker returns OK, while `cdt-cpp -E` counts 3 and `cdt-cpp -c` compiles the branch that overwrites the receiver with the code; - dropping includes erases any macro that expands to a second call. Both steps go through cdt-cpp now, which supplies the wasm32 target, the CDT include graph and the same predefined macros as the real compile. -E emits line markers (the driver rejects -P), so those are dropped afterwards; no real directive survives preprocessing. The counterexamples are compiled with `cdt-cpp -c` rather than a host syntax check, so "this counterexample is legal" means legal in the translation unit that ships. Two counterexamples added for the forms that motivated this, each verified to fail the corresponding ablation: - target_conditional -- preprocessing with the host clang fails it; - macro_expanded, with the macro defined in an INCLUDED header -- stripping #include lines before preprocessing fails it. An inline #define would not have tested that, since any preprocessor expands it. Twelve counterexamples plus the positive control. Mutating the generator to emit (c) still fails end to end. ctest 31/31, integration 24/24. --- tests/unit/dispatch_receiver_tests.sh | 58 +++++++++++++++++---------- 1 file changed, 36 insertions(+), 22 deletions(-) diff --git a/tests/unit/dispatch_receiver_tests.sh b/tests/unit/dispatch_receiver_tests.sh index 45478592e..97cc21f4b 100755 --- a/tests/unit/dispatch_receiver_tests.sh +++ b/tests/unit/dispatch_receiver_tests.sh @@ -30,23 +30,18 @@ fail() { echo " FAIL: $1"; FAIL=$((FAIL + 1)); } WORK="$(mktemp -d)" trap 'rm -rf "$WORK"' EXIT -# Normalise with the REAL preprocessor rather than a hand-rolled lexer. +# Normalise through the CDT DRIVER, so the tokens counted are the ones that will ship. # -# Four hand-written revisions of this were each defeated by a different lexical form -- a -# comment between the identifier and its paren, a backslash-newline splice, a backslash -# followed by spaces then a newline, and a raw string whose contents look like a comment. Every -# fix was another narrow normalisation, which leaves the same class of false accepts. Clang -# already implements phases 1-4 exactly as the compiler that builds the contract does, so it -# decides what a token is. +# Earlier revisions used the bundled clang++ with the host target and deleted #include lines +# first. Both choices changed the translation unit: the host target evaluates `#ifdef __wasm__` +# the wrong way, so a second setter call guarded on it was invisible while cdt-cpp compiled it +# happily; and dropping includes erases any macro that expands to one. cdt-cpp applies the +# wasm32 target, the CDT include graph and the same predefined macros as the real compile. # -# #include lines are dropped first: the dispatch defines no macros, so nothing it contains -# depends on them, and this keeps the check from needing the whole header tree resolved. +# -E emits line markers (no -P: the driver rejects it), so those are dropped afterwards. They +# begin with '#', and after preprocessing no real directive remains. normalise_source() { - sed '/^[[:space:]]*#[[:space:]]*include/d' "$1" > "${1}.noinc" 2>/dev/null || return 1 - "$CLANGXX" -std=c++17 -E -P -nostdinc -nostdinc++ -x c++ "${1}.noinc" 2>/dev/null - local rc=$? - rm -f "${1}.noinc" - return $rc + "$CDT_CPP" -E "$1" 2>/dev/null | sed '/^[[:space:]]*#/d' } # Decide whether one dispatch file satisfies the contract. Echoes OK, or a reason. @@ -195,18 +190,38 @@ mkbad raw_string_comment ' sysio_set_contract_name(r); const char* s = R"d(" // )d"; sysio_set_contract_name(c); (void)s; if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' +# Guarded on the target. The host branch is well-formed, so a checker preprocessing for the +# host counts two occurrences and accepts -- while cdt-cpp compiles the wasm branch, where the +# receiver is immediately overwritten with the code. +mkbad target_conditional ' +#ifdef __wasm__ + sysio_set_contract_name(r); sysio_set_contract_name(c); +#else + sysio_set_contract_name(r); +#endif + if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' +# The second call arrives from a macro defined in an INCLUDED header -- the shape that a +# checker deleting #include lines cannot see, however well it expands what remains. +cat > "${WORK}/record_again.hpp" <<'EOF' +#pragma once +#define RECORD_AGAIN sysio_set_contract_name(c) +EOF +mkbad macro_expanded ' +#include "record_again.hpp" + sysio_set_contract_name(r); RECORD_AGAIN; + if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' mkbad missing_entirely ' if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' mkbad after_dispatch ' if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); } sysio_set_contract_name(r);' for bad in code_not_receiver inside_branch signature_line double_call comment_split \ - spliced_call spliced_ws raw_string_comment missing_entirely after_dispatch; do - # -nostdinc/-nostdinc++: the bundled clang++ targets WebAssembly and carries no host - # standard library, so any #include would resolve only by accident of the platform's - # search path. The fixtures are self-contained; this makes that a requirement, not luck. - if ! "$CLANGXX" -std=c++17 -fsyntax-only -nostdinc -nostdinc++ -Wno-comment \ - "${WORK}/bad_${bad}.cpp" \ + spliced_call spliced_ws raw_string_comment target_conditional macro_expanded \ + missing_entirely after_dispatch; do + # Compiled by the DRIVER, not a host clang: a counterexample must be legal in the + # translation unit that actually ships, and the driver supplies the wasm32 target and the + # CDT include graph. (`-c` to an object we discard; the driver has no -fsyntax-only.) + if ! ( cd "$WORK" && "$CDT_CPP" -c "bad_${bad}.cpp" -o "bad_${bad}.o" ) \ > "${WORK}/bad_${bad}.log" 2>&1; then fail "counterexample compiles: ${bad}" sed 's/^/ /' "${WORK}/bad_${bad}.log" @@ -226,8 +241,7 @@ done mkbad spaced_ok ' sysio_set_contract_name (r); if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' -if ! "$CLANGXX" -std=c++17 -fsyntax-only -nostdinc -nostdinc++ \ - "${WORK}/bad_spaced_ok.cpp" > /dev/null 2>&1; then +if ! ( cd "$WORK" && "$CDT_CPP" -c bad_spaced_ok.cpp -o bad_spaced_ok.o ) > /dev/null 2>&1; then fail "the positive control compiles" fi verdict="$(check_dispatch "${WORK}/bad_spaced_ok.cpp" || true)" From a25add17e1678dbcc1b7317d84c0304901fc3121 Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Wed, 2 Sep 2026 16:22:03 -0500 Subject: [PATCH 22/28] test(kv): filter numeric line markers only, and correct the upstream claims The marker filter dropped every line beginning with '#', which also deletes a multiline raw string whose closing delimiter sits at column 1 after a '#', taking the code that follows it on that line. Reproduced: cdt-cpp compiles it and -E emits both calls, but the filter took the count from 3 to 2 and the checker returned OK. It matches `# ` now, and raw_string_hash is a counterexample -- reverting to the broad filter fails it. The header comment called every listed item a source break against upstream. Two of them are not: - `&table::lower_bound` fails upstream too, because a member template's parameter cannot be deduced. The Wire reason differs (an overload set), the outcome does not. - a trivially-copyable secondary key is upstream's restriction as well; only where it is diagnosed differs, since the static_assert sits in secondary_index_view and fires at get_index<...>(). The actual template-shape break was missing: an explicit `t.template lower_bound(k)` compiles against a member template and is rejected here with "'lower_bound' following the 'template' keyword does not refer to a template". Verified both bounds. The comment now separates breaks from shared restrictions and names the call form that stops compiling. Thirteen counterexamples plus the positive control. ctest 31/31. --- .../contracts/sysio/kv_multi_index.hpp | 27 +++++++++++++------ tests/unit/dispatch_receiver_tests.sh | 21 +++++++++++---- 2 files changed, 35 insertions(+), 13 deletions(-) diff --git a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp index 278c5e964..d65024b3b 100644 --- a/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp +++ b/libraries/sysiolib/contracts/sysio/kv_multi_index.hpp @@ -145,16 +145,27 @@ namespace _kv_multi_index_detail { // Uses sysio::indexed_by and sysio::const_mem_fun from the standard CDT headers. // // A shim for the EOSIO multi_index over a different store. Nearly all contract code carries -// over; these are the places it does not, each a source break against upstream code: +// over. Two things do not, and they are worth keeping apart. +// +// SOURCE BREAKS AGAINST UPSTREAM -- code that compiles there and not here: // // - the postfix iterator operators are deleted, because copying a KV iterator duplicates a -// host-side handle. rbegin()/rend() hand back a std::reverse_iterator, whose postfix -// operators are the adaptor's and are NOT deleted, so reverse loops compile silently; -// - the primary bounds are uint64_t/name overloads rather than upstream's member template, -// so &table::lower_bound cannot be taken bare and a wrapper convertible to both is -// ambiguous (see the note at the bounds themselves); -// - a secondary key must be trivially copyable, enforced by a static_assert in -// secondary_index_view -- so it fires at get_index<...>(), not at declaration. +// host-side handle. Note rbegin()/rend() hand back a std::reverse_iterator, whose postfix +// operators are the adaptor's and are NOT deleted, so reverse loops compile silently and +// the sweep does not find them; +// - the primary bounds are uint64_t/name overloads where upstream has a member template, so +// an explicit call -- `t.template lower_bound(k)`, likewise upper_bound -- is +// rejected with "does not refer to a template". A wrapper convertible to BOTH uint64_t and +// name is also ambiguous here (see the note at the bounds). +// +// RESTRICTIONS SHARED WITH UPSTREAM, which are not breaks even though they bite: +// +// - taking the bare address of a primary bound, `&table::lower_bound`, does not compile -- +// here because the name is an overload set, upstream because a member template's parameter +// cannot be deduced. A named static_cast resolves one on Wire; +// - a secondary key must be trivially copyable. Upstream's supported secondary types are all +// trivially copyable too; what differs is only where it is diagnosed. The static_assert +// lives in secondary_index_view, so it fires at get_index<...>(), not at declaration. // // The mutators reject a duplicate primary key and a handle whose code is not the receiving // account, matching upstream. Each guard is documented where it stands. diff --git a/tests/unit/dispatch_receiver_tests.sh b/tests/unit/dispatch_receiver_tests.sh index 97cc21f4b..739dfbc1c 100755 --- a/tests/unit/dispatch_receiver_tests.sh +++ b/tests/unit/dispatch_receiver_tests.sh @@ -38,10 +38,12 @@ trap 'rm -rf "$WORK"' EXIT # happily; and dropping includes erases any macro that expands to one. cdt-cpp applies the # wasm32 target, the CDT include graph and the same predefined macros as the real compile. # -# -E emits line markers (no -P: the driver rejects it), so those are dropped afterwards. They -# begin with '#', and after preprocessing no real directive remains. +# -E emits line markers (no -P: the driver rejects it), so those are dropped afterwards -- +# NUMERIC ones specifically, `# ""`. Dropping every line that starts with '#' +# also deletes a multiline raw string whose closing delimiter sits at column 1 after a '#', +# taking the executable code that follows it on that line with it. normalise_source() { - "$CDT_CPP" -E "$1" 2>/dev/null | sed '/^[[:space:]]*#/d' + "$CDT_CPP" -E "$1" 2>/dev/null | sed '/^[[:space:]]*#[[:space:]]*[0-9]/d' } # Decide whether one dispatch file satisfies the contract. Echoes OK, or a reason. @@ -210,14 +212,23 @@ mkbad macro_expanded ' #include "record_again.hpp" sysio_set_contract_name(r); RECORD_AGAIN; if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' +# A multiline raw string closing at column 1 after a '#', with the second call on that same +# line. cdt-cpp compiles it and -E emits both calls; a filter that drops every '#'-prefixed +# line deletes the closing delimiter and the call with it. +mkbad raw_string_hash ' + sysio_set_contract_name(r); + const char* s = R"d( +#)d"; sysio_set_contract_name(c); + (void)s; + if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' mkbad missing_entirely ' if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' mkbad after_dispatch ' if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); } sysio_set_contract_name(r);' for bad in code_not_receiver inside_branch signature_line double_call comment_split \ - spliced_call spliced_ws raw_string_comment target_conditional macro_expanded \ - missing_entirely after_dispatch; do + spliced_call spliced_ws raw_string_comment raw_string_hash \ + target_conditional macro_expanded missing_entirely after_dispatch; do # Compiled by the DRIVER, not a host clang: a counterexample must be legal in the # translation unit that actually ships, and the driver supplies the wasm32 target and the # CDT include graph. (`-c` to an object we discard; the driver has no -fsyntax-only.) From 2b8dcd30b2e5a5b4dbd5d2252c348070d9ad127d Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Wed, 2 Sep 2026 20:48:19 -0500 Subject: [PATCH 23/28] test(kv): match the whole line-marker grammar, not a prefix Third raw-string escape past this filter. Dropping every '#'-prefixed line deleted `#)d"; ...`; dropping '#' followed by a digit deleted `#1)d"; ...`. Both took the executable code on the closing-delimiter line with them, and the checker returned OK on a dispatcher cdt-cpp compiles. Matching the complete grammar instead -- `# ""` with optional trailing flags, anchored at both ends -- leaves any line that is not literally a marker intact. Verified against the real emission: # 1 "/path/to/t.cpp" # 1 "" 1 raw_string_hash_num added as a counterexample; reverting to the numeric prefix fails it, and the earlier raw_string_hash still fails the broad form. Eighteen assertions. ctest 31/31. --- tests/unit/dispatch_receiver_tests.sh | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/tests/unit/dispatch_receiver_tests.sh b/tests/unit/dispatch_receiver_tests.sh index 739dfbc1c..ebb75cdc7 100755 --- a/tests/unit/dispatch_receiver_tests.sh +++ b/tests/unit/dispatch_receiver_tests.sh @@ -39,11 +39,15 @@ trap 'rm -rf "$WORK"' EXIT # wasm32 target, the CDT include graph and the same predefined macros as the real compile. # # -E emits line markers (no -P: the driver rejects it), so those are dropped afterwards -- -# NUMERIC ones specifically, `# ""`. Dropping every line that starts with '#' -# also deletes a multiline raw string whose closing delimiter sits at column 1 after a '#', -# taking the executable code that follows it on that line with it. +# matching the COMPLETE marker grammar, `# ""` with optional trailing flags, +# anchored at both ends. +# +# Two narrower filters were each defeated by a raw string whose closing delimiter sits at +# column 1: dropping every '#'-prefixed line deleted `#)d"; ...`, and dropping `#` followed by +# a digit deleted `#1)d"; ...`. Both took the executable code on that line with them. Anchoring +# the whole grammar leaves any line that is not literally a marker intact. normalise_source() { - "$CDT_CPP" -E "$1" 2>/dev/null | sed '/^[[:space:]]*#[[:space:]]*[0-9]/d' + "$CDT_CPP" -E "$1" 2>/dev/null | sed -E '/^# [0-9]+ "[^"]*"([[:space:]]+[0-9]+)*$/d' } # Decide whether one dispatch file satisfies the contract. Echoes OK, or a reason. @@ -221,13 +225,21 @@ mkbad raw_string_hash ' #)d"; sysio_set_contract_name(c); (void)s; if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' +# The same raw-string escape, with a digit after the '#'. A filter matching `#` plus a numeric +# prefix deletes this closing line and the call on it. +mkbad raw_string_hash_num ' + sysio_set_contract_name(r); + const char* s = R"d( +#1)d"; sysio_set_contract_name(c); + (void)s; + if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' mkbad missing_entirely ' if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' mkbad after_dispatch ' if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); } sysio_set_contract_name(r);' for bad in code_not_receiver inside_branch signature_line double_call comment_split \ - spliced_call spliced_ws raw_string_comment raw_string_hash \ + spliced_call spliced_ws raw_string_comment raw_string_hash raw_string_hash_num \ target_conditional macro_expanded missing_entirely after_dispatch; do # Compiled by the DRIVER, not a host clang: a counterexample must be legal in the # translation unit that actually ships, and the driver supplies the wasm32 target and the From ba6b89dfef36a636ebd3861ec699a2c0e969ce2a Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Thu, 3 Sep 2026 14:33:56 -0500 Subject: [PATCH 24/28] test(kv): model the whole marker filename, and stop reading a failed preprocess Three findings from review, each verified by mutation against the committed script. Line markers: the filename was modelled as `[^"]*`, which does not match what clang emits for a `#line` directive carrying an escaped quote -- `# 7 "a\"b.cpp"`. The marker survived normalisation and was then read as the start of apply()'s first statement, so a CORRECT dispatch was rejected. The filename now uses the complete grammar, `"([^"\\]|\\.)*"`, and `marker_escaped_ok` pins it as a positive control. Preprocessing failures were read as verdicts. Every call site invokes check_dispatch on the left of a `||`, which disables errexit for its whole body, so a non-zero driver status was ignored and whatever it had already printed was analysed: plausible output as acceptance, truncated output as a rejection -- and a rejection, in the counterexample table, reads as a PASS. The status is now checked explicitly and reported as INFRA_ERROR, distinct from REJECTED, and `classify_counterexample` separates the two at the call site rather than folding them into `-ne 0`. Section 3 pins both halves with a stand-in preprocessor that prints, then fails. The trailing `$` on the marker pattern was unpinned: the two raw-string rows only showed the old '#'-prefix filters were too broad, and dropping the anchor left all 18 checks green. `raw_string_marker` closes its raw string on a line whose prefix is a complete marker, `# 1 "fake")d"; `, so dropping the anchor deletes the line and the second call with it. mkbad/bad_* renamed to mkfixture/fixture_*, now that the table has positive controls as well as counterexamples, and the two positive controls share the loop the counterexamples use. --- tests/unit/dispatch_receiver_tests.sh | 310 +++++++++++++++++++------- 1 file changed, 228 insertions(+), 82 deletions(-) diff --git a/tests/unit/dispatch_receiver_tests.sh b/tests/unit/dispatch_receiver_tests.sh index ebb75cdc7..1fdc32e03 100755 --- a/tests/unit/dispatch_receiver_tests.sh +++ b/tests/unit/dispatch_receiver_tests.sh @@ -8,53 +8,89 @@ # stays green if that is broken -- the divergence appears only under notification, on chain. # This pins it at the source: the emitted dispatch text, which no other test inspects. # -# The checker is a function over a dispatch FILE, and it is exercised twice: against the real -# generated dispatch, and against a table of crafted counterexamples that must each be -# rejected. Earlier revisions of this test were defeated four times in review -- by handler -# names it did not match, by a branch on the signature line, by two calls on one line, and by a -# comment between the identifier and its paren -- because each fix pattern-matched the last -# evasion. Checking the checker is what stops that: a new evasion is one row below, not a round -# trip. +# The checker is a function over a dispatch FILE, and it is exercised three ways: against the +# real generated dispatch, against a table of crafted counterexamples that must each be +# rejected, and against positive controls that must NOT be rejected. Earlier revisions of this +# test were defeated six times in review -- by handler names it did not match, by a branch on +# the signature line, by two calls on one line, by a comment between the identifier and its +# paren, by a raw string closing at column 1, and by a line marker whose filename contained an +# escaped quote -- because each fix pattern-matched the last evasion. Checking the checker is +# what stops that: a new evasion is one row below, not a round trip. +# +# The checker reports three outcomes, not two, and callers distinguish all three: accepted, +# rejected, and INFRA_ERROR -- the check could not be performed. Collapsing the third into +# either verdict is how a broken toolchain reads as a green run. # # Usage: dispatch_receiver_tests.sh set -euo pipefail BUILD_DIR="$1" CDT_CPP="${BUILD_DIR}/bin/cdt-cpp" -CLANGXX="${BUILD_DIR}/bin/clang++" PASS=0 FAIL=0 pass() { echo " PASS: $1"; PASS=$((PASS + 1)); } fail() { echo " FAIL: $1"; FAIL=$((FAIL + 1)); } +# check_dispatch's exit statuses. 0 is acceptance; these two are not interchangeable. +readonly REJECTED=1 # the dispatch was read, and it breaks the contract +readonly INFRA_ERROR=2 # the dispatch could not be read at all -- no verdict was reached + WORK="$(mktemp -d)" trap 'rm -rf "$WORK"' EXIT -# Normalise through the CDT DRIVER, so the tokens counted are the ones that will ship. +# The COMPLETE preprocessor line-marker grammar, anchored at BOTH ends: +# +# # "" [...] +# +# with the filename modelled as clang emits it -- any character except an unescaped quote or +# backslash, or a backslash followed by anything. Every part of that is load-bearing, and each +# was added after a shape that a narrower filter got wrong: +# +# * dropping every '#'-prefixed line deleted `#)d"; ` -- a raw string closing at +# column 1 -- and the executable code on that line with it; +# * dropping `#` followed by a digit deleted `#1)d"; ` the same way; +# * `[^"]*` for the filename does not match `# 7 "a\"b.cpp"`, which is what cdt-cpp -E emits +# for `#line 7 "a\"b.cpp"`. The marker then survives into the normalised source and is +# read as the start of apply()'s first statement, rejecting a CORRECT dispatch; +# * without the trailing `$`, `# 1 "fake")d"; ` matches on its marker-shaped prefix +# and the line -- call included -- is deleted. +# +# Matching the whole grammar leaves any line that is not literally a marker intact. +readonly LINE_MARKER_RE='^# [0-9]+ "([^"\\]|\\.)*"([[:space:]]+[0-9]+)*$' + +# Preprocess $1 into $2, with the driver's diagnostics captured in $3, and strip line markers. # -# Earlier revisions used the bundled clang++ with the host target and deleted #include lines -# first. Both choices changed the translation unit: the host target evaluates `#ifdef __wasm__` -# the wrong way, so a second setter call guarded on it was invisible while cdt-cpp compiled it +# Normalise through the CDT DRIVER, so the tokens counted are the ones that will ship. Earlier +# revisions used the bundled clang++ with the host target and deleted #include lines first. +# Both choices changed the translation unit: the host target evaluates `#ifdef __wasm__` the +# wrong way, so a second setter call guarded on it was invisible while cdt-cpp compiled it # happily; and dropping includes erases any macro that expands to one. cdt-cpp applies the # wasm32 target, the CDT include graph and the same predefined macros as the real compile. # -# -E emits line markers (no -P: the driver rejects it), so those are dropped afterwards -- -# matching the COMPLETE marker grammar, `# ""` with optional trailing flags, -# anchored at both ends. -# -# Two narrower filters were each defeated by a raw string whose closing delimiter sits at -# column 1: dropping every '#'-prefixed line deleted `#)d"; ...`, and dropping `#` followed by -# a digit deleted `#1)d"; ...`. Both took the executable code on that line with them. Anchoring -# the whole grammar leaves any line that is not literally a marker intact. +# -E emits line markers and the driver rejects -P, so they are stripped afterwards. `pipefail` +# is set, so the pipeline reports the driver's status and the caller can tell a preprocessing +# failure from a verdict. normalise_source() { - "$CDT_CPP" -E "$1" 2>/dev/null | sed -E '/^# [0-9]+ "[^"]*"([[:space:]]+[0-9]+)*$/d' + "$CDT_CPP" -E "$1" 2>"$3" | sed -E "/${LINE_MARKER_RE}/d" > "$2" } # Decide whether one dispatch file satisfies the contract. Echoes OK, or a reason. +# Returns 0 (accepted), $REJECTED, or $INFRA_ERROR. check_dispatch() { - local file="$1" clean - clean="$(mktemp)" - normalise_source "$file" > "$clean" + local file="$1" clean pp_log pp_status=0 + clean="$(mktemp "${WORK}/clean.XXXXXX")" + pp_log="$(mktemp "${WORK}/pplog.XXXXXX")" + + # An explicit status check, because every call site runs this function on the left of a + # `||` -- which disables errexit for its whole body. Without this, a driver that failed + # after printing something plausible was analysed anyway: acceptable-looking output read + # as OK, and truncated output read as a rejection, which in the counterexample loop below + # is indistinguishable from a PASS. + normalise_source "$file" "$clean" "$pp_log" || pp_status=$? + if [ "$pp_status" -ne 0 ]; then + echo "preprocessing ${file} exited ${pp_status}: $(tr '\n' ' ' < "$pp_log")" + return "$INFRA_ERROR" + fi # Every occurrence of the identifier, however spelled. Two are expected: the declaration # in the extern "C" block and the single call inside apply(). Counting matching LINES, or @@ -63,14 +99,14 @@ check_dispatch() { occurrences="$(grep -owE 'sysio_set_contract_name' "$clean" | wc -l)" if [ "$occurrences" -ne 2 ]; then echo "expected 2 occurrences of sysio_set_contract_name (1 declaration + 1 call), found ${occurrences}" - rm -f "$clean"; return 1 + return "$REJECTED" fi local apply_line apply_line="$(grep -nE '^[[:space:]]*(__attribute__.*)?void apply\(' "$clean" | head -1 | cut -d: -f1 || true)" if [ -z "$apply_line" ]; then echo "no apply() definition found" - rm -f "$clean"; return 1 + return "$REJECTED" fi # The body from immediately after the opening brace, INCLUDING any suffix on the signature @@ -83,18 +119,29 @@ check_dispatch() { { print } ' "$clean" | tr '\n' ' ' | sed 's/[[:space:]][[:space:]]*/ /g; s/^ //')" first_stmt="${body%%;*};" - rm -f "$clean" # Normalise spacing so `set (r)` and `set(r)` compare alike. local normalised normalised="$(printf '%s' "$first_stmt" | tr -d ' ')" if [ "$normalised" != "sysio_set_contract_name(r);" ]; then echo "first statement of apply() is: ${first_stmt}" - return 1 + return "$REJECTED" fi echo OK } +# Run check_dispatch on $1, setting VERDICT to its reason and VERDICT_STATUS to its status. +# +# The `||` is what keeps a non-zero status from aborting the script under errexit -- turning a +# reported failure into a truncated run -- while still recording which status it was. Reading +# only the text cannot tell a rejection from an infrastructure error. +VERDICT="" +VERDICT_STATUS=0 +run_check() { + VERDICT_STATUS=0 + VERDICT="$(check_dispatch "$1")" || VERDICT_STATUS=$? +} + echo "=== Dispatch Receiver Tests ===" # --- 1. the real generated dispatch -------------------------------------------------------- @@ -125,27 +172,26 @@ if [ -z "$DISPATCH" ]; then fi pass "a dispatch.cpp was generated" -# `|| true`: check_dispatch returns non-zero on rejection, and under `set -e` the assignment -# would take that status and abort the script -- turning a real failure into a truncated run -# instead of a reported one. -verdict="$(check_dispatch "$DISPATCH" || true)" -if [ "$verdict" = OK ]; then +run_check "$DISPATCH" +if [ "$VERDICT_STATUS" -eq 0 ]; then pass "the generated apply() records the receiver once, before any dispatch" else fail "the generated apply() records the receiver once, before any dispatch" - echo " ${verdict}" + echo " ${VERDICT}" sed 's/^/ /' "$DISPATCH" fi # --- 2. the checker itself ----------------------------------------------------------------- # -# Each of these compiles and each breaks the contract. Every one defeated some earlier -# revision of this test, so they are kept as regressions on the CHECKER. +# Each fixture compiles. The ones in the negative table each break the contract and every one +# defeated some earlier revision of this test, so they are kept as regressions on the CHECKER; +# the ones in the positive table are correct dispatches that merely look unusual, and pin the +# other direction -- a filter tightened until it rejects real output. # $2 is appended directly after the opening brace, so a fixture can put text on the SIGNATURE -# line by starting without a newline. mkbad used to emit one unconditionally, which meant no +# line by starting without a newline. This used to emit one unconditionally, which meant no # counterexample ever exercised that escape even though the header claimed one did. -mkbad() { # $1=name $2=apply-body (leading newline optional) - cat > "${WORK}/bad_$1.cpp" < "${WORK}/fixture_$1.cpp" <: see the -nostdinc note below extern "C" { void sysio_set_contract_name(uint64_t n); @@ -157,16 +203,16 @@ extern "C" { EOF } -mkbad code_not_receiver ' +mkfixture code_not_receiver ' sysio_set_contract_name(c); if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' -mkbad inside_branch ' +mkfixture inside_branch ' if (c == r) { sysio_set_contract_name(r); __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' -mkbad double_call ' +mkfixture double_call ' sysio_set_contract_name(r); sysio_set_contract_name(c); if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' -mkbad comment_split ' +mkfixture comment_split ' sysio_set_contract_name(r); sysio_set_contract_name/**/(c); if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' # A branch opened on the SIGNATURE line, with the setter first on the line below. This is the @@ -174,32 +220,32 @@ mkbad comment_split ' # `if (c == r) {`, so it is rejected; read it from the next line down -- as an earlier revision # did -- and the setter looks like the first statement and it is accepted. A fixture whose # signature line also closes its branch is rejected either way and pins nothing. -mkbad signature_line ' if (c == r) { +mkfixture signature_line ' if (c == r) { sysio_set_contract_name(r); __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' # Split across a phase-2 line splice, which compiles as one identifier. -mkbad spliced_call ' +mkfixture spliced_call ' sysio_set_contract_name(r); sysio_set_contract_\ name(c); if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' # A backslash followed by horizontal whitespace before the newline. Clang splices it (with a # warning), so this is a second call; a normaliser matching only an adjacent backslash-newline # does not see it. -mkbad spliced_ws ' +mkfixture spliced_ws ' sysio_set_contract_name(r); sysio_set_contract_\ name(c); if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' # A raw string whose contents look like a comment. A hand-rolled lexer treats the opening quote # as an ordinary string and the // inside it as a comment, erasing the real call after it. -mkbad raw_string_comment ' +mkfixture raw_string_comment ' sysio_set_contract_name(r); const char* s = R"d(" // )d"; sysio_set_contract_name(c); (void)s; if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' # Guarded on the target. The host branch is well-formed, so a checker preprocessing for the # host counts two occurrences and accepts -- while cdt-cpp compiles the wasm branch, where the # receiver is immediately overwritten with the code. -mkbad target_conditional ' +mkfixture target_conditional ' #ifdef __wasm__ sysio_set_contract_name(r); sysio_set_contract_name(c); #else @@ -212,14 +258,14 @@ cat > "${WORK}/record_again.hpp" <<'EOF' #pragma once #define RECORD_AGAIN sysio_set_contract_name(c) EOF -mkbad macro_expanded ' +mkfixture macro_expanded ' #include "record_again.hpp" sysio_set_contract_name(r); RECORD_AGAIN; if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' # A multiline raw string closing at column 1 after a '#', with the second call on that same # line. cdt-cpp compiles it and -E emits both calls; a filter that drops every '#'-prefixed # line deletes the closing delimiter and the call with it. -mkbad raw_string_hash ' +mkfixture raw_string_hash ' sysio_set_contract_name(r); const char* s = R"d( #)d"; sysio_set_contract_name(c); @@ -227,53 +273,153 @@ mkbad raw_string_hash ' if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' # The same raw-string escape, with a digit after the '#'. A filter matching `#` plus a numeric # prefix deletes this closing line and the call on it. -mkbad raw_string_hash_num ' +mkfixture raw_string_hash_num ' sysio_set_contract_name(r); const char* s = R"d( #1)d"; sysio_set_contract_name(c); (void)s; if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' -mkbad missing_entirely ' +# The same escape again, but closing on a line whose PREFIX is a complete, well-formed line +# marker. This is what pins the trailing `$`: a filter anchored only at the start matches the +# `# 1 "fake"` prefix, deletes the line, and takes the second call with it -- so the run stays +# green with the anchor removed unless this row is here. The two rows above do not cover it; +# they only pin that the old '#'-prefix filters were too broad. +mkfixture raw_string_marker ' + sysio_set_contract_name(r); + const char* s = R"d( +# 1 "fake")d"; sysio_set_contract_name(c); + (void)s; + if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' +mkfixture missing_entirely ' if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' -mkbad after_dispatch ' if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); } +mkfixture after_dispatch ' if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); } sysio_set_contract_name(r);' -for bad in code_not_receiver inside_branch signature_line double_call comment_split \ - spliced_call spliced_ws raw_string_comment raw_string_hash raw_string_hash_num \ - target_conditional macro_expanded missing_entirely after_dispatch; do - # Compiled by the DRIVER, not a host clang: a counterexample must be legal in the - # translation unit that actually ships, and the driver supplies the wasm32 target and the - # CDT include graph. (`-c` to an object we discard; the driver has no -fsyntax-only.) - if ! ( cd "$WORK" && "$CDT_CPP" -c "bad_${bad}.cpp" -o "bad_${bad}.o" ) \ - > "${WORK}/bad_${bad}.log" 2>&1; then - fail "counterexample compiles: ${bad}" - sed 's/^/ /' "${WORK}/bad_${bad}.log" - continue +# The positive controls: correct dispatches whose text is awkward. +mkfixture spaced_ok ' + sysio_set_contract_name (r); + if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' +# A #line directive whose filename carries an ESCAPED QUOTE, immediately before the setter. +# cdt-cpp -E re-emits it as the legal marker `# 7 "a\"b.cpp"`; a filter modelling the filename +# as `[^"]*` cannot match that, leaves the marker in the normalised source, and then reads it +# as the start of apply()'s first statement -- rejecting a dispatch that is correct. +mkfixture marker_escaped_ok ' +#line 7 "a\"b.cpp" + sysio_set_contract_name(r); + if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' + +# Compiled by the DRIVER, not a host clang: a fixture must be legal in the translation unit +# that actually ships, and the driver supplies the wasm32 target and the CDT include graph. +# (`-c` to an object we discard; the driver has no -fsyntax-only.) Echoes non-zero if the +# fixture did not compile, having already reported the failure. +compile_fixture() { + if ( cd "$WORK" && "$CDT_CPP" -c "fixture_$1.cpp" -o "fixture_$1.o" ) \ + > "${WORK}/fixture_$1.log" 2>&1; then + return 0 fi - verdict="$(check_dispatch "${WORK}/bad_${bad}.cpp" || true)" - if [ "$verdict" = OK ]; then - fail "the checker rejects: ${bad}" - echo " accepted a dispatch that breaks the contract" - sed 's/^/ /' "${WORK}/bad_${bad}.cpp" + fail "fixture compiles: $1" + sed 's/^/ /' "${WORK}/fixture_$1.log" + return 1 +} + +# Decide whether one counterexample was CAUGHT, which is the outcome the table requires: +# rejected, having been read. Returns 0 for that, and non-zero -- echoing why -- for either +# other outcome. Acceptance is the obvious failure; an infrastructure error is the quiet one, +# and folding it in with `-ne 0` would report a full green sweep on a machine where the driver +# cannot run at all. Section 3 pins that this distinction is made HERE, at the call site, and +# not only inside check_dispatch. +classify_counterexample() { # $1=fixture file + run_check "$1" + if [ "$VERDICT_STATUS" -eq "$REJECTED" ]; then + return 0 + fi + if [ "$VERDICT_STATUS" -eq 0 ]; then + echo "accepted a dispatch that breaks the contract:" + sed 's/^/ /' "$1" else + echo "no verdict was reached: ${VERDICT}" + fi + return 1 +} + +for bad in code_not_receiver inside_branch signature_line double_call comment_split \ + spliced_call spliced_ws raw_string_comment raw_string_hash raw_string_hash_num \ + raw_string_marker target_conditional macro_expanded missing_entirely \ + after_dispatch; do + compile_fixture "$bad" || continue + reason=""; caught=0 + reason="$(classify_counterexample "${WORK}/fixture_${bad}.cpp")" || caught=$? + if [ "$caught" -eq 0 ]; then pass "the checker rejects: ${bad}" + else + fail "the checker rejects: ${bad}" + printf '%s\n' "$reason" | sed 's/^/ /' fi done # ...and must not reject a well-formed one that merely looks unusual. -mkbad spaced_ok ' - sysio_set_contract_name (r); - if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' -if ! ( cd "$WORK" && "$CDT_CPP" -c bad_spaced_ok.cpp -o bad_spaced_ok.o ) > /dev/null 2>&1; then - fail "the positive control compiles" -fi -verdict="$(check_dispatch "${WORK}/bad_spaced_ok.cpp" || true)" -if [ "$verdict" = OK ]; then - pass "the checker accepts: a space before the paren" -else - fail "the checker accepts: a space before the paren" - echo " ${verdict}" -fi +for good in spaced_ok marker_escaped_ok; do + compile_fixture "$good" || continue + run_check "${WORK}/fixture_${good}.cpp" + if [ "$VERDICT_STATUS" -eq 0 ]; then + pass "the checker accepts: ${good}" + else + fail "the checker accepts: ${good}" + echo " ${VERDICT}" + fi +done + +# --- 3. a failing preprocessor is an infrastructure error, not a verdict -------------------- +# +# Stand in for the driver with something that prints, then fails. Both shapes below used to be +# reported as verdicts, because check_dispatch never looked at the status: plausible output +# read as acceptance, and truncated output read as a rejection -- which, in the loop above, +# reads as a PASS on a machine where the toolchain is broken. +cat > "${WORK}/fake_cdt_cpp" <<'EOF' +#!/bin/bash +# Prints a canned payload and exits with a canned status, both read from files beside it, so +# one stand-in covers every shape of preprocessor failure. +cat "$(dirname "$0")/fake_pp_out" +exit "$(cat "$(dirname "$0")/fake_pp_status")" +EOF +chmod +x "${WORK}/fake_cdt_cpp" +printf '73\n' > "${WORK}/fake_pp_status" + +# Output that WOULD be accepted, so only the status can distinguish it. +cat > "${WORK}/fake_pp_out" <<'EOF' +extern "C" { + void sysio_set_contract_name(unsigned long long n); + void apply(unsigned long long r, unsigned long long c, unsigned long long a) { + sysio_set_contract_name(r); + } +} +EOF + +real_cdt_cpp="$CDT_CPP" +CDT_CPP="${WORK}/fake_cdt_cpp" +for shape in acceptable_output truncated_output; do + [ "$shape" = truncated_output ] && : > "${WORK}/fake_pp_out" + run_check "${WORK}/c.cpp" + if [ "$VERDICT_STATUS" -eq "$INFRA_ERROR" ]; then + pass "a failing preprocessor reaches no verdict: ${shape}" + else + fail "a failing preprocessor reaches no verdict: ${shape}" + echo " status ${VERDICT_STATUS}: ${VERDICT}" + fi + + # ...and the counterexample table must not read that as a catch. This is the half that a + # status alone does not buy: every row above reports a PASS for any non-zero status unless + # the call site separates the two, so a driver that cannot run would sweep the table green. + reason=""; caught=0 + reason="$(classify_counterexample "${WORK}/fixture_code_not_receiver.cpp")" || caught=$? + if [ "$caught" -ne 0 ]; then + pass "a counterexample is not counted as caught: ${shape}" + else + fail "a counterexample is not counted as caught: ${shape}" + echo " the table reported a catch though no verdict was reached" + fi +done +CDT_CPP="$real_cdt_cpp" echo "" echo "Results: ${PASS} passed, ${FAIL} failed" From e5dba38ba16562c66ffa7a006f091695df3284e5 Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Thu, 3 Sep 2026 14:50:15 -0500 Subject: [PATCH 25/28] test(kv): pin the marker filter from above as well as below The previous commit pinned the filter only against being too NARROW. Two independent one-token weakenings left all 24 checks green while making the checker accept a dispatch that calls the setter twice, the second time with the code: * dropping the leading `^` -- a raw string OPENING on the same line as the second call, whose remainder is a well-formed marker, matches on its tail; sed deletes the line and the call with it, 3 occurrences fall to 2; * modelling the filename as `.*` -- a closing delimiter, the second call and a later quoted string on one line let the greedy match run from the first quote to the last, swallowing the call. Both compile through the driver and both are verified against it. `marker_tail` and `marker_greedy` cover them; each fails when its own part of the pattern is weakened and nothing else does. Also: the `-nostdinc` note the fixture header pointed at went away in 7e5a89ca2 when compilation moved to the driver, and compile_fixture returns its status rather than echoing it. --- tests/unit/dispatch_receiver_tests.sh | 40 +++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/tests/unit/dispatch_receiver_tests.sh b/tests/unit/dispatch_receiver_tests.sh index 1fdc32e03..80980e460 100755 --- a/tests/unit/dispatch_receiver_tests.sh +++ b/tests/unit/dispatch_receiver_tests.sh @@ -17,6 +17,12 @@ # escaped quote -- because each fix pattern-matched the last evasion. Checking the checker is # what stops that: a new evasion is one row below, not a round trip. # +# The marker filter is pinned from BOTH sides. Too narrow and it leaves a marker in the +# normalised source, which is read as apply()'s first statement and rejects a correct dispatch; +# too broad and it deletes a line of real code, taking a second setter call with it. Each of +# the four parts of that pattern -- the `^`, the filename grammar, the trailing flags and the +# `$` -- has a row that fails when it alone is weakened. +# # The checker reports three outcomes, not two, and callers distinguish all three: accepted, # rejected, and INFRA_ERROR -- the check could not be performed. Collapsing the third into # either verdict is how a broken toolchain reads as a green run. @@ -192,7 +198,8 @@ fi # counterexample ever exercised that escape even though the header claimed one did. mkfixture() { # $1=name $2=apply-body (leading newline optional) cat > "${WORK}/fixture_$1.cpp" <: see the -nostdinc note below +typedef unsigned long long uint64_t; // not : keeps the preprocessed fixture small + // enough to read when a failure dumps it extern "C" { void sysio_set_contract_name(uint64_t n); void __sysio_action_go_x(uint64_t r, uint64_t c); @@ -290,6 +297,29 @@ mkfixture raw_string_marker ' # 1 "fake")d"; sysio_set_contract_name(c); (void)s; if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' +# A marker-shaped SUFFIX: a raw string OPENING on the same line as the second call, whose +# remainder is a well-formed marker. This pins the leading `^`. Without it the line matches on +# its tail, sed deletes the whole line, and the second call goes with it -- 3 occurrences drop +# to 2 and the checker accepts. Every other raw-string row closes at column 1, so none of them +# can pin the start anchor. +mkfixture marker_tail ' + sysio_set_contract_name(r); + sysio_set_contract_name(c); const char* s = R"z(# 1 "a" +)z"; + (void)s; + if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' +# The filename model from ABOVE, where marker_escaped_ok only pins it from below. A closing +# delimiter, the second call, and a later quoted string all on one line: model the filename as +# `.*` and the greedy match runs from the first quote to the last, swallowing the call. The +# real grammar stops at the unescaped quote that ends the filename, so the line is not a marker +# and survives intact. +mkfixture marker_greedy ' + sysio_set_contract_name(r); + const char* s = R"d( +# 1 "x)d"; sysio_set_contract_name(c); const char* t = "y" + ; + (void)s; (void)t; + if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' mkfixture missing_entirely ' if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' mkfixture after_dispatch ' if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); } @@ -310,8 +340,8 @@ mkfixture marker_escaped_ok ' # Compiled by the DRIVER, not a host clang: a fixture must be legal in the translation unit # that actually ships, and the driver supplies the wasm32 target and the CDT include graph. -# (`-c` to an object we discard; the driver has no -fsyntax-only.) Echoes non-zero if the -# fixture did not compile, having already reported the failure. +# (`-c` to an object we discard; the driver has no -fsyntax-only.) Returns non-zero if the +# fixture did not compile, having already reported the failure itself. compile_fixture() { if ( cd "$WORK" && "$CDT_CPP" -c "fixture_$1.cpp" -o "fixture_$1.o" ) \ > "${WORK}/fixture_$1.log" 2>&1; then @@ -344,8 +374,8 @@ classify_counterexample() { # $1=fixture file for bad in code_not_receiver inside_branch signature_line double_call comment_split \ spliced_call spliced_ws raw_string_comment raw_string_hash raw_string_hash_num \ - raw_string_marker target_conditional macro_expanded missing_entirely \ - after_dispatch; do + raw_string_marker marker_tail marker_greedy target_conditional macro_expanded \ + missing_entirely after_dispatch; do compile_fixture "$bad" || continue reason=""; caught=0 reason="$(classify_counterexample "${WORK}/fixture_${bad}.cpp")" || caught=$? From 6dbf32d391a05b1b5badc279f6a792aacaea6daf Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Fri, 4 Sep 2026 07:24:59 -0500 Subject: [PATCH 26/28] test(kv): count calls in the object, and pin the marker flags Two findings from review. A source-text count sees spellings, and a second call can reach the same wasm import without adding one: extern void again(uint64_t) __asm__("sysio_set_" "contract_name"); again(c); The adjacent string literals are still two tokens after preprocessing -- concatenation is translation phase 6, which -E does not reach -- so the identifier is spelled twice in the file and the text count stays at 2. The object calls the import twice and the second overwrites the receiver with the code. check_dispatch_symbols reads the relocations of the object the driver emits: exactly one call to sysio_set_contract_name inside apply(), and it is the first call there. That states "exactly once, before any dispatch" over the code that ships rather than over the source text, and it does not care how the symbol was spelled. Neither checker subsumes the other -- the text one is what sees that the argument is `r` and not `c` -- so both run, each against the real dispatch, a counterexample and a positive control. asm_label is kept as the counterexample it is, and the run reports that the text checker accepts it rather than asserting so, since a future text checker strong enough to catch it should not fail the suite. The trailing `([[:space:]]+[0-9]+)*` of the marker pattern was unpinned: every marker in these fixtures was flagless, so removing the group left all 26 checks green. marker_flags_ok puts an #include immediately before the setter, which makes CDT emit enter and return markers carrying `1` and `2` between the opening brace and the first statement. Scope of the symbol check is apply() itself, matching the text checker: a setter call made from another function apply() calls is out of range of both. The dispatch TU defines only apply(), so there is no such function to write today, but it is a limit rather than a covered case, and the comment says so. --- tests/unit/dispatch_receiver_tests.sh | 145 ++++++++++++++++++++++++-- 1 file changed, 134 insertions(+), 11 deletions(-) diff --git a/tests/unit/dispatch_receiver_tests.sh b/tests/unit/dispatch_receiver_tests.sh index 80980e460..73beb0581 100755 --- a/tests/unit/dispatch_receiver_tests.sh +++ b/tests/unit/dispatch_receiver_tests.sh @@ -6,22 +6,29 @@ # dispatcher stores `r` (the receiver) rather than `c` (the code), exactly once, before any # dispatch. Every in-tree action is self-sent, so r == c and the whole unit + integration suite # stays green if that is broken -- the divergence appears only under notification, on chain. -# This pins it at the source: the emitted dispatch text, which no other test inspects. +# This pins it at the source, which no other test inspects, in two independent ways. # -# The checker is a function over a dispatch FILE, and it is exercised three ways: against the -# real generated dispatch, against a table of crafted counterexamples that must each be -# rejected, and against positive controls that must NOT be rejected. Earlier revisions of this -# test were defeated six times in review -- by handler names it did not match, by a branch on -# the signature line, by two calls on one line, by a comment between the identifier and its -# paren, by a raw string closing at column 1, and by a line marker whose filename contained an -# escaped quote -- because each fix pattern-matched the last evasion. Checking the checker is -# what stops that: a new evasion is one row below, not a round trip. +# check_dispatch reads the preprocessed dispatch TEXT: it is what can see that the argument is +# `r` and not `c`, and that the call is the first statement. check_dispatch_symbols reads the +# RELOCATIONS of the emitted object: it is what can see a second call however it was spelled, +# including one reaching the import through an asm label that never spells the identifier +# twice. Neither subsumes the other, so both run. +# +# Each is exercised three ways: against the real generated dispatch, against a table of crafted +# counterexamples that must each be rejected, and against positive controls that must NOT be. +# Earlier revisions were defeated seven times in review -- by handler names the checker did not +# match, by a branch on the signature line, by two calls on one line, by a comment between the +# identifier and its paren, by a raw string closing at column 1, by a marker whose filename +# carried an escaped quote, and by that asm label -- because each fix pattern-matched the last +# evasion. Checking the checker is what stops that: a new evasion is one row below, not a round +# trip. # # The marker filter is pinned from BOTH sides. Too narrow and it leaves a marker in the # normalised source, which is read as apply()'s first statement and rejects a correct dispatch; # too broad and it deletes a line of real code, taking a second setter call with it. Each of # the four parts of that pattern -- the `^`, the filename grammar, the trailing flags and the -# `$` -- has a row that fails when it alone is weakened. +# `$` -- has a row that fails when it alone is weakened, positive rows for the first sense and +# counterexamples for the second. # # The checker reports three outcomes, not two, and callers distinguish all three: accepted, # rejected, and INFRA_ERROR -- the check could not be performed. Collapsing the third into @@ -32,6 +39,7 @@ set -euo pipefail BUILD_DIR="$1" CDT_CPP="${BUILD_DIR}/bin/cdt-cpp" +LLVM_OBJDUMP="${BUILD_DIR}/bin/llvm-objdump" PASS=0 FAIL=0 pass() { echo " PASS: $1"; PASS=$((PASS + 1)); } @@ -136,6 +144,52 @@ check_dispatch() { echo OK } +# The same requirement at the SYMBOL level, over the object the driver actually emits. +# +# The text checker counts SPELLINGS, and a second call can reach the same wasm import without +# adding one: +# +# extern void again(uint64_t) __asm__("sysio_set_" "contract_name"); +# again(c); +# +# The adjacent string literals are still two tokens after preprocessing -- concatenation is +# translation phase 6, which -E does not reach -- so the identifier is spelled twice in the +# file and the text count stays at 2. The object calls the import twice, and the second call +# overwrites the receiver with the code. Relocations do not care how the symbol was spelled. +# +# Ordering comes with it: the first call relocation inside apply() must be this one, which +# states "before any dispatch" over the emitted code rather than over the source text. +# +# Scope is apply() itself, matching the text checker. A setter call made from some OTHER +# function that apply() calls is out of range of both -- the dispatch TU defines only apply(), +# so there is no such function to write today, but it is a real limit rather than a covered +# case. +# +# Echoes OK, or a reason. Returns 0 or $REJECTED. +check_dispatch_symbols() { # $1=object file + local relocs count first + relocs="$("$LLVM_OBJDUMP" -dr "$1" 2>/dev/null | awk ' + /^[0-9a-f]+ <.*>:$/ { in_apply = ($0 ~ /:$/); next } + in_apply && /R_WASM_FUNCTION_INDEX_LEB/ { + sym = $NF; sub(/\+[0-9]+$/, "", sym); print sym + }')" + if [ -z "$relocs" ]; then + echo "no call relocations inside apply() in $(basename "$1")" + return "$REJECTED" + fi + count="$(printf '%s\n' "$relocs" | grep -cx 'sysio_set_contract_name' || true)" + if [ "$count" -ne 1 ]; then + echo "apply() calls sysio_set_contract_name ${count} time(s), not once" + return "$REJECTED" + fi + first="$(printf '%s\n' "$relocs" | head -1)" + if [ "$first" != sysio_set_contract_name ]; then + echo "the first call in apply() is ${first}, not sysio_set_contract_name" + return "$REJECTED" + fi + echo OK +} + # Run check_dispatch on $1, setting VERDICT to its reason and VERDICT_STATUS to its status. # # The `||` is what keeps a non-zero status from aborting the script under errexit -- turning a @@ -337,6 +391,27 @@ mkfixture marker_escaped_ok ' #line 7 "a\"b.cpp" sysio_set_contract_name(r); if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' +# An #include immediately before the setter. CDT emits an ENTER and a RETURN marker for it -- +# `# 1 "./empty_header.hpp" 1` and `# N "fixture.cpp" 2` -- between the opening brace and the +# first statement, which is the only shape that pins the trailing `([[:space:]]+[0-9]+)*`: +# drop that group and neither line is a marker any more, both survive normalisation, and the +# first is read as apply()'s first statement. Every other marker in these fixtures is +# flagless, so nothing else covers it. +cat > "${WORK}/empty_header.hpp" <<'HDREOF' +#pragma once +HDREOF +# Reaches the import through an asm label whose spelling is split across two string literals, +# so no second contiguous `sysio_set_contract_name` appears in the preprocessed text. Compiles +# under the driver; the text checker ACCEPTS it, which is the whole reason section 4 exists. +mkfixture asm_label ' + sysio_set_contract_name(r); + extern void again(uint64_t) __asm__("sysio_set_" "contract_name"); + again(c); + if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' +mkfixture marker_flags_ok ' +#include "empty_header.hpp" + sysio_set_contract_name(r); + if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' # Compiled by the DRIVER, not a host clang: a fixture must be legal in the translation unit # that actually ships, and the driver supplies the wasm32 target and the CDT include graph. @@ -388,7 +463,7 @@ for bad in code_not_receiver inside_branch signature_line double_call comment_sp done # ...and must not reject a well-formed one that merely looks unusual. -for good in spaced_ok marker_escaped_ok; do +for good in spaced_ok marker_escaped_ok marker_flags_ok; do compile_fixture "$good" || continue run_check "${WORK}/fixture_${good}.cpp" if [ "$VERDICT_STATUS" -eq 0 ]; then @@ -451,6 +526,54 @@ for shape in acceptable_output truncated_output; do done CDT_CPP="$real_cdt_cpp" +# --- 4. the same requirement over the emitted object --------------------------------------- +# +# Exercised the way the text checker is: against the real generated dispatch, against a +# positive control, and against the counterexample the text checker cannot see. +run_symbols() { # $1=label $2=object $3=expect: accept|reject + local verdict status=0 + verdict="$(check_dispatch_symbols "$2")" || status=$? + if [ "$3" = accept ] && [ "$status" -eq 0 ]; then + pass "the symbol check accepts: $1" + elif [ "$3" = reject ] && [ "$status" -ne 0 ]; then + pass "the symbol check rejects: $1" + else + fail "the symbol check ${3}s: $1" + echo " ${verdict}" + fi +} + +# The real dispatch, compiled on its own: the contract build above already links it, but the +# object is what carries the relocations. +if ( cd "$WORK" && "$CDT_CPP" -c "$DISPATCH" -o real_dispatch.o ) > "${WORK}/real.log" 2>&1; then + run_symbols "the generated dispatch" "${WORK}/real_dispatch.o" accept +else + fail "the generated dispatch compiles on its own" + sed 's/^/ /' "${WORK}/real.log" +fi + +run_symbols "a space before the paren" "${WORK}/fixture_spaced_ok.o" accept + +# The setter present exactly once but AFTER the dispatch. Its object is already built by the +# counterexample loop, and it is what pins the "first call" branch -- without it, deleting that +# branch leaves this section green. +run_symbols "the setter after the dispatch" "${WORK}/fixture_after_dispatch.o" reject + +# A compile failure here must not read as the rejection this row expects, so the check only +# runs once the object exists. +if compile_fixture asm_label; then + run_symbols "an asm label reaching the same import" "${WORK}/fixture_asm_label.o" reject +fi + +# ...and the text checker really does miss that one, which is why both run. Reported rather +# than asserted: a future text checker strong enough to catch it should not fail this suite. +run_check "${WORK}/fixture_asm_label.cpp" +if [ "$VERDICT_STATUS" -eq 0 ]; then + echo " NOTE: the text checker accepts asm_label, as expected -- only the symbol check sees it" +else + echo " NOTE: the text checker now also rejects asm_label (${VERDICT})" +fi + echo "" echo "Results: ${PASS} passed, ${FAIL} failed" [ "$FAIL" -eq 0 ] From 69207004f8e03ef2b275db656647a8ef57d3cb21 Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Fri, 4 Sep 2026 08:23:36 -0500 Subject: [PATCH 27/28] test(kv): refuse indirect calls, and give the analyser its own infra status Two findings from review. A relocation names the target of a DIRECT call. A call_indirect names only a type, so its target is exactly what relocations cannot show -- and the address reaches the table without a direct call appearing: setter_fn volatile fp = again; // R_WASM_TABLE_INDEX_SLEB, not FUNCTION_INDEX_LEB fp(c); // call_indirect which leaves one direct setter call, first, and a second call to the same import that a scan of call relocations cannot count. Both checkers returned OK while apply() overwrote the receiver with the code. The generated dispatch is a chain of direct calls and has no legitimate indirect one, so an indirect call in apply() is now refused rather than analysed, and indirect_alias pins it. check_dispatch_symbols never looked at llvm-objdump's status, repeating in the new checker the mistake the preprocessor path had already been fixed for: it runs on the left of a `||`, which disables errexit for its whole body, so a dump that failed after printing was read as a verdict. A complete-looking dump exiting non-zero read as acceptance; a truncated one read as a rejection, which in a reject row reads as a PASS -- every such row would pass on a machine where llvm-objdump cannot run. The status is checked and reported as INFRA_ERROR, and classify_symbols requires EXACTLY the expected status rather than merely non-zero, so a reject row is not satisfied by an analyser failure. Both halves are pinned by a stand-in objdump that prints, then exits 73, mirroring section 3. Each check has one discriminating row: relaxing the count fails asm_label, deleting the first-call branch fails after_dispatch, removing the indirect ban fails indirect_alias, and none of the three disturbs the others. --- tests/unit/dispatch_receiver_tests.sh | 159 +++++++++++++++++++++----- 1 file changed, 132 insertions(+), 27 deletions(-) diff --git a/tests/unit/dispatch_receiver_tests.sh b/tests/unit/dispatch_receiver_tests.sh index 73beb0581..369001936 100755 --- a/tests/unit/dispatch_receiver_tests.sh +++ b/tests/unit/dispatch_receiver_tests.sh @@ -12,16 +12,17 @@ # `r` and not `c`, and that the call is the first statement. check_dispatch_symbols reads the # RELOCATIONS of the emitted object: it is what can see a second call however it was spelled, # including one reaching the import through an asm label that never spells the identifier -# twice. Neither subsumes the other, so both run. +# twice. Neither subsumes the other, so both run. Where the object cannot answer either -- an +# indirect call names a type and not a target -- it is refused rather than guessed at. # # Each is exercised three ways: against the real generated dispatch, against a table of crafted # counterexamples that must each be rejected, and against positive controls that must NOT be. -# Earlier revisions were defeated seven times in review -- by handler names the checker did not +# Earlier revisions were defeated eight times in review -- by handler names the checker did not # match, by a branch on the signature line, by two calls on one line, by a comment between the # identifier and its paren, by a raw string closing at column 1, by a marker whose filename -# carried an escaped quote, and by that asm label -- because each fix pattern-matched the last -# evasion. Checking the checker is what stops that: a new evasion is one row below, not a round -# trip. +# carried an escaped quote, by an asm label, and by that same alias called through a function +# pointer -- because each fix pattern-matched the last evasion. Checking the checker is what +# stops that: a new evasion is one row below, not a round trip. # # The marker filter is pinned from BOTH sides. Too narrow and it leaves a marker in the # normalised source, which is read as apply()'s first statement and rejects a correct dispatch; @@ -30,9 +31,11 @@ # `$` -- has a row that fails when it alone is weakened, positive rows for the first sense and # counterexamples for the second. # -# The checker reports three outcomes, not two, and callers distinguish all three: accepted, -# rejected, and INFRA_ERROR -- the check could not be performed. Collapsing the third into -# either verdict is how a broken toolchain reads as a green run. +# BOTH checkers report three outcomes, not two, and their callers distinguish all three: +# accepted, rejected, and INFRA_ERROR -- the check could not be performed. Collapsing the third +# into either verdict is how a broken toolchain reads as a green run, and it is the one that +# reads as a PASS: a reject row is satisfied by any non-zero status unless the caller separates +# them. Each analyser has a stand-in that prints, then fails, to pin that. # # Usage: dispatch_receiver_tests.sh set -euo pipefail @@ -165,24 +168,55 @@ check_dispatch() { # so there is no such function to write today, but it is a real limit rather than a covered # case. # -# Echoes OK, or a reason. Returns 0 or $REJECTED. +# INDIRECT calls are refused outright rather than analysed. Relocations name the target of a +# DIRECT call; a `call_indirect` names only a type, so its target is exactly what this cannot +# see -- and the address can reach the table without a direct call ever appearing: +# +# setter_fn volatile fp = again; // R_WASM_TABLE_INDEX_SLEB, not FUNCTION_INDEX_LEB +# fp(c); // call_indirect +# +# leaves one direct setter call, first, and a second call to the same import that a scan of +# call relocations cannot count. The generated dispatch is a chain of direct calls and has no +# legitimate indirect one, so the honest answer is to reject rather than to guess. +# +# Echoes OK, or a reason. Returns 0, $REJECTED, or $INFRA_ERROR. check_dispatch_symbols() { # $1=object file - local relocs count first - relocs="$("$LLVM_OBJDUMP" -dr "$1" 2>/dev/null | awk ' + local records log status=0 calls count first + + # The analyser's own status, for the same reason the preprocessor's is checked: this runs + # on the left of a `||`, which disables errexit for the whole body, so a dump that failed + # after printing something would otherwise be read as a verdict -- a complete-looking dump + # exiting non-zero as acceptance, a truncated one as a rejection, which in a reject row + # reads as a PASS. + log="$(mktemp "${WORK}/objdump.XXXXXX")" + records="$("$LLVM_OBJDUMP" -dr "$1" 2>"$log" | awk ' /^[0-9a-f]+ <.*>:$/ { in_apply = ($0 ~ /:$/); next } - in_apply && /R_WASM_FUNCTION_INDEX_LEB/ { - sym = $NF; sub(/\+[0-9]+$/, "", sym); print sym - }')" - if [ -z "$relocs" ]; then + !in_apply { next } + /call_indirect/ { print "INDIRECT"; next } + /R_WASM_FUNCTION_INDEX_LEB/ { + sym = $NF; sub(/\+[-0-9]+$/, "", sym); print "CALL " sym + }')" || status=$? + if [ "$status" -ne 0 ]; then + echo "llvm-objdump on $(basename "$1") exited ${status}: $(tr '\n' ' ' < "$log")" + return "$INFRA_ERROR" + fi + + if printf '%s\n' "$records" | grep -qx INDIRECT; then + echo "apply() makes an indirect call, whose target this check cannot see" + return "$REJECTED" + fi + + calls="$(printf '%s\n' "$records" | sed -n 's/^CALL //p')" + if [ -z "$calls" ]; then echo "no call relocations inside apply() in $(basename "$1")" return "$REJECTED" fi - count="$(printf '%s\n' "$relocs" | grep -cx 'sysio_set_contract_name' || true)" + count="$(printf '%s\n' "$calls" | grep -cx 'sysio_set_contract_name' || true)" if [ "$count" -ne 1 ]; then echo "apply() calls sysio_set_contract_name ${count} time(s), not once" return "$REJECTED" fi - first="$(printf '%s\n' "$relocs" | head -1)" + first="$(printf '%s\n' "$calls" | head -1)" if [ "$first" != sysio_set_contract_name ]; then echo "the first call in apply() is ${first}, not sysio_set_contract_name" return "$REJECTED" @@ -408,6 +442,17 @@ mkfixture asm_label ' extern void again(uint64_t) __asm__("sysio_set_" "contract_name"); again(c); if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' +# The same alias, reached through a volatile function pointer. cdt-cpp emits ONE direct setter +# relocation, then R_WASM_TABLE_INDEX_SLEB for the address and a call_indirect -- so a scan of +# call relocations counts one call, first, and accepts, while apply() overwrites the receiver +# with the code. This is the row that pins the indirect ban. +mkfixture indirect_alias ' + sysio_set_contract_name(r); + extern void again(uint64_t) __asm__("sysio_set_" "contract_name"); + using setter_fn = void (*)(uint64_t); + setter_fn volatile fp = again; + fp(c); + if (c == r) { __sysio_action_go_x(r, c); } else { __sysio_notify_on_x(r, c); }' mkfixture marker_flags_ok ' #include "empty_header.hpp" sysio_set_contract_name(r); @@ -530,16 +575,34 @@ CDT_CPP="$real_cdt_cpp" # # Exercised the way the text checker is: against the real generated dispatch, against a # positive control, and against the counterexample the text checker cannot see. +# Does $1 meet expectation $2? Returns 0 when it does, and non-zero -- echoing why -- when it +# does not. EXACTLY the expected status, not merely non-zero: an analyser that could not run +# returns INFRA_ERROR, and counting that as the rejection a reject row expects is how a broken +# llvm-objdump sweeps this section green. The same trap the preprocessor path already avoids, +# and it is pinned below the same way. +classify_symbols() { # $1=object $2=accept|reject + local verdict status=0 want=0 + if [ "$2" = reject ]; then want="$REJECTED"; fi + verdict="$(check_dispatch_symbols "$1")" || status=$? + if [ "$status" -eq "$want" ]; then + return 0 + fi + if [ "$status" -eq "$INFRA_ERROR" ]; then + echo "no verdict was reached: ${verdict}" + else + echo "${verdict}" + fi + return 1 +} + run_symbols() { # $1=label $2=object $3=expect: accept|reject - local verdict status=0 - verdict="$(check_dispatch_symbols "$2")" || status=$? - if [ "$3" = accept ] && [ "$status" -eq 0 ]; then - pass "the symbol check accepts: $1" - elif [ "$3" = reject ] && [ "$status" -ne 0 ]; then - pass "the symbol check rejects: $1" + local reason="" ok=0 + reason="$(classify_symbols "$2" "$3")" || ok=$? + if [ "$ok" -eq 0 ]; then + pass "the symbol check ${3}s: $1" else fail "the symbol check ${3}s: $1" - echo " ${verdict}" + echo " ${reason}" fi } @@ -561,9 +624,51 @@ run_symbols "the setter after the dispatch" "${WORK}/fixture_after_dispatch.o" r # A compile failure here must not read as the rejection this row expects, so the check only # runs once the object exists. -if compile_fixture asm_label; then - run_symbols "an asm label reaching the same import" "${WORK}/fixture_asm_label.o" reject -fi +for indirect in asm_label indirect_alias; do + if compile_fixture "$indirect"; then + run_symbols "reaching the same import: ${indirect}" "${WORK}/fixture_${indirect}.o" reject + fi +done + +# ...and a failing analyser is an infrastructure error here too, not a verdict. Without that, +# every reject row above passes on a machine where llvm-objdump cannot run: a truncated dump +# reads as a rejection, which is exactly what those rows are looking for. +cat > "${WORK}/fake_objdump" <<'EOF' +#!/bin/bash +# Prints a canned payload and exits with a canned status, both read from files beside it. +cat "$(dirname "$0")/fake_od_out" +exit "$(cat "$(dirname "$0")/fake_od_status")" +EOF +chmod +x "${WORK}/fake_objdump" +printf '73\n' > "${WORK}/fake_od_status" +# A dump that WOULD be accepted, so only the status can distinguish it. +"$LLVM_OBJDUMP" -dr "${WORK}/fixture_spaced_ok.o" > "${WORK}/fake_od_out" 2>/dev/null + +real_objdump="$LLVM_OBJDUMP" +LLVM_OBJDUMP="${WORK}/fake_objdump" +for shape in acceptable_output truncated_output; do + [ "$shape" = truncated_output ] && : > "${WORK}/fake_od_out" + + sym_verdict=""; sym_status=0 + sym_verdict="$(check_dispatch_symbols "${WORK}/fixture_spaced_ok.o")" || sym_status=$? + if [ "$sym_status" -eq "$INFRA_ERROR" ]; then + pass "a failing analyser reaches no verdict: ${shape}" + else + fail "a failing analyser reaches no verdict: ${shape}" + echo " status ${sym_status}: ${sym_verdict}" + fi + + # ...and a reject row must not be satisfied by it, which is the half a status alone does + # not buy. + sym_reason=""; sym_ok=0 + sym_reason="$(classify_symbols "${WORK}/fixture_spaced_ok.o" reject)" || sym_ok=$? + if [ "$sym_ok" -ne 0 ]; then + pass "a reject row is not satisfied by an analyser failure: ${shape}" + else + fail "a reject row is not satisfied by an analyser failure: ${shape}" + fi +done +LLVM_OBJDUMP="$real_objdump" # ...and the text checker really does miss that one, which is why both run. Reported rather # than asserted: a future text checker strong enough to catch it should not fail this suite. From c63edeeca296ecc51fe983e6b54342e40b4c4267 Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Fri, 4 Sep 2026 10:06:48 -0500 Subject: [PATCH 28/28] test(kv): match call_indirect as an opcode, not as text on the line The indirect-call detector searched every line inside apply() for the string `call_indirect`, and it ran before the relocation branch. A contract may legally declare an action named `call_indirect`; its generated wrapper is `__sysio_action_call_indirect_dispatchrcv`, and the direct relocation to it contains the substring. There is no indirect-call instruction anywhere in that dispatch, but the checker reported one and rejected a correct dispatch. Lines are now classified by shape before content. llvm-objdump indents a relocation record with tabs and an instruction with spaces, so the relocation branch takes the tab-led lines and the opcode is compared as the mnemonic FIELD of what remains -- never as text anywhere on the line. The contract in section 1 gains exactly that action, so the positive control on the real generated dispatch covers it. Reverting to the substring match fails that row, 35/36; removing the indirect ban still fails indirect_alias, so the two remain independently pinned. --- tests/unit/dispatch_receiver_tests.sh | 31 ++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/tests/unit/dispatch_receiver_tests.sh b/tests/unit/dispatch_receiver_tests.sh index 369001936..5c8076524 100755 --- a/tests/unit/dispatch_receiver_tests.sh +++ b/tests/unit/dispatch_receiver_tests.sh @@ -189,12 +189,33 @@ check_dispatch_symbols() { # $1=object file # exiting non-zero as acceptance, a truncated one as a rejection, which in a reject row # reads as a PASS. log="$(mktemp "${WORK}/objdump.XXXXXX")" + # + # Lines are classified by SHAPE first. llvm-objdump indents a relocation record with tabs + # and an instruction with spaces: + # + # " 18: 10 80 ... \tcall\t0" <- instruction + # "\t\t\t00000019: R_WASM_FUNCTION_INDEX_LEB\tsym+0" <- relocation + # + # and the opcode is compared as the mnemonic FIELD, never as text anywhere on the line. + # Searching the whole line for `call_indirect` reads a relocation to a legal action named + # `call_indirect` -- `__sysio_action_call_indirect_dispatchrcv`, which a contract may + # declare -- as an indirect call, and rejects a correct dispatch. The generated contract in + # section 1 declares exactly that action, so the positive control covers it. records="$("$LLVM_OBJDUMP" -dr "$1" 2>"$log" | awk ' /^[0-9a-f]+ <.*>:$/ { in_apply = ($0 ~ /:$/); next } !in_apply { next } - /call_indirect/ { print "INDIRECT"; next } - /R_WASM_FUNCTION_INDEX_LEB/ { - sym = $NF; sub(/\+[-0-9]+$/, "", sym); print "CALL " sym + /^\t/ { + if ($0 ~ /R_WASM_FUNCTION_INDEX_LEB/) { + sym = $NF; sub(/\+[-0-9]+$/, "", sym); print "CALL " sym + } + next + } + { + if (split($0, field, "\t") >= 2) { + mnemonic = field[2] + gsub(/^[ \t]+|[ \t]+$/, "", mnemonic) + if (mnemonic == "call_indirect") print "INDIRECT" + } }')" || status=$? if [ "$status" -ne 0 ]; then echo "llvm-objdump on $(basename "$1") exited ${status}: $(tr '\n' ' ' < "$log")" @@ -245,6 +266,10 @@ class [[sysio::contract("dispatchrcv")]] dispatchrcv : public sysio::contract { public: using contract::contract; [[sysio::action]] void go() {} + // A legal action whose generated wrapper is __sysio_action_call_indirect_dispatchrcv. The + // symbol check must read the OPCODE field, not the line, or this relocation reads as an + // indirect call and a correct dispatch is rejected. + [[sysio::action("callindirect")]] void call_indirect() {} [[sysio::on_notify("sysio.token::transfer")]] void onxfer(sysio::name from, sysio::name to) {} }; EOF