A BATTERY frame with len(data) < 3 caused dbuf.read(2) to return short
bytes; int.from_bytes(b"", ...) silently yielded 0, propagating a bogus
level=0 to HA sensors. Same silent-zero class as N07 (storage fields).
Option B: early-return with debug log, matching the NEW-C pattern for
STATUS_RESPONSE. No BATTERY event is dispatched for malformed frames.
Not in the original forensics report — discovered during G1 N07 work and
logged in issues_log.md. Resolved here because no later branch touches
this handler.
Files changed:
- src/meshcore/reader.py: add `if len(data) < 3: return` guard before
the level read in the BATTERY branch
- tests/unit/test_reader.py: add test_g1_battery_too_short_for_level —
sends a 1-byte frame (type only), asserts no BATTERY event dispatched
and debug log emitted
Adds the four unit tests required by proposal §4.1 verification:
(a) test_g1_handle_rx_malformed_frame_logged_and_swallowed — F06.
Sends a 4-byte CHANNEL_MSG_RECV_V3 frame missing the channel_idx
byte. The handler raises IndexError on dbuf.read(1)[0]; the F06
umbrella must catch it, log "handle_rx parse error" with a
traceback, and return cleanly without dispatching any event.
(b) test_g1_battery_short_frame_omits_storage_fields — N07.
Sends a 3-byte BATTERY frame (type + 2-byte level only). Verifies
that exactly one BATTERY event is dispatched, the payload contains
`level` but does NOT contain `used_kb` or `total_kb`. Pre-fix the
`len(data) > 3` gate would have produced both fields with bogus
silent zeros.
(c) test_g1_status_response_short_frame_skipped — NEW-C.
Sends a 30-byte STATUS_RESPONSE push frame (well below the 60-byte
minimum). Verifies that no STATUS_RESPONSE event is dispatched and
the new "STATUS_RESPONSE push frame too short" debug log fires.
(d) test_g1_parse_packet_payload_txt_type_decodes_high_bits — R02.
Sets up a synthetic channel with a known 16-byte AES key, encrypts
a 16-byte plaintext where byte 4 = (5 << 2) | 1 = 0x15, builds the
full pkt_payload (chan_hash + cipher_mac + ciphertext), wraps it in
a minimal route header, calls parsePacketPayload directly, and
asserts log_data["txt_type"] == 5 and log_data["attempt"] == 1.
Pre-R02-fix `uncrypted[4:4]` was the empty slice, so txt_type was
always 0 — this test would have failed with txt_type=0.
Adds a small _CapturingDispatcher helper class. Adds imports for
logging at the top of the file. The existing test_binary_response is
left untouched.
File: tests/unit/test_reader.py
Three small cleanup fixes bundled per proposal §4.1 commit order.
N08 — CONTROL_DATA empty payload guard. The handler reads
`payload = dbuf.read()` then immediately dereferences `payload[0]`
without checking length. A zero-length payload (firmware truncation
or garbled frame) raises IndexError. Pre-F06 the IndexError would
escape; post-F06 it would log and skip the dispatch via the umbrella.
Adding an explicit `if len(payload) == 0: return` after the read
short-circuits the empty case before it touches `payload[0]`, with a
debug log noting the empty payload. The `return` exits handle_rx
cleanly without engaging the F06 umbrella's parse-error path, which
is the correct behavior — an empty CONTROL_DATA frame is not a parse
error, it's an unusable frame.
F12 — print(res) leftover debug. The RAW_DATA handler had a stray
`print(res)` polluting stdout. Replaced with `logger.debug(res)` to
match the surrounding `logger.debug("Received raw data")` line.
N10 — magic numbers 16 and 17. Two `elif packet_type_value == 16/17`
branches hardcoded the integer values for CONTACT_MSG_RECV_V3 and
CHANNEL_MSG_RECV_V3, both already declared in packets.py:94-95.
Replaced with `PacketType.CONTACT_MSG_RECV_V3.value` and
`PacketType.CHANNEL_MSG_RECV_V3.value` to eliminate drift risk if
the enum is ever renumbered.
Findings: N08 (Info), F12 (Info), N10 (Info)
File: src/meshcore/reader.py
`uncrypted[4:4]` is the empty slice. `int.from_bytes(b"", "little")`
returns 0, so `txt_type` was always 0 for every decrypted channel
message — silently masking the upper 6 bits of byte 4. The line
immediately above (`attempt = uncrypted[4] & 3`) already proves byte
4 is in range, so widening the slice to `[4:5]` is safe.
This is a one-character fix and changes the observable value of
`txt_type` for all callers. Existing consumers that branched on
`txt_type` were effectively dead code; this restores the intended
behavior.
Finding: R02 (Info)
File: src/meshcore/meshcore_parser.py
The ADVERT branch in MeshcorePacketParser.parsePacketPayload reads the
flags byte with `pk_buf.read(1)[0]`, which IndexErrors on a short
advert payload (the minimum advert is 32 + 4 + 64 + 1 = 101 bytes
before any optional fields). Pre-F06, the IndexError would escape as a
swallowed task exception. With F06's umbrella now in place it would
log and skip the dispatch, but the proposal §4.1 NEW-B asks for a
narrower local guard so a malformed advert doesn't poison the rest of
the parse path.
The optional `lat/lon/feat1/feat2` reads after the flags byte also
silently produce zeros on short reads (`int.from_bytes(b"", ...)`
returns 0), which would propagate bogus zero coordinates upstream.
Wrapping the whole branch limits the blast radius to a single
malformed advert.
Wrap the entire body of the ADVERT elif (from `pk_buf = io.BytesIO(...)`
through the final `log_data["adv_feat2"]` assignment) in
`try/except (IndexError, ValueError)` and log a debug message with the
exception type, message, and `pkt_payload` length on failure. This
matches the defensive pattern the proposal specifies.
Finding: NEW-B (S3)
File: src/meshcore/meshcore_parser.py
Add a 2-byte hard-floor length check at the top of
MeshcorePacketParser.parsePacketPayload. The minimum viable payload is 1
header byte + 1 path_byte for a direct route; anything shorter would
crash on `path_byte = pbuf.read(1)[0]` (IndexError on the empty buffer).
The reader.py LOG_DATA branch only requires `len(data) > 3`, so a 4-byte
LOG_DATA frame produces a 1-byte payload here — that path is reachable.
The caller in reader.py dereferences log_data['route_type'],
['payload_type'], ['path_len'], and ['path'] immediately after the
parse, so an empty log_data would KeyError on the dispatch (caught by
the F06 umbrella, but the dispatch would still be skipped). Populate
sentinel values matching the existing route_typename = "UNK" pattern in
the function before the early return so the caller's downstream lookups
don't KeyError, then return early with a debug log.
Finding: R01 (S2)
File: src/meshcore/meshcore_parser.py
Why: parse_status with offset=8 reads up through data[56:60]
(the rx_airtime field), so a full STATUS_RESPONSE push frame is
60 bytes: 1 type + 1 reserved + 6 pubkey + 52 status fields. The
push handler in handle_rx previously called parse_status with no
length check at all, so a short frame would slice through empty
data and silently produce zeros for every missing field. HA sensor
telemetry would silently report all-zero status — same class as N07.
The BINARY_RESPONSE STATUS path at the bottom of handle_rx already
gates parse_status with `len(response_data) >= 52` on its
offset-stripped buffer; this commit adds the equivalent gate for
the push path: `if len(data) < 60: log + return`. The `return`
short-circuits cleanly out of the umbrella try block without
dispatching a STATUS_RESPONSE event for the bogus parse.
Refs: Forensics report finding NEW-C (S3)
Why: The BATTERY handler previously gated the used_kb / total_kb
reads on `len(data) > 3`, which is wrong. The full
RESP_CODE_BATT_AND_STORAGE frame is 11 bytes (1 type + 2 level +
4 used_kb + 4 total_kb), so a 4-10 byte truncated frame would pass
the guard, and io.BytesIO.read(4) silently returns short bytes
instead of raising. int.from_bytes(b"", ...) returns 0, so HA
sensor telemetry silently reports zero storage on a truncated frame.
Tighten the guard to `len(data) >= 11` so the storage fields are
only parsed when the full frame is present. Inline comment added
to document the expected frame layout.
Note: the unconditional 2-byte `level` read at the top of the
handler has the same class of issue (no guard, silent zero on a
1-byte frame). That is out of scope for finding N07 and has been
logged in issues_log.md as a separate item.
Refs: Forensics report finding N07 (S3)
Why: The ALLOWED_REPEAT_FREQ branch in handle_rx had `except e:` —
syntactically valid Python only if `e` happens to be bound to an
exception class, which it isn't. The first time the inner read loop
actually raised, the except clause itself would raise NameError
("name 'e' is not defined") and propagate out of the handler. The
proposal correctly notes this is unreachable in practice today
because `int.from_bytes(b"", ...)` returns 0 so the loop terminates
cleanly, but it is a latent footgun. Replace with the standard
`except Exception as e:` form and swap the `print(e)` for a proper
`logger.warning(...)` call to match the rest of the file (which uses
the module logger, not stdout).
Refs: Forensics report finding F11 (S3)
Why: The LOGIN_FAILED handler in handle_rx referenced an undefined
identifier `pbuf` instead of the local BytesIO `dbuf`. Firmware emits
PUSH_CODE_LOGIN_FAIL as a fixed 8-byte frame, which trivially
satisfies the `len(data) > 7` guard, so every remote auth failure
raised NameError. The sibling LOGIN_SUCCESS handler a few lines above
already uses `dbuf.read(6).hex()` correctly; this commit aligns the
LOGIN_FAILED branch with the same pattern.
Refs: Forensics report finding F10 (S1)
Why: handle_rx is invoked from a detached task in MessageReader, so any
exception escaping its ~850-line if/elif dispatch is silently swallowed
by asyncio as "Task exception was never retrieved." The only crash
guard previously was a single try/except IndexError around the first
byte read; everything past line 73 was unguarded. This commit adds an
umbrella try: ... except Exception as e: around the entire dispatch
body that logs the exception class, message, raw frame hex, and full
traceback via logger.error. The umbrella neutralizes the crash surface
of F10, F11, N07, N08, R01, NEW-B, and NEW-C, which the next commits
will then fix individually now that they are observable.
Refs: Forensics report finding F06 (umbrella crash protection)
Matthew Wolter