RadioLibInterface: harden RX path (4 related fixes)

[nightjoker7]

Summary

Four small, related hardening fixes in src/mesh/RadioLibInterface.cpp, all in handleReceiveInterrupt() / randomBytes(). Bundled because splitting would be four one-to-two-liners on the same function; happy to split on request.

1. random(0, 255) off-by-one in randomBytes()

RadioLib's random(min, max) is half-open — it returns values in [min, max), not [min, max]. random(0, 255) therefore covers 0-254 and can never produce byte value 255. Changed to random(0, 256) for the full 0-255 range.

Impact: minor statistical bias in any randomness seeded via this path (e.g., PKI key material on platforms that fall through to this slow path). Not security-critical in practice but is wrong.

2. CRC-mismatch log flood

In noisy RF environments every preamble-triggered corrupted packet fires LOG_ERROR with 9 format fields of context. In my urban deployment I was seeing 500+ CRC_MISMATCH lines per hour — buries real errors.

Split the error-path so only RADIOLIB_ERR_CRC_MISMATCH drops to LOG_DEBUG with a shorter format string. All other error states still get the full LOG_ERROR context.

3. from == 0 mis-classified as rxGood

The existing check correctly refuses to process packets where sender is zeroed out (a known Remote-Node-Admin bypass vector per the security audit):

rxGood++;   // <-- increments before the reject check
// altered packet with "from == 0" can do Remote Node Administration without permission
if (radioBuffer.header.from == 0) {
    LOG_WARN("Ignore received packet without sender");
    return;   // <-- returns WITHOUT logging airtime
}

Two bugs:

• increments rxGood before the early return — so a rejected packet inflates the good-RX counter used for reliability metrics
• returns without calling airTime->logAirtime(RX_ALL_LOG, rxMsec) — airtime accounting misses this packet's time on-air

Fixed by moving rxGood++ past the check, and adding rxBad++ + airTime->logAirtime() on the reject path, matching the other reject branches in this function.

4. packetPool.allocZeroed() NULL dereference under heap pressure

meshtastic_MeshPacket *mp = packetPool.allocZeroed();
// no null-check
mp->from = radioBuffer.header.from;   // <-- hard-fault if mp is NULL

allocZeroed() can return nullptr when the packet pool is exhausted — e.g., during a phone-connect config-dump that's eating heap while RX is still running, or during an MQTT-downlink burst.

Added a null-check that drops the packet and logs airtime. This matches the pattern used elsewhere in the function for resource-exhaustion rejects (payloadLen < 0 etc.).

Risk

• Happy path: byte-identical, zero behavior change. 4 of 4 are reject-path or accounting-only.
• Noisy RF: slightly quieter logs (item 2).
• Heap pressure: prevents one hard-fault class (item 4) — converts crash to packet-drop.
• Reject-path accounting: now correctly excludes rejected packets from rxGood (item 3).

Testing

Deployed on two-node Station G2 fleet (custom build 2.7.23.0fcf6c3) for several weeks. No regressions. Measurable reduction in log volume; no observed LoadProhibited from the allocZeroed path since applying.

Related

• Part of a broader RX-path hardening pass. Separate PRs in this series:
  • Router: null-check destination node before dereferencing public_key in PKI decode #10226 — Router PKI destination null-check
  • NextHopRouter: fix 49.7-day millis() rollover in retransmission timing #10227 — NextHopRouter millis() rollover
  • TrafficManagement: exempt traceroute from per-source rate limiting #10228 — TrafficMgmt TRACEROUTE_APP exempt
  • MeshService: null-check sender node before dereferencing has_user on hot RX path #10229 — MeshService sender null-check
  • StreamInterface: prevent socket/reader-thread leak on handshake failure python#918 — client-side TCPInterface handshake-leak (often triggers the heap-pressure conditions that expose item 4)

[Copilot]

Pull request overview

