Skip to content

fix(mesh): drop oversized RX packets instead of asserting#10432

Open
DatanoiseTV wants to merge 1 commit into
meshtastic:developfrom
DatanoiseTV:fix/radio-rx-oversized-packet
Open

fix(mesh): drop oversized RX packets instead of asserting#10432
DatanoiseTV wants to merge 1 commit into
meshtastic:developfrom
DatanoiseTV:fix/radio-rx-oversized-packet

Conversation

@DatanoiseTV
Copy link
Copy Markdown
Contributor

handleReceiveInterrupt bounded the encrypted-payload memcpy with an
assert, which a future modem with a longer max payload would turn into
a panic-reset on a malformed (or simply larger) air packet. Replace
with a runtime length check that releases the pool entry, bumps rxBad,
and returns. assert() also vanishes in NDEBUG builds, so there was no
actual bound check in release.

Split out from #10424 per @thebentern's request — single-concern PR.

Build verification

pio run -e t-deck-tft succeeds, no new warnings.

Attestations

  • I have tested that my proposed changes behave as described — review/static-analysis only, not on-air.
  • On-hardware testing requested from community: build-verified t-deck-tft only.

@github-actions github-actions Bot added needs-review Needs human review bugfix Pull request that fixes bugs labels May 9, 2026
@thebentern thebentern requested a review from Copilot May 9, 2026 02:08
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the LoRa RX path in RadioLibInterface::handleReceiveInterrupt() by replacing an assert()-based bound check with a runtime length guard, avoiding panic-resets (and avoiding unchecked copies in NDEBUG builds) when an incoming packet’s encrypted payload is larger than the destination buffer.

Changes:

  • Replaced the assert(payloadLen <= sizeof(mp->encrypted.bytes)) with a runtime size check.
  • On oversized RX payloads: log a warning, release the allocated packet, increment rxBad, log RX airtime, and return early.

Comment thread src/mesh/RadioLibInterface.cpp
handleReceiveInterrupt bounded the encrypted-payload memcpy with an
assert, which a future modem with a longer max payload would turn into
a panic-reset on a malformed (or simply larger) air packet. Replace
with a runtime length check that releases the pool entry, bumps rxBad,
and returns. assert() also vanishes in NDEBUG builds, so there was no
actual bound check in release.
@DatanoiseTV DatanoiseTV force-pushed the fix/radio-rx-oversized-packet branch from 5303074 to 277817b Compare May 9, 2026 02:29
@DatanoiseTV
Copy link
Copy Markdown
Contributor Author

Good catch — fixed in 277817b. The outer else branch increments rxGood before any of the inner validity checks, so the cleanest minimal fix is to decrement it back when reclassifying the packet as rxBad on this path. Matches the existing pattern that from == 0 (which also early-returns from this same outer-else block) leaves untouched, while keeping our new oversized branch counted exactly once.

@thebentern thebentern requested a review from GUVWAF May 9, 2026 17:44
packetPool.release(mp);
// The outer else branch already incremented rxGood for this packet;
// undo that and reclassify as rxBad so num_packets_rx isn't double-counted.
rxGood--;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These statistics are really about whether it was correctly decoded when receiving over the air, which is the case (the CRC must have passed if you reach here), hence "rxGood" is still correct.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm with @GUVWAF here. don't touch the RX statistics at all, just make it fail gracefully instead of asserting. Also aomething to note: this was in place originally as a 'safeguard' against people stuffing too much into a meshpacket, so more of a sanity check. Arguably a happy path decision.

Copy link
Copy Markdown
Member

@caveman99 caveman99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please have a look at the failing tests. Can Probably ignore the 502 but the rest is of importance.

@cvaldess
Copy link
Copy Markdown
Contributor

Tested this on a Nordic nRF54L15-DK with EBYTE E22-900M30S (SX1262)
on an EU_868 mesh.

  • Builds clean (RAM 50.9%, Flash 32.4% — no footprint change).
  • Boots OK, LoRa RX flowing from neighbour nodes (rxSNR 11–13).
  • The defensive branch didn't fire in ~5 min of normal traffic (no oversized
    packets to provoke it), but the assert() removal alone is meaningful on
    this port: with CONFIG_RESET_ON_FATAL_ERROR=n our crash handler does a
    sys_reboot(SYS_REBOOT_COLD), so the previous bound check would have
    rebooted the device on the first malformed/oversized frame in release
    builds (where assert() already vanishes).

LGTM as a port-side reviewer.

Tested-by: cvaldess

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Pull request that fixes bugs needs-review Needs human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants