From ebc103f0854720e456fa36eceef22520fc0e738e Mon Sep 17 00:00:00 2001 From: mikecarper Date: Fri, 16 Oct 2026 05:26:29 -0700 Subject: [PATCH] Reject binary trace routes that exceed representable hop count --- examples/companion_radio/MyMesh.cpp | 2 +- test/fixtures/trace_profile_timing.cpp | 117 +++++++++++++++++++++++-- test/test_trace_profile_timing.py | 32 ++++++- 3 files changed, 142 insertions(+), 9 deletions(-) diff --git a/examples/companion_radio/MyMesh.cpp b/examples/companion_radio/MyMesh.cpp index 1aa9d6088..9229d8926 100644 --- a/examples/companion_radio/MyMesh.cpp +++ b/examples/companion_radio/MyMesh.cpp @@ -7037,7 +7037,7 @@ void MyMesh::handleCmdFrame(size_t len) { uint8_t path_len = len - 10; uint8_t flags = cmd_frame[9]; uint8_t path_sz = flags & 0x03; // NEW v1.11+ - if ((path_len >> path_sz) > MAX_PATH_SIZE || (path_len % (1 << path_sz)) != 0) { // make sure is multiple of path_sz + if ((path_len >> path_sz) >= MAX_PATH_SIZE || (path_len % (1 << path_sz)) != 0) { // make sure is multiple of path_sz writeErrFrame(ERR_CODE_ILLEGAL_ARG); } else { uint32_t tag, auth; diff --git a/test/fixtures/trace_profile_timing.cpp b/test/fixtures/trace_profile_timing.cpp index 918fa7e0a..a1e5ba8b2 100644 --- a/test/fixtures/trace_profile_timing.cpp +++ b/test/fixtures/trace_profile_timing.cpp @@ -11,6 +11,8 @@ #include #define ERR_EVENT_FULL 1 +#define ACTION_RELEASE 0 +#define ACTION_RETRANSMIT_DELAYED(pri, delay) ((((uint32_t)1 + (pri))<<24) | (delay)) #include "constants.inc" class RadioLibWrapper { @@ -42,7 +44,9 @@ struct Manager { std::array packets; std::array used{}; std::vector queued; + unsigned allocations = 0, releases = 0; mesh::Packet* allocNew() { + ++allocations; for (unsigned i = 0; i != used.size(); ++i) if (!used[i]) { used[i] = true; return &packets[i]; } @@ -53,7 +57,7 @@ struct Manager { } void free(mesh::Packet* packet) { for (unsigned i = 0; i != used.size(); ++i) if (&packets[i] == packet) { - assert(used[i]); used[i] = false; return; + assert(used[i]); used[i] = false; ++releases; return; } assert(false); } @@ -84,13 +88,26 @@ public: bool queueOutboundPacket(Packet*, uint8_t, uint32_t); bool sendPacket(Packet*, uint8_t, uint32_t); }; -struct Tables { void markSent(const Packet*) {} }; -static uint8_t getTraceDirectPriority(const Packet*) { return 0; } +struct Tables { + void markSent(const Packet*) {} + bool wasSeen(const Packet*) { return false; } + void markSeen(const Packet*) {} +}; +#include "trace_priority.inc" class Mesh : public Dispatcher { public: Tables tables; Tables* _tables = &tables; void maybeScheduleDirectRetry(Packet*, uint8_t) {} + struct Identity { + uint8_t key[8] = {}; + bool isHashMatch(const uint8_t* bytes, size_t length) { return memcmp(bytes, key, length) == 0; } + } self_id; + unsigned completed = 0; + bool allowPacketForward(const Packet*) { return true; } + uint32_t getDirectRetransmitDelay(const Packet*) { return 0; } + void onTraceRecv(Packet*, uint32_t, uint32_t, uint8_t, const uint8_t*, const uint8_t*, uint8_t) { ++completed; } + uint32_t receiveTrace(Packet*); Packet* createTrace(uint32_t, uint32_t, uint8_t); bool sendDirect(Packet*, const uint8_t*, uint8_t, uint32_t = 0); }; @@ -211,10 +228,10 @@ static unsigned producerChecks() { unsigned cases = 0; for (unsigned mode = 0; mode != 10; ++mode) for (bool swapped : {false, true}) for (bool terminal : {false, true}) for (uint8_t hash_size : {1, 2, 4, 8}) - for (uint8_t hops : {1, 7, 20, 21, 31, 41, 43, 63, 64}) { + for (uint8_t hops : {1, 7, 20, 21, 31, 41, 43, 63}) { const unsigned bytes = unsigned(hash_size) * hops; if (bytes > MAX_PACKET_PAYLOAD - 9 || (!terminal && bytes > MAX_FRAME_SIZE - 10) - || (terminal && (hash_size == 8 || hops == 64))) continue; + || (terminal && hash_size == 8)) continue; uint8_t route[MAX_PACKET_PAYLOAD] = {}; for (unsigned i = 0; i < bytes; ++i) route[i] = uint8_t(i + 1); const uint8_t flags = hash_size == 8 ? 3 : mesh::traceFlagsForHashSize(hash_size); @@ -283,7 +300,95 @@ static unsigned helperChecks() { return cases; } -int main() { +struct TraceWalk { + unsigned forwarded, completed, path_length, snr_bytes, wire_length; +}; + +static TraceWalk forwardTrace(MyMesh& origin, uint8_t hash_size, uint8_t hops) { + uint8_t wire[MAX_TRANS_UNIT]; + unsigned length = origin.manager.queued.front()->writeTo(wire); + for (unsigned hop = 0; hop != hops; ++hop) { + MyMesh receiver; receiver._radio = origin._radio; + auto* packet = receiver.obtainNewPacket(); assert(packet); + assert(packet->readFrom(wire, length)); + memcpy(receiver.self_id.key, packet->payload + 9 + hop * hash_size, hash_size); + packet->_snr = 16; + const uint32_t action = receiver.receiveTrace(packet); + assert(action != ACTION_RELEASE && receiver.completed == 0); + assert(packet->path_len == hop + 1); + assert(receiver.queueOutboundPacket(packet, (action >> 24) - 1, action & 0x00ffffff)); + length = packet->writeTo(wire); + assert(length <= MAX_TRANS_UNIT); + receiver.releasePacket(packet); + } + MyMesh endpoint; endpoint._radio = origin._radio; + auto* packet = endpoint.obtainNewPacket(); assert(packet); + assert(packet->readFrom(wire, length)); + assert(endpoint.receiveTrace(packet) == ACTION_RELEASE); + TraceWalk result{hops, endpoint.completed, packet->path_len, + packet->getPathByteLen(), length}; + endpoint.releasePacket(packet); + return result; +} + +static void hopBoundaryChecks(unsigned selected_hash = 0) { + for (uint8_t hash_size : {1, 2}) { + if (selected_hash && selected_hash != hash_size) continue; + for (uint8_t hops : {63, 64}) { + Radio radio; configure(radio, 2, false); + MyMesh node; node._radio = &radio; + node.cmd_frame[0] = CMD_SEND_TRACE_PATH; + const uint32_t tag = 17, auth = 23; + memcpy(node.cmd_frame + 1, &tag, 4); memcpy(node.cmd_frame + 5, &auth, 4); + node.cmd_frame[9] = hash_size - 1; + for (unsigned hop = 0; hop != hops; ++hop) { + node.cmd_frame[10 + hop * hash_size] = hop + 1; + if (hash_size == 2) node.cmd_frame[11 + hop * hash_size] = 0x99; + } + const size_t host_length = 10 + hash_size * hops; + assert(host_length <= MAX_FRAME_SIZE); + node.handleTraceFrame(host_length); + if (hops == 63) { + assert(node.error == 0 && node._delayed_replies.hasBinaryTrace()); + assert(node.manager.allocations == 1 && node.manager.queued.size() == 1); + const auto walked = forwardTrace(node, hash_size, hops); + assert(walked.forwarded == 63 && walked.completed == 1); + assert(walked.path_length == 63 && walked.snr_bytes == 63); + } else if (node.error != ERR_CODE_ILLEGAL_ARG) { + // Exact old admission exposes two independent protocol limits: a raw + // path_len of 64 serializes as packed hash-size 2/count 0, and TRACE's + // completion callback is guarded by path_len < MAX_PATH_SIZE. + assert(node.error == 0 && node._delayed_replies.hasBinaryTrace()); + assert(node.manager.allocations == 1 && node.manager.queued.size() == 1); + const auto walked = forwardTrace(node, hash_size, hops); + assert(walked.forwarded == 64 && walked.completed == 0); + assert(walked.path_length == 64 && walked.snr_bytes == 0); + const uint32_t airtime = radio.driver.getProfileAirtime(0, 11 + hash_size * hops); + const uint32_t wrapped = 500U + (6U * airtime + 250U); + const uint32_t unwrapped = 500U + (6U * airtime + 250U) * 65U; + assert(node._delayed_replies.trace.timeout == wrapped && wrapped < unwrapped); + fprintf(stderr, "old 64-hop admission hash=%u host=%zu forwarded=%u completed=%u snr_bytes=%u timeout=%u unwrapped=%u\n", + hash_size, host_length, walked.forwarded, walked.completed, walked.snr_bytes, wrapped, unwrapped); + assert(false && "64-hop trace must be rejected: admitted route cannot complete and timeout count wraps"); + } else { + assert(node.manager.allocations == 0 && node.manager.releases == 0); + assert(node.manager.queued.empty() && node.rng.calls == 0); + assert(!node._delayed_replies.hasBinaryTrace() && !node._terminal_trace_pending); + assert(node._delayed_replies.trace.phase == mesh::CompanionDelayedReplies::Empty); + for (unsigned i = 0; i != mesh::CompanionDelayedReplies::HISTORY_SIZE; ++i) + assert(node._delayed_replies.reserveTrace(tag + i, auth, node.clock.now) + != mesh::CompanionDelayedReplies::NO_HISTORY); + } + } + } + puts("PASS: 63-hop traces complete and 64-hop commands have no side effects"); +} + +int main(int argc, char** argv) { + if (argc == 2 && strncmp(argv[1], "hop-boundary", 12) == 0) { + hopBoundaryChecks(strlen(argv[1]) == 14 ? argv[1][13] - '0' : 0); + return 0; + } const unsigned producers = producerChecks(), helpers = helperChecks(); printf("PASS: trace timing %u producer cases, %u existing/helper policy cases\n", producers, helpers); } diff --git a/test/test_trace_profile_timing.py b/test/test_trace_profile_timing.py index 31fcca303..ac1251c30 100644 --- a/test/test_trace_profile_timing.py +++ b/test/test_trace_profile_timing.py @@ -16,7 +16,7 @@ FIXTURE = ROOT / "test/fixtures/trace_profile_timing.cpp" class TraceProfileTimingTests(unittest.TestCase): - def run_fixture(self, mutation=None): + def run_fixture(self, mutation=None, scenario=None): companion = (ROOT / "examples/companion_radio/MyMesh.cpp").read_text() dispatcher = (ROOT / "src/Dispatcher.cpp").read_text() mesh = (ROOT / "src/Mesh.cpp").read_text() @@ -33,6 +33,10 @@ class TraceProfileTimingTests(unittest.TestCase): methods += extract_braced(dispatcher, signature) + "\n" for signature in ("Packet* Mesh::createTrace(", "bool Mesh::sendDirect("): methods += extract_braced(mesh, signature) + "\n" + trace_receive = extract_braced(mesh, + "if (pkt->isRouteDirect() && pkt->getPayloadType() == PAYLOAD_TYPE_TRACE)") + methods += "uint32_t Mesh::receiveTrace(Packet* pkt) {\n" + trace_receive + methods += "\nreturn ACTION_RELEASE;\n}\n" methods += "}\n" + extract_braced(driver, "uint32_t RadioLibWrapper::getProfileAirtime(") + "\n" terminal = extract_braced(companion, "void MyMesh::sendTerminalTraceRoute(") binary = extract_braced(companion, "} else if (cmd_frame[0] == CMD_SEND_TRACE_PATH") @@ -52,6 +56,10 @@ class TraceProfileTimingTests(unittest.TestCase): work = Path(directory) (work / "production.inc").write_text(methods, encoding="ascii") (work / "constants.inc").write_text(constants, encoding="ascii") + (work / "trace_priority.inc").write_text("\n".join(extract_braced(mesh, signature) + for signature in ("static uint8_t decodeTraceHashSize(", + "static uint8_t getTraceRemainingHops(", + "static uint8_t getTraceDirectPriority(")), encoding="ascii") binary_path = work / "trace" sanitizers = (["-fsanitize=address,undefined", "-fno-sanitize-recover=all", "-fno-pie", "-no-pie"] if sys.platform.startswith("linux") else []) @@ -62,7 +70,27 @@ class TraceProfileTimingTests(unittest.TestCase): str(ROOT / "src/helpers/CompanionDelayedReplies.cpp"), "-o", str(binary_path)], capture_output=True, text=True, timeout=60) self.assertEqual(built.returncode, 0, built.stdout + built.stderr) - return subprocess.run([str(binary_path)], capture_output=True, text=True, timeout=20) + return subprocess.run([str(binary_path)] + ([scenario] if scenario else []), + capture_output=True, text=True, timeout=20) + + def test_binary_hop_boundary_rejects_64_before_side_effects_and_63_completes(self): + result = self.run_fixture(scenario="hop-boundary") + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + self.assertIn("PASS: 63-hop traces complete and 64-hop commands have no side effects", result.stdout) + + def test_exact_old_64_hop_admission_cannot_complete_and_wraps_timeout(self): + def old(terminal, binary): + changed, count = re.subn(r"\(path_len >> path_sz\) >= MAX_PATH_SIZE", + "(path_len >> path_sz) > MAX_PATH_SIZE", binary) + self.assertEqual(count, 1) + return terminal, changed + for hash_size in (1, 2): + with self.subTest(hash_size=hash_size): + result = self.run_fixture(old, "hop-boundary-" + str(hash_size)) + self.assertNotEqual(result.returncode, 0) + self.assertIn("old 64-hop admission hash=" + str(hash_size), result.stderr) + self.assertIn("64-hop trace must be rejected: admitted route cannot complete and timeout count wraps", + result.stderr) def test_actual_trace_producers_mask_admission_and_complete_wire_length(self): result = self.run_fixture()