This PR hardens the RadioLib-backed RX interrupt path and a RadioLib RNG helper to improve correctness, accounting, and log signal-to-noise in RadioLibInterface.

Changes:

• Fix off-by-one byte generation in randomBytes() by adjusting the random() upper bound.
• Reduce CRC-mismatch log volume by downgrading that specific error to LOG_DEBUG with a shorter message.
• Improve RX reject-path accounting (from==0) and add a null-check for packetPool.allocZeroed() to avoid crashes under pool exhaustion.

[Copilot]

rxGood++ is incremented before packetPool.allocZeroed(). With the new nullptr-drop path, a packet that is ultimately dropped due to pool exhaustion will still be counted as a good RX. Consider moving rxGood++ to after a successful allocation (and ideally after enqueue/deliver), and treating allocation failure as rxBad instead.

[GUVWAF]

This is still a correctly received packet over the air, which is where the statistic is for. It's just that we don't allow this.

[GUVWAF]

I agree it should not really be considered an error, but it's valuable information to have. I would rather leave it as LOG_INFO, and also please move all the original parameters back into the log statement.

[GUVWAF]

This is also not a "bad" Rx. And in this case you already counted it as "good" too.

[nightjoker7]

Thanks for the review @GUVWAF — all three points addressed in 93cf96fab:

CRC mismatch (line 488) — kept at LOG_INFO (not DEBUG), restored the full parameter set from the original LOG_ERROR call (id, from, to, flags, SNR, RSSI, next_hop, relay_node).

from == 0 path (line 514) — dropped the rxBad++ and airTime->logAirtime I had added. This path now just LOG_WARNs and returns; the OTA reception is already counted as rxGood above per your rationale.

payloadLen < 0 path (line 529) — same treatment. Dropped rxBad++ and airTime, kept the LOG_WARN. This was inside my original refactor so it matched the other paths; now it matches the original 'silent ignore' behavior.

Pool-exhaustion path — I also resolved a conflict with the pre-applied Copilot suggestion (rxBad++ on the NULL-alloc branch). Copilot wanted to increment rxBad there, but that directly contradicts your "also not a 'bad' Rx" comment on the same class of non-processing paths. I went with your reading — dropped the LOG_ERROR (the allocator already WARNs) and did not add rxBad++. The rxGood++ above still counts the OTA reception.

Net effect: all 4 original bugs remain fixed (CRC log volume, randomBytes off-by-one, from==0 null-deref, pool-exhaustion null-check) with counter semantics aligned to existing philosophy.

[GUVWAF]

This is now neither counted as good, nor bad.

[GUVWAF]

I don't consider this "debug" information. "Error" is fine here, but at least "Warn".

[nightjoker7]

@GUVWAF rebased on the latest develop tip — was flagged CONFLICTING. Branch is now 4 clean commits on top of 989b8620b (no new code from me beyond the rebase; same logical diff: +32/-10 across RadioLibInterface.cpp and Router.cpp).

Your 5 inline comments from 2026-04-23/24 are addressed in the existing commits, which survived the rebase as 65117c09f and 8f17900e2:

• RadioLibInterface.cpp:514 "still a correctly received packet" — rxGood++ is now hoisted to the top of the success branch so any CRC-clean packet counts, regardless of whether we then refuse to process it.
• :488 "leave it as LOG_INFO with original parameters" — CRC mismatches now use LOG_INFO with all original packet-header fields (id / from / to / flags / SNR / RSSI / nextHop / relay) preserved. Other RX errors stay at LOG_ERROR.
• :529 "not a bad Rx, already counted as good" — rxBad++ removed from the from==0 path; counters now report once per packet.
• :500 "neither good nor bad" — fixed by hoisting rxGood++ so all paths hit exactly one counter.
• Router.cpp:502 "Warn at minimum, Error fine" — promoted from LOG_DEBUG to LOG_WARN.

Verified locally: pio test -e coverage -f test_radio -f test_default → 26/26 pass. Re-requesting review.