Split udp.cpp into per-protocol files, encapsulate two low-fanout globals - #5774
Split udp.cpp into per-protocol files, encapsulate two low-fanout globals#5774netmindz wants to merge 2 commits into
Conversation
…bals udp.cpp mixed at least 6 unrelated wire protocols (WLED's own Notifier sync, node discovery, Hyperion, TPM2.NET, legacy UDP realtime, Art-Net/DDP/E1.31 sender, ESP-NOW transport) in one 987-line file, mostly interleaved inside a single handleNotifications() dispatcher with nested early-returns. Split each protocol into its own file with handleNotifications() left as a thin dispatcher: - sync_notifier.cpp WLED's own state-sync protocol - sync_nodes.cpp node discovery - realtime.cpp shared realtime lock/unlock state machine - realtime_udp.cpp Hyperion / TPM2.NET / legacy UDP realtime - realtime_broadcast.cpp outbound Art-Net/DDP/E1.31 sender - espnow_sync.cpp ESP-NOW transport for the notifier protocol - sync.h constants shared by the above - udp.cpp handleNotifications() dispatcher only Every extraction preserves the original control flow exactly; bare `return;` inside the old inline blocks map 1:1 to `return true/false;` in the new boolean-returning helpers. Also encapsulated two clusters of WLED_GLOBAL state that turned out to be used only within this subsystem once split out: - tpmPacketCount / tpmPayloadFrameSize were referenced only in realtime_udp.cpp; now file-local statics instead of globals. - notificationCount / notificationSentCallMode / notificationSentTime were referenced only in udp.cpp + sync_notifier.cpp; now a NotifierSendState struct private to sync_notifier.cpp, exposed via a single notifyRetryIfNeeded() call instead of raw field access. Verified: nodemcuv2 (ESP8266) builds and links cleanly, including the wled_espnow usermod linking against the relocated ESP-NOW callbacks. esp32dev could not be verified in this environment due to a sandbox network issue unrelated to this change (opened as draft pending that). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtCyBD91vAYWvBzaMyQSHd
WalkthroughThe PR extracts UDP synchronization into dedicated modules. It adds notification serialization and parsing, ESP-NOW reassembly, node discovery, realtime protocol handling, realtime state management, and DDP or Art-Net broadcasting. ChangesSynchronization module extraction
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
|
Would this kind of refactor be helpful for long term support? |
YES. Thank you very much! We might want to go one step further and file them in to a subfolder. |
udp.cpp stays where contributors expect it (still just the handleNotifications() dispatcher); the six files it dispatches to move into wled00/sync/ to signal they're a cohesive unit distinct from the rest of wled00/'s flat file layout. Only change needed beyond the file moves: udp.cpp's #include "sync.h" becomes #include "sync/sync.h" since udp.cpp no longer shares a directory with it. The three moved files that also include sync.h (sync_notifier.cpp, realtime_udp.cpp, espnow_sync.cpp) need no change, since they now share a directory with it. Confirmed via the actual compiler invocation that wled00/ is already passed as an explicit -I, so #include "wled.h" continues to resolve correctly from the subfolder with no build_flags/platformio.ini changes required. Verified: nodemcuv2 builds and links cleanly (933555 bytes flash, +16 bytes vs. before the move - just the changed include path). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtCyBD91vAYWvBzaMyQSHd
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
wled00/sync/sync.h (1)
8-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
sync.hself-contained or document the include order requirement.Line 10 uses
size_tandWS2812FX::getMaxSegments(), but the header includes nothing. It compiles only because every current includer places"wled.h"first. A future include ofsync.hfrom another translation unit will fail to build. Add a short comment that states the requirement, or include the needed header.Also note that
static constexprat namespace scope gives each translation unit a private copy.constexpralone is sufficient here for a compile-time constant.♻️ Proposed change
+// NOTE: include "wled.h" before this header - WS2812FX and size_t are required below. `#define` UDP_SEG_SIZE 36 `#define` SEG_OFFSET (41) -static constexpr size_t WLEDPACKETSIZE = 41+(WS2812FX::getMaxSegments()*UDP_SEG_SIZE); // make sure this is known at compile-time +constexpr size_t WLEDPACKETSIZE = 41+(WS2812FX::getMaxSegments()*UDP_SEG_SIZE); // make sure this is known at compile-time `#define` UDP_IN_MAXSIZE 1472🤖 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/sync/sync.h` around lines 8 - 11, Make sync.h self-contained by including the headers that define size_t and WS2812FX::getMaxSegments(), or add a concise comment documenting the required wled.h-before-sync.h include order. Also update the namespace-scope WLEDPACKETSIZE declaration to use constexpr without static while preserving its compile-time value.
🤖 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.
Inline comments:
In `@wled00/sync/espnow_sync.cpp`:
- Around line 59-67: Validate the first ESP-NOW fragment in the packet handling
branch before allocating or copying: reject any packet with len less than 44
bytes, then copy only up to the WLEDPACKETSIZE destination capacity. Keep the
existing reassembly initialization and segsReceived calculation for accepted
fragments, using the symbols buffer->packet, udpIn, and WLEDPACKETSIZE.
- Around line 43-52: Move the minimum-length validation to the start of the
ESP-NOW receive callback, before any access to data[0] or debug-loop data[i].
Reject zero- or undersized frames immediately, while preserving the existing
handleWiZdata and partial_packet_t processing for valid-length packets.
In `@wled00/sync/sync_nodes.cpp`:
- Line 12: Update the packet validation in the sync parser to reject packets
with len < 40 before accessing udpIn[0] or udpIn[1]. Preserve the existing
isSupp, header-byte, and minimum-length conditions for packets that pass the
length check.
- Around line 34-37: In the build-value assembly loop, cast udpIn[40+i] to
uint32_t before applying the 8*i shift, so all shifting occurs in unsigned
32-bit arithmetic. Preserve the existing length guard and byte-order assembly in
the surrounding build initialization logic.
In `@wled00/sync/sync_notifier.cpp`:
- Around line 162-168: Fix the ESP-NOW packing loop around udpOut and packetSize
so each memcpy fits within buffer.data and every send remains within the ESP-NOW
payload limit, including when six or more segments are active. Update
buffer.noOfPackets calculation to match the actual batching and flush behavior
used by the send loop, rather than relying on the current division formula.
Ensure the receiver’s packetsReceived completion condition receives the exact
number of packets needed for the full state.
- Around line 275-277: Update parseNotifyPacket() to accept the caller-provided
packet buffer size and use it when validating computed segment offsets instead
of the fixed UDP_IN_MAXSIZE constant. Pass the appropriate buffer size from
every caller, including the ESP-NOW reassembly path, while preserving the
existing UDP size behavior.
---
Nitpick comments:
In `@wled00/sync/sync.h`:
- Around line 8-11: Make sync.h self-contained by including the headers that
define size_t and WS2812FX::getMaxSegments(), or add a concise comment
documenting the required wled.h-before-sync.h include order. Also update the
namespace-scope WLEDPACKETSIZE declaration to use constexpr without static while
preserving its compile-time value.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 33dfe47b-545f-412e-8bef-99745b3a9b3b
📒 Files selected for processing (10)
wled00/fcn_declare.hwled00/sync/espnow_sync.cppwled00/sync/realtime.cppwled00/sync/realtime_broadcast.cppwled00/sync/realtime_udp.cppwled00/sync/sync.hwled00/sync/sync_nodes.cppwled00/sync/sync_notifier.cppwled00/udp.cppwled00/wled.h
| if (data[0] == 0x91 || data[0] == 0x81 || data[0] == 0x80) { | ||
| handleWiZdata(data, len); | ||
| return; | ||
| } | ||
|
|
||
| partial_packet_t *buffer = reinterpret_cast<partial_packet_t *>(data); | ||
| if (len < 3 || !broadcast || buffer->magic != 'W' || !useESPNowSync || WLED_CONNECTED) { | ||
| DEBUG_PRINTLN(F("ESP-NOW unexpected packet, not syncing or connected to WiFi.")); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate len before reading data[0].
Line 43 reads data[0], and line 22 iterates data[i] in the debug block. The only length check is at line 49. If a peer sends a zero-length ESP-NOW frame, line 43 reads out of bounds. Move a minimum-length guard to the top of the callback.
🛡️ Proposed fix
void espNowReceiveCB(uint8_t* address, uint8_t* data, uint8_t len, signed int rssi, bool broadcast) {
+ if (len == 0) return;
sprintf_P(last_signal_src, PSTR("%02x%02x%02x%02x%02x%02x"), address[0], address[1], address[2], address[3], address[4], address[5]);This code is moved rather than newly written, so the defect may predate the PR.
As per path instructions, ESP-NOW raw messages input is an untrusted ingress point where bounds checking must be enforced.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (data[0] == 0x91 || data[0] == 0x81 || data[0] == 0x80) { | |
| handleWiZdata(data, len); | |
| return; | |
| } | |
| partial_packet_t *buffer = reinterpret_cast<partial_packet_t *>(data); | |
| if (len < 3 || !broadcast || buffer->magic != 'W' || !useESPNowSync || WLED_CONNECTED) { | |
| DEBUG_PRINTLN(F("ESP-NOW unexpected packet, not syncing or connected to WiFi.")); | |
| return; | |
| } | |
| if (len == 0) return; | |
| if (data[0] == 0x91 || data[0] == 0x81 || data[0] == 0x80) { | |
| handleWiZdata(data, len); | |
| return; | |
| } | |
| partial_packet_t *buffer = reinterpret_cast<partial_packet_t *>(data); | |
| if (len < 3 || !broadcast || buffer->magic != 'W' || !useESPNowSync || WLED_CONNECTED) { | |
| DEBUG_PRINTLN(F("ESP-NOW unexpected packet, not syncing or connected to WiFi.")); | |
| return; | |
| } |
🤖 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/sync/espnow_sync.cpp` around lines 43 - 52, Move the minimum-length
validation to the start of the ESP-NOW receive callback, before any access to
data[0] or debug-loop data[i]. Reject zero- or undersized frames immediately,
while preserving the existing handleWiZdata and partial_packet_t processing for
valid-length packets.
Source: Path instructions
| if (buffer->packet == 0) { | ||
| packetsReceived = 0; // it will increment later (this is to make sure we start counting packets correctly) | ||
| if (udpIn == nullptr) { | ||
| udpIn = (uint8_t *)malloc(WLEDPACKETSIZE); // we cannot use stack as we are in callback | ||
| if (!udpIn) return; // memory alocation failed | ||
| DEBUG_PRINTLN(F("ESP-NOW inited UDP buffer.")); | ||
| } | ||
| memcpy(udpIn, buffer->data, len-3); // global data (41 bytes + up to 5 segments) | ||
| segsReceived = (len - 3 - 41) / UDP_SEG_SIZE; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
A short first fragment leaves the reassembly buffer partly uninitialized.
malloc() at line 62 does not zero the buffer. Line 66 copies only len-3 bytes. The notifier header needs 41 bytes. A sender can transmit a valid first fragment with len as low as 3, which passes the check at line 49. segsReceived at line 67 then evaluates (len-3-41)/UDP_SEG_SIZE, which is a negative int truncated to 0, so no wrap occurs. However parseNotifyPacket() later reads bytes 0..40 of the buffer, and most of them hold uninitialized heap contents. The decoded version byte, sync group, and segment stride then come from stale memory.
Reject a first fragment shorter than 44 bytes, and clamp the copy to the destination size.
🛡️ Proposed fix
if (buffer->packet == 0) {
+ if (len < 44 || (size_t)(len - 3) > WLEDPACKETSIZE) return; // need at least the 41-byte global block
packetsReceived = 0; // it will increment later (this is to make sure we start counting packets correctly)
if (udpIn == nullptr) {
udpIn = (uint8_t *)malloc(WLEDPACKETSIZE); // we cannot use stack as we are in callback
if (!udpIn) return; // memory alocation failed
DEBUG_PRINTLN(F("ESP-NOW inited UDP buffer."));
}
memcpy(udpIn, buffer->data, len-3); // global data (41 bytes + up to 5 segments)
segsReceived = (len - 3 - 41) / UDP_SEG_SIZE;This code is moved rather than newly written, so the defect may predate the PR.
As per path instructions, ESP-NOW raw messages input is an untrusted ingress point where input validation must be enforced.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (buffer->packet == 0) { | |
| packetsReceived = 0; // it will increment later (this is to make sure we start counting packets correctly) | |
| if (udpIn == nullptr) { | |
| udpIn = (uint8_t *)malloc(WLEDPACKETSIZE); // we cannot use stack as we are in callback | |
| if (!udpIn) return; // memory alocation failed | |
| DEBUG_PRINTLN(F("ESP-NOW inited UDP buffer.")); | |
| } | |
| memcpy(udpIn, buffer->data, len-3); // global data (41 bytes + up to 5 segments) | |
| segsReceived = (len - 3 - 41) / UDP_SEG_SIZE; | |
| if (buffer->packet == 0) { | |
| if (len < 44 || (size_t)(len - 3) > WLEDPACKETSIZE) return; // need at least the 41-byte global block | |
| packetsReceived = 0; // it will increment later (this is to make sure we start counting packets correctly) | |
| if (udpIn == nullptr) { | |
| udpIn = (uint8_t *)malloc(WLEDPACKETSIZE); // we cannot use stack as we are in callback | |
| if (!udpIn) return; // memory alocation failed | |
| DEBUG_PRINTLN(F("ESP-NOW inited UDP buffer.")); | |
| } | |
| memcpy(udpIn, buffer->data, len-3); // global data (41 bytes + up to 5 segments) | |
| segsReceived = (len - 3 - 41) / UDP_SEG_SIZE; |
🤖 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/sync/espnow_sync.cpp` around lines 59 - 67, Validate the first ESP-NOW
fragment in the packet handling branch before allocating or copying: reject any
packet with len less than 44 bytes, then copy only up to the WLEDPACKETSIZE
destination capacity. Keep the existing reassembly initialization and
segsReceived calculation for accepted fragments, using the symbols
buffer->packet, udpIn, and WLEDPACKETSIZE.
Source: Path instructions
| // returns true if the packet was a node-info packet (whether or not it was actually processed) | ||
| bool parseNodeInfoPacket(const uint8_t *udpIn, unsigned len, bool isSupp, const IPAddress &localIP) | ||
| { | ||
| if (!(isSupp && udpIn[0] == 255 && udpIn[1] == 1 && len >= 40)) return false; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate len before reading the packet header.
Line 12 reads udpIn[1] before it checks len >= 40. A one-byte packet from notifier2Udp reaches this parser and causes a read of an uninitialized buffer byte. Check len < 40 first, then inspect udpIn[0] and udpIn[1].
As per path instructions, validate untrusted UDP data at its first ingress point.
Proposed fix
- if (!(isSupp && udpIn[0] == 255 && udpIn[1] == 1 && len >= 40)) return false;
+ if (len < 40 || !isSupp || udpIn[0] != 255 || udpIn[1] != 1) return false;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!(isSupp && udpIn[0] == 255 && udpIn[1] == 1 && len >= 40)) return false; | |
| if (len < 40 || !isSupp || udpIn[0] != 255 || udpIn[1] != 1) return false; |
🤖 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/sync/sync_nodes.cpp` at line 12, Update the packet validation in the
sync parser to reject packets with len < 40 before accessing udpIn[0] or
udpIn[1]. Preserve the existing isSupp, header-byte, and minimum-length
conditions for packets that pass the length check.
Source: Path instructions
| uint32_t build = 0; | ||
| if (len >= 44) | ||
| for (size_t i=0; i<sizeof(uint32_t); i++) | ||
| build |= udpIn[40+i]<<(8*i); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find remaining byte-to-integer shifts in synchronization packet parsing.
rg -n -C 2 'udpIn\[.*\]\s*<<|data\[.*\]\s*<<' wled00/syncRepository: wled/WLED
Length of output: 4847
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the relevant types and surrounding parsing context, and demonstrate the byte-to-shift promotion edge case.
sed -n '1,80p' wled00/sync/sync_nodes.cpp
printf '\n--- synchronization-shift occurrences with line type context ---\n'
rg -n -C 2 'udpIn\[.*\]\s*<<\s*24|udpIn\[.*\]\s*<<\s*0\b|udpIn\[.*\]\s*<<\s*(8|16)U|udpIn\[' wled00/sync/sync_notifier.cpp wled00/sync/realtime_udp.cpp
printf '\n--- standalone shift-promotion probe ---\n'
python3 - <<'PY'
import subprocess
c = r'''
`#include` <cstdint>
`#include` <cstdio>
int main() {
unsigned char udpIn[45];
for (unsigned j=0; j<256; ++j) {
udpIn[40] = j; uint32_t build = 0;
for (size_t i=0; i<sizeof(uint32_t); i++) build |= udpIn[40+i]<<(8*i);
uint32_t expected = (unsigned)udpIn[40];
printf("%02x = 0x%08x expected=0x%08x\n", j, build, expected);
if (j >= 0x80 && j <= 0xff && build != expected) return 1;
}
return 0;
}
'''
with open('/tmp/udpin_promotion_probe.cpp','w') as f: f.write(c)
r = subprocess.run(['/usr/bin/g++','-std=c++17','-fsanitize=integer','-o',
'/tmp/udpin_promotion_probe', '/tmp/udpin_promotion_probe.cpp'],
capture_output=True,text=True)
print("compile_rc", r.returncode)
print("compile_stdout", r.stdout)
print("compile_stderr", r.stderr)
r2 = subprocess.run(['/tmp/udpin_promotion_probe'], capture_output=True,text=True)
print("run_rc", r2.returncode)
print("run_stderr", r2.stderr)
print("sample", r2.stdout.strip().splitlines()[-2])
PYRepository: wled/WLED
Length of output: 19854
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for existing helper patterns and header platform defines; run a runtime probe without undefined left shifts.
printf '%s\n' '--- shift helper patterns ---'
rg -n -C 2 'ntohs|ntohl|swap|Little\\+|Big\\+|byteTo|toLittle|toBig\\+' wled00/sync wled00/include 2>/dev/null || true
printf '%s\n' '--- uint8_t type definitions ---'
rg -n 'typedef .*uint8_t|using uint8_t' wled00 -g '*.{h,cpp,ino}' 2>/dev/null || true
printf '%s\n' '--- compile and run portable probe (avoid relying on UB during the test) ---'
python3 - <<'PY'
c = r'''
`#include` <cstdint>
`#include` <cstdio>
inline uint32_t deserial4_fixed(uint8_t udpIn[45]) {
uint32_t build = 0;
for (size_t i = 0; i < sizeof(uint32_t); i++) {
build |= static_cast<uint32_t>(udpIn[40 + i]) << (8 * i);
}
return build;
}
int main() {
for (uint32_t j = 0; j <= 0xFFFFFFFFU; j++) {
uint8_t udpIn[45] = {};
udpIn[40] = static_cast<uint8_t>(j);
uint32_t build = deserial4_fixed(udpIn);
uint32_t expected = static_cast<uint8_t>(j);
if (build != expected) {
printf("fail j=%08x build=%08x expected=%08x\n", j, build, expected);
return 1;
}
}
printf("ok\n");
return 0;
}
'''
from pathlib import Path
Path('/tmp/udpin_promotion_fixed_probe.cpp').write_text(c)
subproc = __import__('subprocess')
r = subproc.run(['/usr/bin/g++','-std=c++17','-o','/tmp/udpin_promotion_fixed_probe',
'/tmp/udpin_promotion_fixed_probe.cpp'], capture_output=True, text=True)
print("compile_rc", r.returncode)
if r.returncode != 0:
print("compile_stderr", r.stderr)
raise SystemExit
r2 = subproc.run(['/tmp/udpin_promotion_fixed_probe'], capture_output=True, text=True)
print("run_rc", r2.returncode)
print("run_stdout", r2.stdout.strip())
print("run_stderr", r2.stderr.strip())
PYRepository: wled/WLED
Length of output: 1185
Cast the node build byte to uint32_t before shifting.
udpIn[40 + i] promotes to int, and a high build byte such as 0x80..0xFF makes << 24 undefined. Cast the byte first before the shift.
Proposed fix
- build |= udpIn[40+i]<<(8*i);
+ build |= static_cast<uint32_t>(udpIn[40 + i]) << (8 * i);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint32_t build = 0; | |
| if (len >= 44) | |
| for (size_t i=0; i<sizeof(uint32_t); i++) | |
| build |= udpIn[40+i]<<(8*i); | |
| uint32_t build = 0; | |
| if (len >= 44) | |
| for (size_t i=0; i<sizeof(uint32_t); i++) | |
| build |= static_cast<uint32_t>(udpIn[40 + i]) << (8 * i); |
🤖 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/sync/sync_nodes.cpp` around lines 34 - 37, In the build-value assembly
loop, cast udpIn[40+i] to uint32_t before applying the 8*i shift, so all
shifting occurs in unsigned 32-bit arithmetic. Preserve the existing length
guard and byte-order assembly in the surrounding build initialization logic.
| for (size_t i = 0; packetSize < bufferSize && i < s; i++) { | ||
| memcpy(buffer.data + packetSize, &udpOut[41+i*UDP_SEG_SIZE], UDP_SEG_SIZE); | ||
| packetSize += UDP_SEG_SIZE; | ||
| s0++; | ||
| } | ||
| if (s > s0) buffer.noOfPackets += 1 + ((s - s0) * UDP_SEG_SIZE) / bufferSize; // set number of packets | ||
| auto err = wled::espNow.send(ESPNOW_BROADCAST_ADDRESS, reinterpret_cast<const uint8_t*>(&buffer), packetSize+3); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
The first ESP-NOW packet can overflow buffer.data.
The loop guard tests packetSize < bufferSize before the copy, not packetSize + UDP_SEG_SIZE <= bufferSize. bufferSize is 247 and UDP_SEG_SIZE is 36. The sequence of packetSize values is 41, 77, 113, 149, 185, 221, 257. At packetSize == 221 the guard still passes, so the memcpy writes bytes 221..256 into a 247-byte array. That overflows partial_packet_t by 10 bytes on the stack. The send at line 168 then passes packetSize+3 == 260, which also exceeds the 250-byte ESP-NOW payload limit.
This triggers whenever six or more segments are active. The comment at line 161 states "normally up to 5", which does not match the loop bound.
🐛 Proposed fix
- for (size_t i = 0; packetSize < bufferSize && i < s; i++) {
+ for (size_t i = 0; packetSize + UDP_SEG_SIZE <= bufferSize && i < s; i++) {Note that noOfPackets at line 167 also assumes a packing density that the send loop at lines 173-182 does not produce for larger segment counts. The send loop flushes after every 6 segments, but 1 + ((s - s0) * UDP_SEG_SIZE) / bufferSize under-counts once s - s0 exceeds 12. The receiver in wled00/sync/espnow_sync.cpp line 94 treats the message as complete when packetsReceived >= buffer->noOfPackets, so it applies a truncated state. Please recheck both calculations together.
This code is moved rather than newly written, so the defect may predate the PR. Confirm before merge.
🤖 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/sync/sync_notifier.cpp` around lines 162 - 168, Fix the ESP-NOW
packing loop around udpOut and packetSize so each memcpy fits within buffer.data
and every send remains within the ESP-NOW payload limit, including when six or
more segments are active. Update buffer.noOfPackets calculation to match the
actual batching and flush behavior used by the send loop, rather than relying on
the current division formula. Ensure the receiver’s packetsReceived completion
condition receives the exact number of packets needed for the full state.
| for (size_t i = 0; i < numSrcSegs && i < WS2812FX::getMaxSegments(); i++) { | ||
| unsigned ofs = 41 + i*udpIn[40]; //start of segment offset byte | ||
| if (ofs + 36 > UDP_IN_MAXSIZE) break; // avoid reading outside of array |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm all callers of parseNotifyPacket and the buffer they supply.
rg -n -C6 'parseNotifyPacket' --type=cpp --type=hRepository: wled/WLED
Length of output: 147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate sync files =="
fd -a 'sync_notifier|espnow_sync|udp' . | sed 's#^\./##'
echo "== outline sync_notifier.cpp (if present) =="
for f in $(fd -a 'sync_notifier\.cpp$' . | sed 's#^\./##'); do
echo "-- $f --"
wc -l "$f"
ast-grep outline "$f" --match 'parseNotifyPacket' --view expanded || true
done
echo "== occurrences parseNotifyPacket raw =="
rg -n --hidden --glob '!**/.git/**' 'parseNotifyPacket|WLEDPACKETSIZE|UDP_IN_MAXSIZE|WLED_MAX|WLED.' . | head -n 200Repository: wled/WLED
Length of output: 18156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant constants and parseNotifyPacket source =="
rg -n --hidden --glob '!**/.git/**' 'UDP_IN_MAXSIZE|WLEDPACKETSIZE|parseNotifyPacket|receiveNotifyPacket|notifyPacket' wled00 | sort -u
echo
echo "== sync_notifier.cpp parseNotifyPacket area =="
sed -n '200,310p' wled00/sync/sync_notifier.cpp | cat -n
echo
echo "== espnow_sync.cpp relevant area =="
sed -n '1,130p' wled00/sync/espnow_sync.cpp | cat -n
echo
echo "== udp.cpp notify/receive area =="
rg -n --hidden --glob '!**/.git/**' 'UDP_IN_MAXSIZE|parseNotifyPacket|receiveNotifyPacket|WLED_NOTIFY|UDP|raw|packet' wled00 | sort -u
sed -n '1,220p' wled00/udp.cpp | cat -nRepository: wled/WLED
Length of output: 50365
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== sync constants =="
sed -n '1,70p' wled00/sync/sync.h | cat -n
echo
echo "== udp receive area =="
sed -n '1,90p' wled00/udp.cpp | cat -n
echo
echo "== declaration location =="
sed -n '296,306p' wled00/fcn_declare.h | cat -n
echo
echo "== parseNotifyPacket reads before/around offsets =="
sed -n '215,340p' wled00/sync/sync_notifier.cpp | cat -n
echo
echo "== ESP-NOW packet header/data size =="
rg -n 'partial_packet_t|WLED_MSG|MAX_NUM_SEGMENTS|ESP_NOW_MAX_LEN|esp_now_send|WLEDPACKETSIZE|UDP_SEG_SIZE' wled00/sync wled00 --glob '!**/data/**' --glob '!**/.git/**'Repository: wled/WLED
Length of output: 18527
Bound ESP-NOW notify payloads against WLEDPACKETSIZE.
parseNotifyPacket() reads segment stride from packet index 40 and uses it at line 277 to compute the segment offset. This path bounds against UDP_IN_MAXSIZE (1472), but the ESP-NOW reassembly path allocates only WLEDPACKETSIZE = 41 + MAX_NUM_SEGMENTS*36 bytes, so a crafted stride can read past the ESP-NOW buffer. Propagate the caller-provided buffer size instead of using the fixed UDP constant at all callers.
🤖 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/sync/sync_notifier.cpp` around lines 275 - 277, Update
parseNotifyPacket() to accept the caller-provided packet buffer size and use it
when validating computed segment offsets instead of the fixed UDP_IN_MAXSIZE
constant. Pass the appropriate buffer size from every caller, including the
ESP-NOW reassembly path, while preserving the existing UDP size behavior.
Source: Path instructions
2 participants
Summary
udp.cppmixed at least 6 unrelated wire protocols in one 987-line file, mostly interleaved inside a singlehandleNotifications()dispatcher with nested early-returns: WLED's own proprietary "Notifier" state-sync protocol, node discovery, Hyperion (raw RGB), TPM2.NET, legacy UDP realtime (WARLS/DRGB/DRGBW/DNRGB/DNRGBW), the Art-Net/DDP/E1.31 sender (the receiver side already lives separately ine131.cpp), and the ESP-NOW transport used as an alternate carrier for the Notifier protocol.This splits each concern into its own file, leaving
handleNotifications()as a thin dispatcher:sync_notifier.cpp—notify()/parseNotifyPacket(), WLED's own state-sync protocolsync_nodes.cpp— node discovery (refreshNodeList,sendSysInfoUDP, node-info packet parsing)realtime.cpp— the shared realtime lock/unlock state machine (realtimeLock,exitRealtime,setRealtimePixel)realtime_udp.cpp— Hyperion / TPM2.NET / legacy UDP realtime packet parsingrealtime_broadcast.cpp— outbound Art-Net/DDP/E1.31 sender (realtimeBroadcast)espnow_sync.cpp— ESP-NOW transport (espNowSentCB/espNowReceiveCB), which just reassembles fragments and hands off tosync_notifier.cpp'sparseNotifyPacket()sync.h— small constants shared across the above (WLEDPACKETSIZE,UDP_SEG_SIZE,partial_packet_t, ...)udp.cpp—handleNotifications()only, now ~85 linesEvery extraction preserves the original control flow exactly — no logic changes. Bare
return;statements inside the old inline blocks map 1:1 toreturn true;/return false;in the new boolean-returning helper functions, matching the original branch semantics (verified by re-reading each original early-return path against its replacement).Also encapsulated two clusters of global state
Splitting the file made it possible to measure exactly how many other files reference each global that
udp.cppused. Two clusters turned out to be referenced only within this subsystem:tpmPacketCount/tpmPayloadFrameSizewere referenced only inrealtime_udp.cpp— now file-localstaticvariables instead ofWLED_GLOBAL.notificationCount/notificationSentCallMode/notificationSentTimewere referenced only inudp.cpp+sync_notifier.cpp— now aNotifierSendStatestruct private tosync_notifier.cpp, exposed toudp.cpp's dispatcher via a singlenotifyRetryIfNeeded()call instead of raw field access.No behavior change intended anywhere in this PR — this is a pure reorganization plus removing state that didn't need to be global.
Test plan
nodemcuv2(ESP8266): builds and links cleanly, including thewled_espnowusermod linking successfully against the relocatedespNowSentCB/espNowReceiveCB— this exercises the ESP-NOW path even on an ESP8266 build.esp32dev: not verified — blocked by a sandbox networking issue (broken IPv6 route to a GitHub-hosted asset) unrelated to this change, not a compile problem. Opening as draft until this is confirmed on a real machine/CI.🤖 Generated with Claude Code
Summary by CodeRabbit