From c90a360498b8b89168e2ef378db92d5677d7727d Mon Sep 17 00:00:00 2001 From: Matt <47545907+SoundMatt@users.noreply.github.com> Date: Sat, 22 Aug 2026 16:23:46 -0700 Subject: [PATCH] fix(transport): widen pending_key() to preserve byte_bus_id's full 11 bits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit rcp::udp::Client::pending_key() and rcp::l2::Client::pending_key() computed their response-correlation map key by shifting byte_bus_id left 8 bits and OR-ing in transaction_num, then truncating the result to uint16_t. But byte_bus_id is an 11-bit wire field (0-2047, avtp::ByteBusId's own comment; acf.hpp's detail::kByteBusIdMask), not 8 bits, so the truncation silently dropped its top 3 bits: pending_key(5, 7) and pending_key(261, 7) both evaluated to 0x0507, even though 5 and 261 are two different, wire-legal byte_bus_id values. Under concurrent requests sharing a Client instance (multiple threads calling Client::request() against different byte_bus_id endpoints, exactly what REQ-UDP-011's own text describes: "two outstanding requests with distinct (byte_bus_id, transaction_num) pairs each receive their own response"), two colliding keys land in the same pending_ map slot. The second request's map insertion silently overwrites the first's promise pointer, so: - the first request's response is delivered to the SECOND request's promise (misdelivery — the second caller receives the wrong request's data), and - the first request's own promise, no longer reachable from pending_, never gets set — that Client::request() call hangs until ctx's deadline (or forever with no deadline). Fix: widen pending_key()'s return type to uint32_t and shift bus_id left by the same 8 bits, but without truncating the result back down — bus_id (0-2047, 11 bits) now occupies bits 8-18 of the key and transaction_num (0-255, 8 bits) occupies bits 0-7, so no two distinct (byte_bus_id, transaction_num) pairs can ever alias to the same key. Client::pending_'s map key type is updated to uint32_t to match, in both udp.hpp and l2.hpp. Also moved pending_key out of each Client class (where it was a private static method) to a top-level pure function in each file — rcp/udp.hpp's own encode_annexj_datagram/decode_annexj_datagram and rcp/l2.hpp's own is_unicast_mac already establish this file's convention for pure, socket-free helpers: unconditional of any platform guard, so the function itself (and a test exercising it directly) compiles and runs on every platform. This also makes the fix independently unit-testable without a live socket, which rcp/l2.hpp's Client needs (AF_PACKET/CAP_NET_RAW, Linux-only) but a plain function does not. Tests added: - tests/test_udp.cpp: a pure pending_key() unit test proving pending_key(5, 7) != pending_key(261, 7) and sweeping every byte_bus_id 256 apart across the full 11-bit range for collisions (tagged REQ-UDP-011, the existing requirement whose own text this bug violated); and a new two-thread, real-loopback-socket regression test ("Client::request does not misdeliver or hang for two genuinely concurrent requests...") that drives a genuine overlap between a byte_bus_id=5 request (server-side delayed 150ms) and a byte_bus_id=261 request sharing transaction_num 7, and asserts each gets its own correct response. - tests/test_l2.cpp: the same pure pending_key() unit test (no REQ tag — no existing cpp-RCP L2 requirement covers Client-level response correlation by key the way REQ-UDP-011 does for udp.hpp; a new REQ-L2-011 entry in .fusa-reqs.json may be warranted but was not added here per this change's own scope). A live-socket equivalent of the threaded udp.hpp test was not added to tests/l2_veth_roundtrip.cpp since that harness needs CAP_NET_RAW/root and only runs in the dedicated l2-veth CI job, not the privilege-free ctest suite this fix was verified against. Verification: - Full clean rebuild (cmake -DRCP_BUILD_TESTS=ON -DCMAKE_BUILD_TYPE=Debug && cmake --build build -j): 0 errors, 0 warnings. - ctest: 100% pass, 58/58 test binaries, including both new pending_key regression tests. - Mutation-testing sanity check: reintroduced the original uint16_t truncation arithmetic (keeping the new call sites/map type so it still compiled), rebuilt, and confirmed the new tests fail against it — pending_key(5,7) == pending_key(261,7) (both 0x0507), and the concurrent-request regression test's byte_bus_id=5 leg times out (ErrTimeout) exactly matching the "indefinite hang" failure mode this finding describes. Reverted the mutation and reran the full suite to confirm 100% pass again. Closes a HIGH-severity finding from the cpp-RCP v3.0.0 deep audit (batch 1, transport pending_key collision). Co-Authored-By: Claude Sonnet 5 Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com> --- include/rcp/l2.hpp | 35 +++++++++++++---- include/rcp/udp.hpp | 31 +++++++++++---- tests/test_l2.cpp | 41 ++++++++++++++++++++ tests/test_udp.cpp | 91 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 184 insertions(+), 14 deletions(-) diff --git a/include/rcp/l2.hpp b/include/rcp/l2.hpp index 9c6fe0c..142b4be 100644 --- a/include/rcp/l2.hpp +++ b/include/rcp/l2.hpp @@ -208,6 +208,31 @@ inline bool is_unicast_mac(const MacAddress& mac) noexcept { return (mac[0] & 0x01u) == 0u; } +// pending_key combines byte_bus_id (an 11-bit wire field — avtp::ByteBusId's +// own comment, and acf.hpp's detail::kByteBusIdMask) and transaction_num (a +// full 8-bit field) into one collision-free correlation key for Client's own +// pending_ member below (RCP_L2_LINUX branch only — Client is a stub on +// every other platform). Same formula and same fix as rcp/udp.hpp's own +// top-level pending_key (that file's own comment explains the collision +// this widening fixes). Placed here, unconditional of RCP_L2_LINUX like +// is_unicast_mac just above, rather than in the RCP_L2_LINUX-only detail +// namespace below, so this pure function — and the test exercising it +// directly — compiles and runs on every platform, not only Linux. +// +// Shifting bus_id left by 8 keeps every (byte_bus_id, transaction_num) pair +// distinct: transaction_num occupies bits 0-7 and byte_bus_id (0-2047, 11 +// bits) occupies bits 8-18 of the returned uint32_t, so the two fields never +// overlap. A previous version of this function returned uint16_t, computing +// the identical shift but then truncating the result back down to 16 bits — +// silently dropping byte_bus_id's top 3 bits, so e.g. byte_bus_id 5 and 261 +// (differing by exactly 256) collided whenever they shared a +// transaction_num, misdelivering one Client's pending response to another +// (or hanging it indefinitely) under concurrent requests. cpp-RCP v3.0.0 +// deep audit finding; fixed by widening this key to uint32_t. +inline uint32_t pending_key(avtp::ByteBusId bus_id, uint8_t transaction_num) noexcept { + return (static_cast(bus_id) << 8) | static_cast(transaction_num); +} + // ── Frame ───────────────────────────────────────────────────────────────────── // Same AVTPDU shape as rcp/udp.hpp::Frame (one NTSCF/TSCF header wrapping // one ACF_ABB/ACF_GBB message) — deliberately duplicated here rather than @@ -929,7 +954,7 @@ class Client { out.message_timestamp = message_timestamp; out.payload = req_payload; - const uint16_t key = pending_key(req.byte_bus_id, req.transaction_num); + const uint32_t key = pending_key(req.byte_bus_id, req.transaction_num); auto result = std::make_shared>(); auto future = result->get_future(); { @@ -984,10 +1009,6 @@ class Client { bool ok() const noexcept { return fd_ >= 0; } private: - static uint16_t pending_key(avtp::ByteBusId bus_id, uint8_t transaction_num) noexcept { - return static_cast((static_cast(bus_id) << 8) | transaction_num); - } - avtp::StreamId stream_id_; MacAddress dest_mac_{}; MacAddress local_mac_{}; @@ -996,7 +1017,7 @@ class Client { std::atomic closed_{false}; std::atomic seq_{0}; std::mutex mu_; - std::map>> pending_; + std::map>> pending_; std::thread read_thread_; void read_loop() { @@ -1021,7 +1042,7 @@ class Client { Frame resp; if (decode_l2_frame(buf.data(), static_cast(n), hdr, resp)) continue; - const uint16_t key = pending_key(resp.info.byte_bus_id, resp.info.transaction_num); + const uint32_t key = pending_key(resp.info.byte_bus_id, resp.info.transaction_num); std::lock_guard lk(mu_); auto it = pending_.find(key); if (it != pending_.end()) { diff --git a/include/rcp/udp.hpp b/include/rcp/udp.hpp index 84b695f..6b9dfb6 100644 --- a/include/rcp/udp.hpp +++ b/include/rcp/udp.hpp @@ -185,6 +185,27 @@ struct FrameResponse { std::vector payload; }; +// pending_key combines byte_bus_id (an 11-bit wire field — avtp::ByteBusId's +// own comment, and acf.hpp's detail::kByteBusIdMask) and transaction_num (a +// full 8-bit field) into one collision-free correlation key for +// Client::pending_ below. A pure function, independent of RCP_UDP_POSIX like +// encode_annexj_datagram/decode_annexj_datagram above, so it — and the test +// exercising it directly — compiles and runs on every platform. +// +// Shifting bus_id left by 8 keeps every (byte_bus_id, transaction_num) pair +// distinct: transaction_num occupies bits 0-7 and byte_bus_id (0-2047, 11 +// bits) occupies bits 8-18 of the returned uint32_t, so the two fields never +// overlap. A previous version of this function returned uint16_t, computing +// the identical shift but then truncating the result back down to 16 bits — +// silently dropping byte_bus_id's top 3 bits, so e.g. byte_bus_id 5 and 261 +// (differing by exactly 256) collided whenever they shared a +// transaction_num, misdelivering one Client's pending response to another +// (or hanging it indefinitely) under concurrent requests. cpp-RCP v3.0.0 +// deep audit finding; fixed by widening this key to uint32_t. +inline uint32_t pending_key(avtp::ByteBusId bus_id, uint8_t transaction_num) noexcept { + return (static_cast(bus_id) << 8) | static_cast(transaction_num); +} + #if defined(RCP_UDP_POSIX) // ── Frame ───────────────────────────────────────────────────────────────────── @@ -688,7 +709,7 @@ class Client { out.message_timestamp = message_timestamp; out.payload = req_payload; - const uint16_t key = pending_key(req.byte_bus_id, req.transaction_num); + const uint32_t key = pending_key(req.byte_bus_id, req.transaction_num); auto result = std::make_shared>(); auto future = result->get_future(); { @@ -746,10 +767,6 @@ class Client { uint32_t last_recv_encap_seq() const noexcept { return last_recv_encap_seq_.load(); } private: - static uint16_t pending_key(avtp::ByteBusId bus_id, uint8_t transaction_num) noexcept { - return static_cast((static_cast(bus_id) << 8) | transaction_num); - } - avtp::StreamId stream_id_; int fd_; std::atomic closed_{false}; @@ -757,7 +774,7 @@ class Client { std::atomic encap_seq_{0}; // Annex J encapsulation sequence number, outgoing std::atomic last_recv_encap_seq_{0}; // most recent one seen on an inbound datagram std::mutex mu_; - std::map>> pending_; + std::map>> pending_; std::thread read_thread_; void read_loop() { @@ -779,7 +796,7 @@ class Client { Frame resp; if (decode_frame(avtpdu, avtpdu_len, resp)) continue; - const uint16_t key = pending_key(resp.info.byte_bus_id, resp.info.transaction_num); + const uint32_t key = pending_key(resp.info.byte_bus_id, resp.info.transaction_num); std::lock_guard lk(mu_); auto it = pending_.find(key); if (it != pending_.end()) { diff --git a/tests/test_l2.cpp b/tests/test_l2.cpp index ee36ba0..c9c602b 100644 --- a/tests/test_l2.cpp +++ b/tests/test_l2.cpp @@ -37,6 +37,8 @@ #include #include +#include + using namespace rcp; using namespace rcp::l2; @@ -255,6 +257,45 @@ TEST_CASE("is_unicast_mac reports false for the all-ones broadcast address", "[l REQUIRE_FALSE(is_unicast_mac(mac)); } +// ── pending_key — Client response-correlation key (pure, no socket) ───────── +// +// rcp::l2::Client::request()/read_loop() correlate an inbound response to +// its outstanding request via pending_key(byte_bus_id, transaction_num), +// the same formula rcp/udp.hpp's own top-level pending_key uses (that +// file's own comment documents the collision this widening fixes) — no +// real AF_PACKET socket needed to exercise the pure key-computation logic +// itself (it is unconditional of RCP_L2_LINUX, like is_unicast_mac just +// above), so this stays in this privilege-free file rather than +// tests/l2_veth_roundtrip.cpp (this file's own header comment). +TEST_CASE("pending_key does not collide for byte_bus_id values that differ by a " + "multiple of 256 and share a transaction_num", + "[l2]") { + // byte_bus_id is an 11-bit wire field (0-2047; avtp::ByteBusId's own + // comment, acf.hpp's detail::kByteBusIdMask) -- 5 and 261 are both + // wire-legal and differ by exactly 256. A previous version of + // pending_key returned uint16_t, computing this same left-shift-by-8 + // but then truncating the result back down to 16 bits, so these two + // collided into the identical map key (0x0507) whenever they shared a + // transaction_num (cpp-RCP v3.0.0 deep audit finding) — silently + // misdelivering one Client::request() caller's response to another's + // promise, or hanging one of them indefinitely. + REQUIRE(pending_key(5, 7) != pending_key(261, 7)); + REQUIRE(pending_key(5, 7) == pending_key(5, 7)); + + // Sweep every byte_bus_id that used to alias to the same 16-bit key + // under the old truncation (i.e. every value 256 apart, across the + // whole 11-bit range) crossed with a few transaction_num values, and + // confirm every (byte_bus_id, transaction_num) pair now maps to a + // distinct key. + std::set seen; + for (uint32_t bus = 0; bus <= 2047; bus += 256) { + for (uint32_t txn = 0; txn <= 255; txn += 85) { + auto key = pending_key(static_cast(bus), static_cast(txn)); + REQUIRE(seen.insert(key).second); // must be a fresh key, never seen before + } + } +} + // ── AVTP envelope-only decode (decode_avtp_frame_header/decode_l2_frame_header) ── TEST_CASE("decode_avtp_frame_header decodes an NTSCF envelope and reports the raw ACF offset", diff --git a/tests/test_udp.cpp b/tests/test_udp.cpp index 66e8257..d2c70f5 100644 --- a/tests/test_udp.cpp +++ b/tests/test_udp.cpp @@ -37,6 +37,8 @@ #include #include +#include +#include using namespace rcp; using namespace rcp::udp; @@ -310,6 +312,33 @@ TEST_CASE("encode_annexj_datagram produces a monotonically increasing wire prefi } } +TEST_CASE("pending_key does not collide for byte_bus_id values that differ by a multiple of " + "256 and share a transaction_num", + "[udp][REQ-UDP-011]") { + // byte_bus_id is an 11-bit wire field (0-2047; avtp::ByteBusId's own + // comment, acf.hpp's detail::kByteBusIdMask) -- 5 and 261 are both + // wire-legal and differ by exactly 256. A previous version of + // pending_key returned uint16_t, computing this same left-shift-by-8 + // but then truncating the result back down to 16 bits, so these two + // collided into the identical map key (0x0507) whenever they shared a + // transaction_num (cpp-RCP v3.0.0 deep audit finding). + REQUIRE(pending_key(5, 7) != pending_key(261, 7)); + REQUIRE(pending_key(5, 7) == pending_key(5, 7)); + + // Sweep every byte_bus_id that used to alias to the same 16-bit key + // under the old truncation (i.e. every value 256 apart, across the + // whole 11-bit range) crossed with a few transaction_num values, and + // confirm every (byte_bus_id, transaction_num) pair now maps to a + // distinct key. + std::set seen; + for (uint32_t bus = 0; bus <= 2047; bus += 256) { + for (uint32_t txn = 0; txn <= 255; txn += 85) { + auto key = pending_key(static_cast(bus), static_cast(txn)); + REQUIRE(seen.insert(key).second); // must be a fresh key, never seen before + } + } +} + // ── MultiFrame — multiple ACF requests in one AVTPDU (cpp-RCP-04-fresh) ────── TEST_CASE("MultiFrame round-trips two ACF_ABB messages packed into one AVTPDU", @@ -563,6 +592,68 @@ TEST_CASE("Client::request correlates concurrent requests by byte_bus_id/transac client.close(); } +TEST_CASE("Client::request does not misdeliver or hang for two genuinely concurrent requests " + "whose byte_bus_id values differ by a multiple of 256 and share a transaction_num " + "(pending_key collision regression)", + "[udp][REQ-UDP-011]") { + Server server(make_stream_id(0x02, 6), "127.0.0.1", 0); + REQUIRE(server.ok()); + + // byte_bus_id 5's response is deliberately delayed so its request is + // still genuinely pending (inserted into Client::pending_, not yet + // erased) when byte_bus_id 261's request -- sharing the same + // transaction_num, and differing from 5 by exactly 256 -- gets inserted + // into that very same map from a second thread. Under the old uint16_t + // pending_key both requests computed the identical map key, so the + // second insertion silently clobbered the first's map entry; whichever + // response arrived first then got delivered to the WRONG promise (or, + // depending on arrival order, byte_bus_id 5's request never got a + // response delivered to it at all and hung until ctx's timeout). + server.set_handler(make_echo_handler([](const acf::AcfMessageInfo& req, const std::vector&) { + if (req.byte_bus_id == 5) std::this_thread::sleep_for(std::chrono::milliseconds(150)); + FrameResponse r; + r.info = acf::make_response(req, acf::ResponseKind::ReadResponse); + // Echo byte_bus_id back (big-endian u16) so this test can prove + // each request's OWN response actually came back, not merely that + // *a* response arrived — a misdelivered response under the bug + // still completes the waiting future, just with the wrong + // request's data. + r.payload = {static_cast(req.byte_bus_id >> 8), + static_cast(req.byte_bus_id)}; + return r; + })); + + Client client(make_stream_id(0x03, 6), "127.0.0.1", server.port()); + REQUIRE(client.ok()); + + acf::AcfMessageInfo resp5, resp261; + std::vector payload5, payload261; + std::error_code ec5, ec261; + + std::thread t5([&]{ + auto ctx = Context::with_timeout(std::chrono::seconds(2)); + ec5 = client.request(ctx, standard_request(5, 7), {}, resp5, payload5); + }); + std::this_thread::sleep_for(std::chrono::milliseconds(30)); // let t5's insert land first + std::thread t261([&]{ + auto ctx = Context::with_timeout(std::chrono::seconds(2)); + ec261 = client.request(ctx, standard_request(261, 7), {}, resp261, payload261); + }); + + t5.join(); + t261.join(); + + REQUIRE_FALSE(ec5); + REQUIRE_FALSE(ec261); + REQUIRE(resp5.byte_bus_id == 5); + REQUIRE(resp261.byte_bus_id == 261); + REQUIRE(payload5 == std::vector{0, 5}); + REQUIRE(payload261 == std::vector{1, 5}); + + server.close(); + client.close(); +} + // ── Annex J encapsulation sequence number (real sockets) ──────────────────── TEST_CASE("Client's Annex J encapsulation sequence number increments monotonically "