DDP input: reject packets with unsupported data type or non-display destination#5390
DDP input: reject packets with unsupported data type or non-display destination#5390
Conversation
… data type Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
|
@copilot move all DDP defines into ESPAsyncE131.h so all of them are in one place, even the ones still in udp.cpp |
Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
Done in b1757c3. All remaining DDP defines ( |
…nd add to .gitignore Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
| int lastPushSeq = e131LastSequenceNumber[0]; | ||
|
|
||
| // reject unsupported data types (only RGB and RGBW are supported) | ||
| if (p->dataType != DDP_TYPE_RGB24 && p->dataType != DDP_TYPE_RGBW32) return; |
There was a problem hiding this comment.
@DedeHai this is the core change, right?
Maybe its worth to make a backport to 0.15.4 with just this sanity check
There was a problem hiding this comment.
that plus the next line, the rest was just moving the defines (and renaming)
| // reject status and config packets (not implemented) | ||
| if (p->destination == DDP_ID_STATUS || p->destination == DDP_ID_CONFIG) return; | ||
|
|
||
| //reject late packets belonging to previous frame (assuming 4 packets max. before push) |
There was a problem hiding this comment.
@DedeHai maybe also check if this assumption "max 4 packets per push" still makes sense.
Not sure how many LEDs fit into one packet - but considering maybe 4 packets * 512 LEDs as the absolute max content per frame doesn't sound right to me - we can support more than 2048 LEDs for example with HUB75.
There was a problem hiding this comment.
needs investigation. I have pushed 64x64 over websocket no problem - not sure how big one WS packet is but code comments suggest its about 500 LEDs worth.
There was a problem hiding this comment.
I've created an issue ticket so we can come back to this question later.
| int lastPushSeq = e131LastSequenceNumber[0]; | ||
|
|
||
| // reject unsupported data types (only RGB and RGBW are supported) | ||
| if (p->dataType != DDP_TYPE_RGB24 && p->dataType != DDP_TYPE_RGBW32) return; |
There was a problem hiding this comment.
If I read the DDP specs correctly, datatype can also be 0 = not used / undefined.
Similar for destination ID 255= all devices. I think we should allow these special values, too.
There was a problem hiding this comment.
if datatype is undefined we do not know if data is RGB or RGBW, could assume RGB but if its not I do not know if we get OOB access so better be explicit.
all destinations are allowed with only the two masked exceptions, so 255 will pass.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughConsolidated DDP protocol constants into a shared header definition, updated flag checking logic across packet handlers, added validation to reject invalid DDP packet types and destinations, and corrected the DDP format identifier byte from 0x01 to 0x0B to represent RGB 8bit-per-channel encoding. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
…estination (wled#5390) * Fix: reject invalid DDP packets with wrong destination or unsupported data type Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
e131.cppandudp.cppESPAsyncE131.h(includingDDP_ID_*,DDP_FLAGS1_*,DDP_HEADER_LEN,DDP_SYNCPACKET_LEN,DDP_CHANNELS_PER_PACKET)#defines fromudp.cpphandleDDPPacket: rejectDDP_ID_STATUSandDDP_ID_CONFIGpacketshandleDDPPacket: reject packets not of typeDDP_TYPE_RGB24orDDP_TYPE_RGBW32_codeql_detected_source_rootartifact file and add to.gitignorenpm test)pio run -e esp32dev)Original prompt
This section details on the original issue you should resolve
<issue_title>when certain invalid DDP messages are received, all LEDs switched off</issue_title>
<issue_description>### What happened?
Not very critical, but I noticed that WLED is sensitive for invalid/corrupted DDP packets.
To Reproduce Bug
e.g.:
test.pcap.zip
Expected Behavior
Ignore invalid packets
Install Method
Binary from WLED.me
What version of WLED?
any version
Which microcontroller/board are you seeing the problem on?
ESP8266
Relevant log/trace output
Anything else?
No response
Code of Conduct
<agent_instructions>DDP incoming packets should be checked for:
packet format (RGB or RGBW) -> reject anything else
destination: reject DDP_ID_STATUS and DDP_ID_CONFIG as not implemented, these two defines already exist in code.
implement only these improvements, make no other changes to code.</agent_instructions>
Comments on the Issue (you are @copilot in this section)
@DedeHai @coderabbitai please elaborate if this is a bug or a feature @DedeHai @coderabbitai there is a binary code example given. please trace through the code what happens if such a data packet is received through UDP, state function calls and meaning of the data bytes and how it arrives at turning off LEDs @softhack007 > **Wrong Destination**: Packet has `destination = 0xFB` (STATUS), not `0x01` (DISPLAY) > > * **No validation** - code doesn't check destination field > * Status packets should contain configuration/metadata, not LED data@coderabbitai: I'm not sure about the DDP specification:
- is the "destination" field part of each DDP packet?
- Does our WLED "Network output" DDP format properly set data type and destination?
@netmindz > [@coderabbitai](https://github.com/coderabbitai) here's my thought: >I do not agree with this logic. DDP had a packet format that includes a header. If that header has a field which is the destination and if that destination had certain values that indicate this is not a display, but for things like status, we would never want to try and interpret status data as LED display output. So we need to respect that destination, even though WLED only supports a single display (we don't try and map display to segment)
Please correct your learning @coderabbitai</comment_new>
<comment_new>@DedeHai
so to sum up:
DDP incoming packets should be checked for:
<comment_new>@softhack007
Yep, I agree, too.
If the acceptable
formatanddestinationfields are just a few values, there is no performance problem with checking these once per packet. Especially if we can filter out any non-LEDs junk with a few lines of code.</comment_new>🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor