fix: enforce regmap size invariant to close OOB writes (#64, #65)

.github/workflows/ci.yml

@@ -153,6 +153,45 @@ jobs:
           find include tests -name '*.hpp' -o -name '*.cpp' | \
             xargs clang-tidy -p build --warnings-as-errors='*' || true
 
+  # ── ASan/UBSan (regmap) ───────────────────────────────────────────────────────
+  # Scoped to test_regmap: rcp/regmap.hpp's Ep0 does raw std::vector<T>
+  # indexing across per-endpoint access-control checks and the config
+  # vectors those checks gate (cpp-RCP-N2-01/N2-02, issues #64/#65), so this
+  # is the header most worth sanitizer coverage on a fast, gating job. This
+  # is additive to build-and-test above, not a replacement for it.
+  asan-ubsan-regmap:
+    name: ASan/UBSan (regmap)
+    runs-on: ubuntu-22.04
+    needs: build-and-test
+    steps:
+      - uses: actions/checkout@v4
+
+      - name: Install tools
+        run: |
+          sudo apt-get update -qq
+          sudo apt-get install -y cmake ninja-build
+
+      - name: Configure (ASan + UBSan)
+        env:
+          CC: clang-14
+          CXX: clang++-14
+        run: |
+          cmake -B build-asan \
+            -DCMAKE_BUILD_TYPE=Debug \
+            -DCMAKE_CXX_STANDARD=17 \
+            -DCMAKE_CXX_FLAGS="-fsanitize=address,undefined -fno-omit-frame-pointer -g" \
+            -DCMAKE_EXE_LINKER_FLAGS="-fsanitize=address,undefined" \
+            -G Ninja
+
+      - name: Build test_regmap
+        run: cmake --build build-asan --target test_regmap --parallel
+
+      - name: Run test_regmap under ASan/UBSan
+        env:
+          ASAN_OPTIONS: halt_on_error=1
+          UBSAN_OPTIONS: halt_on_error=1,print_stacktrace=1
+        run: ./build-asan/tests/test_regmap
+
   # ── LCOV coverage ─────────────────────────────────────────────────────────────
   coverage:
     name: Coverage (LCOV)

include/rcp/regmap.hpp

@@ -329,8 +329,33 @@ class Ep0 {
     // ServerLifecycle::generic_config_locked/functional_config_locked).
     enum class ConfigBlock { Generic, Functional };
 
+    // endpoint_owner_ is the vector every access-control check (below)
+    // bounds a target endpoint against, but the writes those checks gate
+    // land in regs.generic_configs/regs.functional_configs instead — two
+    // vectors this class does not own and cannot force the caller to keep
+    // in sync with endpoint_count. If they are not the same length as
+    // endpoint_count when this object is constructed, size endpoint_owner_
+    // to 0 rather than to the untrustworthy endpoint_count: every non-EP0
+    // access check then fails closed with invalid_parameter instead of
+    // handing out an in-range verdict that a subsequent write turns into an
+    // out-of-bounds write on the shorter/empty config vector (cpp-RCP-N2-01,
+    // issue #64). write_generic_config/write_functional_config additionally
+    // bounds-check against the actual vector being indexed as a second,
+    // independent layer of defense (see below).
     Ep0(RegisterMap& regs, lifecycle::ServerLifecycle& lc) noexcept
-        : regs_(regs), lifecycle_(lc), endpoint_owner_(regs.endpoint_count) {}
+        : regs_(regs), lifecycle_(lc),
+          endpoint_owner_(size_invariant_holds(regs) ? regs.endpoint_count : 0) {}
+
+    // is_endpoint_table_consistent reports whether the size invariant this
+    // class's access checks rely on currently holds (regs_.generic_configs
+    // .size() == regs_.functional_configs.size() == regs_.endpoint_count).
+    // It is false only when the RegisterMap this Ep0 was constructed with
+    // violated the invariant and per-endpoint access has therefore been
+    // fail-closed (endpoint_owner_ sized to 0) until a valid replacement
+    // map is installed via write_whole_map.
+    bool is_endpoint_table_consistent() const noexcept {
+        return size_invariant_holds(regs_) && endpoint_owner_.size() == regs_.endpoint_count;
+    }
 
     // claim_root_client implements the exclusive root-client rule: once a
     // stream holds the root-client slot, a different stream's claim is
@@ -402,23 +427,47 @@ class Ep0 {
     // call this.
     const RegisterMap& read_whole_map() const noexcept { return regs_; }
 
-    // write_whole_map is the root-client-only half of the same rule.
+    // write_whole_map is the root-client-only half of the same rule. Beyond
+    // the root-client check, it must also keep the size invariant every
+    // access check in this class depends on intact (cpp-RCP-N2-02, issue
+    // #65): a root client could otherwise install a replacement map whose
+    // generic_configs/functional_configs disagree with its own
+    // endpoint_count, or that simply differs in endpoint_count from the map
+    // endpoint_owner_ was last sized against, either of which lets a
+    // subsequent per-endpoint write pass a stale/incorrect bounds check and
+    // land out of bounds on the new vectors. So: reject an internally
+    // inconsistent replacement map outright, and resize endpoint_owner_ to
+    // match the (now validated) new endpoint_count as part of the same
+    // operation, discarding prior per-endpoint owner assignments — they
+    // describe endpoints in the map being replaced, not necessarily the
+    // same endpoints in the new one.
     std::error_code write_whole_map(size_t client, RegisterMap new_map) noexcept {
         if (!is_root_client(client))
             return make_error_code(RegMapErrc::unauthorized_access);
+        if (!size_invariant_holds(new_map))
+            return make_error_code(RegMapErrc::invalid_parameter);
         regs_ = std::move(new_map);
+        endpoint_owner_.assign(regs_.endpoint_count, std::nullopt);
         return {};
     }
 
     // write_generic_config / write_functional_config apply the per-endpoint
     // write-access and per-block lock checks above before mutating the
-    // targeted endpoint's config block in the underlying RegisterMap.
+    // targeted endpoint's config block in the underlying RegisterMap. The
+    // explicit size check against the vector actually being indexed (rather
+    // than trusting check_write_access's endpoint_owner_-based verdict
+    // alone) is a second, independent layer of defense against cpp-RCP-N2-01
+    // (issue #64): even if endpoint_owner_ and the config vectors were ever
+    // to drift apart again, these writes still cannot go out of bounds.
     std::error_code write_generic_config(size_t client, EndpointId target,
                                           EndpointGenericConfig cfg) noexcept {
         if (target == kEp0) return make_error_code(RegMapErrc::invalid_parameter);
         auto ec = check_write_access(client, target, ConfigBlock::Generic);
         if (ec) return ec;
-        regs_.generic_configs[static_cast<size_t>(target - 1)] = std::move(cfg);
+        const auto idx = static_cast<size_t>(target - 1);
+        if (idx >= regs_.generic_configs.size())
+            return make_error_code(RegMapErrc::invalid_parameter);
+        regs_.generic_configs[idx] = std::move(cfg);
         return {};
     }
 
@@ -427,11 +476,25 @@ class Ep0 {
         if (target == kEp0) return make_error_code(RegMapErrc::invalid_parameter);
         auto ec = check_write_access(client, target, ConfigBlock::Functional);
         if (ec) return ec;
-        regs_.functional_configs[static_cast<size_t>(target - 1)] = std::move(cfg);
+        const auto idx = static_cast<size_t>(target - 1);
+        if (idx >= regs_.functional_configs.size())
+            return make_error_code(RegMapErrc::invalid_parameter);
+        regs_.functional_configs[idx] = std::move(cfg);
         return {};
     }
 
 private:
+    // size_invariant_holds is the shared root-invariant check both the
+    // constructor and write_whole_map enforce: every access-control check
+    // above bounds a target endpoint against endpoint_owner_ (sized from
+    // endpoint_count), but the writes those checks gate land in
+    // regs.generic_configs/regs.functional_configs — this must hold for
+    // that to be safe (cpp-RCP-N2-01/N2-02, issues #64/#65).
+    static bool size_invariant_holds(const RegisterMap& regs) noexcept {
+        return regs.generic_configs.size() == regs.endpoint_count &&
+               regs.functional_configs.size() == regs.endpoint_count;
+    }
+
     RegisterMap&                       regs_;
     lifecycle::ServerLifecycle&        lifecycle_;
     std::optional<size_t>              root_client_;

tests/test_regmap.cpp

@@ -349,6 +349,138 @@ TEST_CASE("Assigning an owner to EP0 itself is rejected as INVALID_PARAMETER", "
     REQUIRE(ec == make_error_code(RegMapErrc::invalid_parameter));
 }
 
+// ── Size-invariant enforcement (cpp-RCP-N2-01 / cpp-RCP-N2-02, issues #64/#65) ──
+// Ep0's access checks (check_read_access/check_write_access/set_endpoint_owner)
+// all bound a target endpoint against endpoint_owner_, which is sized from
+// endpoint_count. The actual writes land in regs_.generic_configs /
+// regs_.functional_configs instead. These tests confirm a RegisterMap whose
+// endpoint_count disagrees with those vectors' lengths is rejected/fails
+// closed rather than letting an in-range access check be followed by an
+// out-of-bounds write.
+
+TEST_CASE("A RegisterMap whose config vectors disagree with endpoint_count fails closed instead of allowing OOB access",
+          "[regmap][REQ-REGMAP-013]") {
+    RegisterMap m;
+    m.endpoint_count = 4;
+    // generic_configs / functional_configs deliberately left empty/default
+    // -- exactly the mismatch issue #64 describes.
+    ServerLifecycle lc;
+    Ep0 ep0(m, lc);
+
+    REQUIRE_FALSE(ep0.is_endpoint_table_consistent());
+
+    REQUIRE_FALSE(ep0.claim_root_client(1));
+
+    // Every non-EP0 access check must fail closed (INVALID_PARAMETER), not
+    // report a target in [1, endpoint_count] as valid.
+    auto ec_owner = ep0.set_endpoint_owner(1, 1);
+    REQUIRE(ec_owner);
+    REQUIRE(ec_owner == make_error_code(RegMapErrc::invalid_parameter));
+
+    auto ec_read = ep0.check_read_access(1);
+    REQUIRE(ec_read);
+    REQUIRE(ec_read == make_error_code(RegMapErrc::invalid_parameter));
+
+    EndpointGenericConfig gcfg;
+    auto ec_gwrite = ep0.write_generic_config(1, /*target=*/1, gcfg);
+    REQUIRE(ec_gwrite);
+    REQUIRE(ec_gwrite == make_error_code(RegMapErrc::invalid_parameter));
+
+    EndpointFunctionalConfig fcfg;
+    auto ec_fwrite = ep0.write_functional_config(1, /*target=*/1, fcfg);
+    REQUIRE(ec_fwrite);
+    REQUIRE(ec_fwrite == make_error_code(RegMapErrc::invalid_parameter));
+
+    // EP0 (whole-map) read/write are unaffected -- the invariant only
+    // gates per-endpoint access.
+    REQUIRE_FALSE(ep0.check_read_access(kEp0));
+}
+
+TEST_CASE("A RegisterMap with a smaller mismatch (nonzero but undersized config vectors) also fails closed",
+          "[regmap][REQ-REGMAP-013]") {
+    RegisterMap m;
+    m.endpoint_count = 4;
+    m.generic_configs.resize(1);
+    m.functional_configs.resize(1);
+    ServerLifecycle lc;
+    Ep0 ep0(m, lc);
+
+    REQUIRE_FALSE(ep0.is_endpoint_table_consistent());
+    REQUIRE_FALSE(ep0.claim_root_client(1));
+
+    EndpointGenericConfig cfg;
+    // target=4 would be in-range against endpoint_count (and against the
+    // pre-fix endpoint_owner_.size()==4), but generic_configs only has 1
+    // slot -- this must not be allowed through.
+    auto ec = ep0.write_generic_config(1, /*target=*/4, cfg);
+    REQUIRE(ec);
+    REQUIRE(ec == make_error_code(RegMapErrc::invalid_parameter));
+}
+
+TEST_CASE("write_whole_map rejects a replacement map whose config vectors disagree with its own endpoint_count",
+          "[regmap][REQ-REGMAP-013]") {
+    auto map = make_map(2);
+    ServerLifecycle lc;
+    Ep0 ep0(map, lc);
+    REQUIRE_FALSE(ep0.claim_root_client(1));
+
+    RegisterMap bad;
+    bad.endpoint_count = 3;
+    bad.generic_configs.resize(1);      // mismatched
+    bad.functional_configs.resize(3);
+
+    auto ec = ep0.write_whole_map(1, bad);
+    REQUIRE(ec);
+    REQUIRE(ec == make_error_code(RegMapErrc::invalid_parameter));
+
+    // Rejected write must leave the live map untouched.
+    REQUIRE(ep0.read_whole_map().endpoint_count == 2);
+    REQUIRE(ep0.is_endpoint_table_consistent());
+}
+
+TEST_CASE("write_whole_map with a valid replacement map resizes endpoint_owner_ to match the new endpoint_count",
+          "[regmap][REQ-REGMAP-005]") {
+    auto map = make_map(2);
+    ServerLifecycle lc;
+    Ep0 ep0(map, lc);
+    REQUIRE_FALSE(ep0.set_endpoint_owner(1, 10));
+    REQUIRE_FALSE(ep0.set_endpoint_owner(2, 20));
+    REQUIRE_FALSE(ep0.claim_root_client(1));
+
+    // Grow: install a 4-endpoint replacement. Before the fix, endpoint_owner_
+    // stayed sized at 2, so endpoint 4 could never get an owner (silent
+    // DoS) even though write_generic_config's own bounds check would have
+    // been the thing standing between a client and an OOB write.
+    auto bigger = make_map(4);
+    REQUIRE_FALSE(ep0.write_whole_map(1, bigger));
+    REQUIRE(ep0.is_endpoint_table_consistent());
+
+    // Prior per-endpoint owner assignments are discarded by the whole-map
+    // replacement -- they described endpoints in the old map, not
+    // necessarily the same endpoints in the new one.
+    EndpointGenericConfig cfg;
+    auto ec_stale_owner = ep0.write_generic_config(/*client=*/10, /*target=*/1, cfg);
+    REQUIRE(ec_stale_owner);
+    REQUIRE(ec_stale_owner == make_error_code(RegMapErrc::unauthorized_access));
+
+    // The root client can write any endpoint in the new, larger range,
+    // including endpoint 4, which did not exist under the old map.
+    REQUIRE_FALSE(ep0.write_generic_config(1, /*target=*/4, cfg));
+    REQUIRE(ep0.read_whole_map().generic_configs[3].request_queue_size == cfg.request_queue_size);
+
+    // Shrink: install a 1-endpoint replacement. endpoint 4 must no longer
+    // be accepted as a write target -- this is exactly the stale-bounds
+    // scenario issue #65 describes (endpoint_owner_ too large for the new,
+    // shorter config vectors) if endpoint_owner_ were not resized down too.
+    auto smaller = make_map(1);
+    REQUIRE_FALSE(ep0.write_whole_map(1, smaller));
+    REQUIRE(ep0.is_endpoint_table_consistent());
+
+    auto ec_oob = ep0.write_generic_config(1, /*target=*/4, cfg);
+    REQUIRE(ec_oob);
+    REQUIRE(ec_oob == make_error_code(RegMapErrc::invalid_parameter));
+}
+
 // ── The four mandatory error codes are distinct ─────────────────────────────────
 
 TEST_CASE("The four mandatory register-map error codes are distinct values in their own category",