[Kartikey1306]

Update: #144 has merged, so this now applies directly to master — there is no longer a parent to merge first. The stacking note below predates that.

Stacked on #144 (master's Python guards are red without it). Review 9569226..337b06d for this change alone: two commits. 20c0b45 is the fix -- debug/src/gdb_stub.c (+36/−12), tests/test_debug.c (+3 tests), CHANGELOG.md. 337b06d answers the review's two findings: the drain is bounded and the m/M cap is derived from the packet size (debug/src/gdb_stub.c, tests/test_debug.c, new tests/unit/test_gdb_stub_limits.py, CHANGELOG.md).

Problem (eos #157)

qSupported answered PacketSize=400 — 1024 bytes — while eos_gdb_handle_exception() receives into char pkt[512]. GDB sends packets up to the size it is told, so any packet over 510 bytes was one the stub had promised to accept and then NACKed. Two things made that worse than a wrong number:

• recv_packet() stopped reading at the buffer's end and took the next two body bytes as the checksum, so the stream was out of step after an oversized packet.
• The dispatch loop read every negative recv_packet() result as a dead transport and exited. After the -, GDB resends — into a stub that had stopped listening. The same happened on the first packet with a corrupt checksum.

Verified in the scripted-transport harness: on the unfixed stub a packet of exactly the advertised size is NACKed (first_ack() == '-').

Change

• The limit is one constant derived from the buffer (GDB_PKT_MAX = GDB_PKT_BUF - 1), and qSupported reports it (PacketSize=1ff), so the two cannot drift apart again.
• A body that does not fit is drained to its #, its checksum consumed, and the packet refused with - — the next packet is read from where it really starts.
• recv_packet() returns GDB_RECV_BAD_PACKET for a refused packet and GDB_RECV_TRANSPORT_GONE when a read fails; only the latter ends the session. A refused packet is followed by GDB's resend, which the loop now waits for.

Nothing else in the protocol handling changes: the M/m/Z/z/q handlers and their existing rejection tests are untouched.

Tests (Verified)

Test	Unfixed stub	Fixed
test_gdb_packet_of_the_advertised_size_is_accepted — a q… body of exactly PacketSize bytes, then D	NACKed at the ack	+, empty reply 8, then # for D
test_gdb_packet_longer_than_advertised_does_not_desync — 40 bytes over, then D	(session ended)	-, no reply to it, # for D
test_gdb_corrupt_checksum_is_nacked_and_the_session_continues — #00, then D	session ended at the first packet	-, then #

ctest 43/43 on this head, pytest tests/unit 47 passed (43 + the 4 in the new guard).

Review follow-up (337b06d)

Finding	What changed
1 (P1) -- the for (;;) drain was unbounded; a peer that never sends # held the exception handler forever	The drain counts what it discards and gives up after another GDB_PKT_MAX bytes: - is written, GDB_RECV_BAD_PACKET is returned, the session stays up, and the $ search resynchronises. A body that is merely too long is still drained and refused. test_gdb_body_that_never_ends_is_abandoned: a read that hands out $ then x forever up to a 20,000-byte cap and only then reports the transport gone; the stub must write its ack after at most 2*PacketSize + 8 reads and it must be -. Negative control against 20c0b45: no ack is written before the cap (the test fails on reads_at_first_write > 0), which is the hang in miniature.
2 (P2) -- m/M refused len > 128 while PacketSize invites 248	GDB_MEM_MAX = ((GDB_PKT_MAX - GDB_MEM_HDR) / 2) with GDB_MEM_HDR = 15 (the longest M<addr>,<len>: with a 32-bit address), the buffers sized by it, no literal left. The accept path cannot run in tests/test_debug.c -- the memory hooks dereference the 32-bit address on a 64-bit host, as the file's header says -- so it is pinned from two sides: test_gdb_memory_transfer_one_past_the_cap_is_refused computes the cap from the advertised size, asserts it is above the old 128, and that one byte past it is E01 on an otherwise well-formed (+-acked) packet; tests/unit/test_gdb_stub_limits.py asserts in the source that PacketSize is fed GDB_PKT_MAX, that GDB_MEM_MAX is that derivation, that both handlers compare against it with no 128/256/257 literal, and that the largest m reply fits send_packet()'s buffer. Against 20c0b45, 3 of its 4 fail.

Closing issue

Fixes #157

[srpatcha]

Review — eos#158 "fix(gdb_stub): accept a packet of the advertised size, and survive a refused one"

head: 20c0b45 author: Kartikey1306 ci: pass — 30 green, 0 red; Create GitHub Release and assign skipped

Verdict: First look at 9569226..20c0b45 (one commit — debug/src/gdb_stub.c +36/−12,
tests/test_debug.c +85, CHANGELOG.md +1) as the body asks; the other nine files belong to
#144, reviewed separately at this exact base. The diagnosis is right and the three claimed
behaviours all reproduce: I built the tree, ran ctest (43/43), and confirmed by negative
control that first_ack() == '+' fails against the unfixed stub. But the fix trades a
desync for a hang. Replacing while (i < maxlen - 1) with for (;;) removed the only bound
on the body loop: a stream that never contains # now reads forever. I measured it — 2,000,000
reads and still going, against 514 on the code this replaces.

Findings

#	Severity	File:line	Finding	Recommended fix
1	High (P1)	debug/src/gdb_stub.c:71-79	The body loop is now unbounded, so a peer that never sends # hangs the exception handler forever. The old loop was while (i < maxlen - 1), which terminated after at most 511 iterations — badly (it mistook body bytes for the checksum), but it terminated. The new for (;;) exits only on # or on a failed read. On a blocking UART or TCP transport neither happens for a peer that keeps sending: eos_gdb_handle_exception() never returns, and the faulted CPU never proceeds. Measured, not inferred: a harness whose io.read returns 'x' forever ran 2,000,000 reads and was still reading against this head (harness cap, exit 42), and returned after 514 reads against 9569226's gdb_stub.c — same harness, same binary layout, only the file swapped. This is malformed-input handling on an externally reachable parser, which .ai/security.md requires defined behaviour for, and .ai/reviewer.md lists timeout among the failure paths a review must check. It is a regression this commit introduces, on the exact code path the commit exists to fix.	Bound the drain. Keep a separate counter for bytes consumed after the buffer fills and give up once it exceeds a stated limit — e.g. if (++drained > GDB_PKT_MAX) return GDB_RECV_BAD_PACKET; inside the else arm, so a body that is merely oversized is still drained and refused (which is the behaviour the PR wants and its test covers), while a body that never ends is abandoned. GDB_RECV_BAD_PACKET rather than GDB_RECV_TRANSPORT_GONE keeps the session alive for the resend; the stream may then be out of step, but the do { } while (c != '$') resynchroniser at :65-68 is what handles that and it is unchanged. One test: push a body of GDB_PKT_MAX * 2 bytes with no #, assert the call returns.
2	Medium (P2)	debug/src/gdb_stub.c:110-114 vs :130-134, :146-150	The advertised size is now consistent with the receive buffer, but still not with what the memory handlers accept — the same advertise/accept mismatch #157 is about, one layer up. qSupported reports PacketSize=1ff (511), so GDB sizes its bulk m/M transfers from 511 bytes of packet; handle_read_mem() and handle_write_mem() both refuse len > 128 with E01, i.e. a payload of 256 hex digits. A GDB that takes the stub at its word on a M will be told E01 for a request the stub invited — which is the shape of the defect the commit message describes ("GDB sends packets up to the size it is told"). Pre-existing, not introduced here: at PacketSize=400 the gap was wider. But this PR is the one that makes the advertised number mean something, and it leaves the half a user actually hits. Rated Medium rather than High because it degrades a debug session rather than corrupting it — E01 is a clean refusal with a test behind it (gdb rejects M packet with oversized length), not memory damage.	Derive the advertised size from the tightest real limit, the way GDB_PKT_MAX now derives from the buffer: either raise the m/M cap and the 128-byte buf[]/resp[257] with it, or advertise a PacketSize the memory handlers can actually satisfy. Whichever way, one constant, cited by both — the argument in the comment at :52-55 applies unchanged. If the 128-byte cap is deliberate (it looks it — it is tested), say so in that comment and state that PacketSize is bounded by the transport, not by the memory handlers.

Not findings, recorded so they are not mistaken for uncovered:

• The three new tests genuinely execute. ctest --verbose shows [PASS] gdb packet of the advertised size is accepted, … longer than advertised does not desync, … corrupt checksum is NACKed and the session continues, and the file's === ALL DEBUG TESTS PASSED ===. A test
  defined and never called from main() was the thing worth checking; it is not that.
• No check was weakened: no test disabled, no assertion removed, no lint loosened, no
  || true, no permission widened. The commit adds three assertions' worth of coverage and
  removes none. The one-line .github/workflows/ci.yml change in the 12-file view belongs to
  #144, not to this commit.
• buf[i] = '\0' at :80 writes buf[511] at most, the last byte of char pkt[512]. No
  overflow; the accept boundary is exactly the advertised one.
• handle_query's new char supported[64] with snprintf is bounded and generous for
  PacketSize=1ff;swbreak+;hwbreak+ (31 bytes).
• The PacketSize value is wire-visible and it changed (400 → 1ff), so brief item 8
  applies. The body and the CHANGELOG both state the change and why, which satisfies it.

Architecture conformance

Conforms. §21 places eos in Tier 1 — Foundation, and debug/ is inside the owning repo,
so §21.1 is not engaged — no repository boundary moves and no subsystem is split out. §5.1's
direction-of-dependency law holds: debug/src/gdb_stub.c includes only eos/gdb_stub.h,
<string.h> and <stdio.h>, and the commit adds no #include, no link line and no manifest
entry, so nothing points up a tier. The stub calls eos_gdb_read_mem/eos_gdb_write_mem,
which are declared in its own header — no new edge toward eAI, eNI, eBrowser, eOffice, health
or aerospace. §8's eBoot TCB rules are not engaged: this is the debug stub, not boot code.

The clause served is §6.1's "Kernel tracing hooks should exist without requiring tracing in
production images" and, more directly, §25.2's "Debugger, trace, memory/flash reporting" as
an MLP requirement — a debug session that ended on the first corrupt packet is not that.
§28's evidence model is respected: the PR claims "Verified" and backs each row with the
scripted-transport harness, which I re-ran.

No proposal appended. Nothing here shows the master design wrong, stale or silent — §6.1
and §25.2 name the requirement correctly and this is the implementation catching up. Finding 2
is a code-level inconsistency, not a design gap.

Proposed changes

1. Bound the drain (finding 1). Inside recv_packet()'s else arm:
       else if (++drained > GDB_PKT_MAX) return GDB_RECV_BAD_PACKET;
   with `int drained = 0;` beside `overflow`. A test that pushes a body of
   2 * GDB_PKT_MAX bytes with no '#' and asserts the call returns.
   This is the one that should not merge without an answer.

2. Reconcile PacketSize with the m/M cap, or document why they differ
   (finding 2). Not blocking.

No fix PR opened. Finding 1 is High, which is inside the brief's autofix rule, but the
defect exists only on this PR's branch — origin/master's recv_packet() still has the
bounded while (i < maxlen - 1) loop, verified by the negative control above. A branch cut
from the default branch, which is what fix-start.sh produces, would have nothing to patch,
and the brief forbids touching a branch belonging to an existing PR. The repair is delivered as
text and the harness that demonstrates the defect is described precisely enough to reproduce.

Verification performed for this review

The eos checkout has a clean working tree (git status --short empty, before and after)
and was not modified — no worktree, checkout, stash or reset. The PR head was fetched
read-only into refs/autoreview/pr158. Everything below ran against
git archive 20c0b45b | tar -x into a temporary directory outside the repository, with GCC 15
and CMake/Ninja from the host. Nothing was committed or pushed.

Check	Result
cmake -B build/host -G Ninja -DCMAKE_BUILD_TYPE=Release -DEOS_BUILD_TESTS=ON -DEOS_PRODUCT=vbox_test	PASS
cmake --build build/host --parallel 4	PASS
ctest --output-on-failure --no-tests=error --timeout 120 --parallel 4	PASS — 100%, 43/43, 0 failed. Matches the body's "43/43" exactly
ctest -R test_debug --verbose	All three new tests execute and pass, plus the 12 pre-existing ones
Negative control — debug/src/gdb_stub.c restored to 9569226, test_debug rebuilt	FAIL, as claimed: test_debug.c:219: test_gdb_packet_of_the_advertised_size_is_accepted: Assertion 'first_ack() == '+'' failed, subprocess aborted. Restored afterwards; ctest -R test_debug green again
Unbounded-drain harness, this head — io.read returns 'x' forever, no #	2,000,000 reads, still reading (harness cap) — finding 1
Same harness, 9569226's gdb_stub.c	returned after 514 reads — the bound this commit removes
Compiler warnings in the changed files	none. gdb_stub.c and test_debug.c produce no warnings. The build emits 89 warnings in total, all pre-existing and all elsewhere (core/src/config.c 32 -Wstringop-truncation, systems/src/rootfs.c 7 -Wformat-truncation, and others) — not this PR's, recorded only so the clean result above is not read as a clean build
recv_packet() call sites	exactly one, gdb_stub.c:238, and it is the one taught the new return codes
PacketSize / buffer references anywhere else	none outside gdb_stub.c, test_debug.c and CHANGELOG.md; no second copy of the constant to drift
git show --stat 20c0b45b	3 files, +122/−12 — the body's scope claim is accurate
CI on this head (checks.txt)	30 pass, 0 fail, incl. Build & Test (Linux x86_64), Build & Test (Windows MSVC), Host Tests (x86_64), Cross-compile ARM Cortex-M4 (×2), Cross-compile ARM64 kernel, QEMU Boot Test, QEMU ARM64 simulation (13 TCs), Static Analysis (cppcheck + clang-tidy), CodeQL, Analyze (C/C++), policy / Policy / Linked Issue
PR metadata	mergeable: MERGEABLE, mergeStateStatus: BLOCKED, reviewDecision: REVIEW_REQUIRED

Not checked

• No real GDB was attached. Every behaviour above went through tests/test_debug.c's
  scripted transport or my own harness, both of which are in-process arrays, not a serial line
  or a socket. In particular I did not verify GDB's own interpretation of PacketSize —
  whether it counts the $, # and the two checksum bytes, and how much it reserves as
  overhead when sizing an m/M. Finding 2's claim that GDB will request more than 128 bytes
  of memory at PacketSize=1ff is Inferred from the protocol and the 128-byte cap in the
  handlers, not observed. The finding's other half — that the stub advertises 511 and refuses
  memory requests above 128 — is read directly from :110-114, :131 and :147 and is not in
  doubt.
• The blocking-read assumption behind finding 1 was not tested on a real transport. My
  harness's read always succeeds, which models a peer that keeps sending. A transport whose
  read returns 0 on idle — as the test harness's does — yields GDB_RECV_TRANSPORT_GONE and
  terminates. The hang requires a peer that keeps the bytes coming, or a blocking read; both are
  ordinary for UART and TCP, and gdb_stub.h:20's int (*read)(void*, uint8_t*, int) does not
  document which. That ambiguity is itself worth a sentence in the header.
• Cross-compilation — NOT RUN locally. Only the host x86_64 build was configured and built.
  The ARM Cortex-M4, ARM64 and MSVC legs are green in CI and I did not reproduce them.
• QEMU simulation — NOT RUN locally. Green in CI; not reproduced.
• cppcheck / clang-tidy — NOT RUN locally. Not invoked by the host build. The
  Static Analysis job is green on this head, and neither tool is likely to flag for (;;)
  with a return inside it, so finding 1 would not have been caught there either.
• The #144 half of the 12-file diff is not reviewed here — reviewed separately at head
  95692266, which is this PR's base. If #144 changes, the 43/43 figure no longer describes
  what will merge.
• mergeStateStatus: BLOCKED — REVIEW_REQUIRED plus the #144 stack. Land #144 first, as
  the body says.

---

Automated architecture review of 20c0b45b858b — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

[Kartikey1306]

Both taken, in 337b06d (range now 9569226..337b06d; the body has a "Review follow-up" table). The hang measurement is the finding I should have made myself — I bounded the buffer and unbounded the loop in the same edit.

Finding 1 — the drain never ends. The drain now counts what it discards and gives up after another GDB_PKT_MAX bytes, exactly as you laid it out: - goes out, GDB_RECV_BAD_PACKET comes back so the session stays up, and the $ search at the top of recv_packet() resynchronises on whatever follows. A body that is merely too long is still drained to its # and refused, which the existing desync test still covers. test_gdb_body_that_never_ends_is_abandoned uses a read that hands out $ and then x until a 20,000-byte cap and only then reports the transport gone, and asserts the stub wrote its ack after at most 2*PacketSize + 8 reads and that the ack was -. Against 20c0b45 the stub writes nothing before the cap, so the test fails on reads_at_first_write > 0 — your two-million-reads result in miniature.

Finding 2 — the memory cap. Derived, the way you asked: GDB_MEM_MAX = ((GDB_PKT_MAX - GDB_MEM_HDR) / 2), GDB_MEM_HDR = 15 for the longest M<addr>,<len>: with a 32-bit address, so 248 bytes; both handlers compare against it and size their buffers by it, and the 128/257 literals are gone. The accept path cannot run in tests/test_debug.c — eos_gdb_read_mem()/eos_gdb_write_mem() dereference the 32-bit address, so an accepted M0,f8:… segfaults on a 64-bit host (I tried) — so it is pinned from two sides: test_gdb_memory_transfer_one_past_the_cap_is_refused computes the cap from the advertised size, asserts it is above the old 128, and that one byte past it is E01 on a well-formed packet; tests/unit/test_gdb_stub_limits.py asserts in the source that PacketSize is fed GDB_PKT_MAX, that GDB_MEM_MAX is that derivation, that neither handler carries a literal, and that the largest m reply fits send_packet()'s buffer. 3 of those 4 fail against 20c0b45.

ctest 43/43 (test_debug 18 PASS lines), pytest tests/unit 47.

[srpatcha]

Reviewed. Approve. A 248-byte memory read is now a 500-byte reply inside the 512-byte buffer, and test_debug aborts on master and passes here. Nit (pre-existing): send_packet() (gdb_stub.c:43-49) does not clamp snprintf's return value to the buffer. Not tested against a real GDB.