Fix ledmap parser reading past end of map array#5693
Conversation
deserializeMap() loops while customMappingSize < getLengthTotal(), reading one comma-delimited token at a time. When ledmap.json has "map" before "width"/"height", and getLengthTotal() exceeds the map array's actual entry count (e.g. a trailing PWM/OnOff bus sits past the matrix), the loop continues past the array's closing ']' and atoi()s the next JSON key, silently storing 0 at the next slot of customMappingTable. The trailing strip's writes then route to physical pixel 0. The existing 'if (!foundDigit) break' handles the case where ']' is the only content in a chunk. This adds the symmetric case for when ']' follows the last valid entry in the same chunk: store the entry, then exit the loop.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughIn ChangesLED-map JSON parsing fix
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
added
the
AI
|
While the explanation ("why JSON key order matterrs") is full of wrong factual claims, the fix may still be reasonable. There is actually no relation to leading width/height fields (old MM format). Line 2159 Silently re-routing to pixel 0 is possibly related to how the ledmap array gets initialized - a customMappingsTable entry that's not present in the ledmap.json file would retain the previous value - initially 0. This will get solved with another PR |
|
@coderabbitai please verify if the problem described by the user is actually caused by reading past the end of array in ledmap.json. you can assume that the input is a sized-optimized JSON file without additional whitespace. The source code to check starts here Line 2154 in 3a46f44 Make sure you ignore the explanations provided in the PR description, as the explanations provided in the PR description are full of wrong assumptions. Also check if #5242 alone would fix the problem. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Read the deserializeMap parsing section from FX_fcn.cpp
sed -n '2140,2250p' wled00/FX_fcn.cppLength of output: 4107 🏁 Script executed: #!/bin/bash
# Get PR `#5242` details and diff
gh pr view 5242 --repo wled/WLED --json title,body,state,files
gh pr diff 5242 --repo wled/WLED 2>/dev/null | head -200Length of output: 1853
Bug verification: does the overread actually happen?Yes, it is real. Here's the precise mechanism for a compact (no-whitespace) JSON like The loop uses Without the
The Does PR
|
|
its a valid fix, definitely a bug but also only shows on "sloppy" incomplete ledmaps. |
3 participants
Summary
WS2812FX::deserializeMap()(wled00/FX_fcn.cpp:2160-2178) reads themaparray one comma-delimited token at a time, breaking out only whencustomMappingSize >= getLengthTotal(). WhengetLengthTotal()exceeds the actualmap[]entry count (e.g. a trailing PWM/OnOff bus placed after a 2D matrix), and the JSON has"map"before"width"/"height", the loop continues past the array's closing]and reads the next JSON key.atoi("\"width\":23")returns 0, which gets stored atcustomMappingTable[matrixSize]. The trailing strip's segment writes are then silently rerouted to physical pixel 0.The existing
if (!foundDigit) breakhandles the case where]appears alone in a chunk. This adds the symmetric case for when]follows the last valid digit in the same chunk — store the entry, then exit.How it works
endis already computed earlier asstrchr(number, ']'). If non-null, the chunk we just processed contained the array terminator, so there are no more entries to read regardless of whatgetLengthTotal()is.Reproduction (pre-fix)
mainand16_x)ledmap.jsondefining a 23×23 matrix with the WLED-MM key order:{"map":[...],"width":23,"height":23}[529, 530)Writes to segment 1 are routed to physical pixel 0 of the digital bus instead of the PWM bus. Pin 9 receives no PWM updates.
Why JSON key order matters
Commit 336e074 ("fix for 0byte size files, also made reading ledmaps more efficient", Nov 2025) changed WLED's UI to generate
"width"/"height"before"map". That implicitly dodges this bug because when the parser reads past the array, it hits the closing}—strchrfinds]-or-nothing patterns it already handles via the no-digit-found path. The original PR framed the change as a load-time optimization; it also turns out to be a quiet workaround. WLED-MM and pre-336e074bledmaps still trigger the bug.Testing
esp32c3devandesp32dev(pio run -e esp32c3dev,pio run -e esp32dev).npm testpasses.main, broken-order ledmap, trailing PWM bus → PWM dead, matrix pixel 0 driven by segment 1. ✗"width"/"height"come first (thestrchr(']')-with-no-digit path still handles them).Risk
One line added, scoped to the trailing-strip + map-first JSON ordering case. The new break only fires on chunks that already contained the array terminator, so well-formed JSON with width/height first is unaffected.
Summary by CodeRabbit