fix(storage): correctness bugs in ModelMap::writeModelLabels - #7713
Merged
Conversation
Three issues found while reviewing #7709's raw-file label rewrite: - Header search/copy bounds used sizeof(buf) instead of the actual bytes read, so a model file shorter than 512 bytes had uninitialized stack memory written into the rewritten file. - writeModelLabels() returns true on success, but both callers (renameLabel, updateModelFile) assigned it directly to a variable named 'fault', inverting the polarity - a successful rename reported failure and vice versa. Currently latent since both call sites discard the return value, but wrong as written. - f_write() and f_rename() results weren't checked for short writes/failure, so a full or removed SD card during a rename could silently delete the original file and leave a truncated replacement. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
WriteModelLabelsPreservesBodySize pins the uninitialized-memory fix: two model files under 512 bytes differing only in body length must produce outputs whose size differs by exactly that same amount. Verified this fails against the pre-fix code (both outputs collapsed to the same size, padded with garbage instead of bounded by the real byte count) and passes against the fix. WriteModelLabelsFailsCleanlyWithoutHeader covers the missing-header error path. Requires SIMU + STORAGE_MODELSLIST (a color-target native build, e.g. PCB=X10) - ModelMap/writeModelLabels don't exist on B&W targets. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
pfeerick
added
bug/regression ↩️
pfeerick
marked this pull request as ready for review
August 26, 2026 23:09
pfeerick
added a commit
that referenced
this pull request
Aug 26, 2026
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit 3562502)
pfeerick
added a commit
that referenced
this pull request
Aug 28, 2026
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit 3562502)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Found while independently reviewing the raw-file label rewrite added by #7709 (
ModelMap::writeModelLabels()), which reads a model's existing.yml, rewrites itsheader:section, and raw-copies the rest of the file byte-for-byte into a temp file before swapping it in.Three issues in that copy path:
sizeof(buf)(512) instead of the number of bytes actually read. For any model file shorter than 512 bytes, the remainder of the stack buffer is uninitialized, and that garbage got written into the rewritten model file.writeModelLabels()returnstrueon success (per its own docstring), but both callers (renameLabel,updateModelFile) assign the result directly to a variable namedfault, whose convention elsewhere in those same functions istrue= failure. A successful rename would report failure and vice versa. Currently latent since both real call sites discard the return value, but wrong as written and a landmine for the next caller that checks it.f_write()'s actual bytes-written count norf_rename()'s result were checked, so a full or removed SD card mid-rename could silently delete the original file and leave a truncated replacement in its place.Test plan
Added two tests to
radio/src/tests/modelslist.cpp(ModelsList.WriteModelLabelsPreservesBodySize,ModelsList.WriteModelLabelsFailsCleanlyWithoutHeader), exercisingwriteModelLabels()against a real (simulated) FatFS filesystem. Confirmed the first test fails against the pre-fix code (both outputs collapse to the same size regardless of real body-length difference) and passes against the fix.gtests-radiopasses on aSTORAGE_MODELSLISTtarget (PCB=X10) - 110/110, including the 2 new testsgtests-radiopasses on a B&W target (PCB=X9D+) - 122/122 (new tests compile out there, sinceModelMapdoesn't exist on B&W)🤖 Generated with Claude Code