bugfix in parsePacket(): accept short artnet packets#5588
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
softhack007
left a comment
There was a problem hiding this comment.
looks correct for me 👍 (not tested, as I don't have art-net equipment)
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/src/dependencies/e131/ESPAsyncE131.cpp (1)
102-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd minimum-length guards before Art-Net field reads
After defaulting to
P_ARTNET, undersized UDP packets can reach Line 114/Line 116 and dereference fields beyond the received buffer. Please gate the Art-Net branch with a minimum packet length (at least enough forart_id+ opcode) beforememcmp/opcode checks.Proposed fix
void ESPAsyncE131::parsePacket(AsyncUDPPacket _packet) { bool error = false; uint8_t protocol = P_ARTNET; const size_t pktLen = _packet.length(); e131_packet_t *sbuff = reinterpret_cast<e131_packet_t *>(_packet.data()); // E1.31 packet identifier (ACN_ID = "ASC-E1.17"), need at least 16 bytes to safely read acn_id (offset 4, length 12). if (pktLen >= 16) { if (!memcmp(sbuff->acn_id, ESPAsyncE131::ACN_ID, sizeof(sbuff->acn_id))) protocol = P_E131; } if (protocol == P_ARTNET) { + // Need at least Art-Net ID (8) + opcode (2) + if (pktLen < 10) { + error = true; + } else { if (memcmp(sbuff->art_id, ESPAsyncE131::ART_ID, sizeof(sbuff->art_id))) error = true; //not "Art-Net" if (sbuff->art_opcode != ARTNET_OPCODE_OPDMX && sbuff->art_opcode != ARTNET_OPCODE_OPPOLL) error = true; //not a DMX or poll packet + } } else { //E1.31 error handling🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/src/dependencies/e131/ESPAsyncE131.cpp` around lines 102 - 117, The Art‑Net branch currently assumes the buffer is large enough and can read sbuff->art_id and sbuff->art_opcode even for undersized packets; before the protocol == P_ARTNET checks, add a minimum-length guard using pktLen to ensure the packet is at least large enough to contain art_id and art_opcode (compare against the sum of sizes used by art_id and opcode) and only then perform memcmp(sbuff->art_id, ESPAsyncE131::ART_ID, ...) and the sbuff->art_opcode checks; update the logic around protocol/P_ARTNET and error setting so undersized packets are treated as error/ignored rather than dereferencing out-of-bounds data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@wled00/src/dependencies/e131/ESPAsyncE131.cpp`:
- Around line 102-117: The Art‑Net branch currently assumes the buffer is large
enough and can read sbuff->art_id and sbuff->art_opcode even for undersized
packets; before the protocol == P_ARTNET checks, add a minimum-length guard
using pktLen to ensure the packet is at least large enough to contain art_id and
art_opcode (compare against the sum of sizes used by art_id and opcode) and only
then perform memcmp(sbuff->art_id, ESPAsyncE131::ART_ID, ...) and the
sbuff->art_opcode checks; update the logic around protocol/P_ARTNET and error
setting so undersized packets are treated as error/ignored rather than
dereferencing out-of-bounds data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9d29c053-ac5b-4cc8-b1f4-a187cc9447f3
📒 Files selected for processing (1)
wled00/src/dependencies/e131/ESPAsyncE131.cpp
|
@DedeHai found this in the rabbit's "out-of-range comments" - might be worth to check.
|
fixes #5584
Summary by CodeRabbit