fix(radio): exFat directory handling#7484
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces all ChangesEliminate f_getcwd() Dependency
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 |
![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
🤖 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 `@radio/src/gui/colorlcd/libui/file_browser.cpp`:
- Around line 133-136: The FileBrowser constructor and the path change logic
(around line 256) both call f_chdir without checking its return value, which
means currentPath can be updated even when the directory change fails, causing a
mismatch between the tracked path and the actual FatFs working directory. Fix
this by checking the return value of f_chdir before updating currentPath - only
commit the new path to currentPath if f_chdir successfully returns (typically
returns 0 on success), otherwise handle the error appropriately by logging it or
throwing an exception to prevent silent failures that cause incorrect directory
targeting in subsequent operations.
In `@radio/src/gui/colorlcd/radio/radio_sdmanager.cpp`:
- Around line 473-474: The strncpy call copying to clipboard.data.sd.filename
does not guarantee null termination when the source string reaches the size
limit. After the strncpy statement that copies with CLIPBOARD_PATH_LEN - 1,
explicitly set the final byte of clipboard.data.sd.filename to the null
terminator character to ensure the string is always properly terminated,
preventing undefined behavior when the filename is later accessed or read.
In `@radio/src/gui/common/stdlcd/radio_sdmanager.cpp`:
- Around line 144-145: The strncpy call for clipboard.data.sd.filename does not
explicitly NUL-terminate the destination buffer, which can leave the string
unterminated when the source string is at or exceeds the buffer size. Add a line
immediately after the strncpy statement to explicitly NUL-terminate
clipboard.data.sd.filename at index CLIPBOARD_PATH_LEN-1 by setting it to '\0',
ensuring the copied filename is always properly terminated regardless of the
input length.
- Around line 67-79: In the sdManagerChdir function, replace the unbounded
strcat calls (lines 76-77) with bounded string operations that validate the
resulting path length does not exceed FF_MAX_LFN (255 bytes) before
concatenating. Additionally, check the return value of f_chdir on line 79 and
only update sdManagerPath if the call succeeds; if f_chdir fails, the path
should remain unchanged to keep internal tracking consistent with the
filesystem's actual current directory. Similarly, in the getSelectionFullPath
function, replace the unbounded strcpy and strcat operations with bounded
alternatives that check against buffer limits. Finally, after the strncpy call
at line 144 for clipboard.data.sd.filename, explicitly add NUL termination
(similar to how directory is terminated at line 143) to ensure the string is
properly null-terminated.
- Around line 82-87: In the getSelectionFullPath function, replace the unsafe
strcpy and strcat operations with snprintf to safely compose the file path with
bounds checking. Use snprintf with the appropriate buffer size limit for the lfn
parameter to ensure the composed path (sdManagerPath concatenated with "/" and
the selected filename) does not exceed the buffer capacity and prevent potential
buffer overflow.
In `@radio/src/lib_file.cpp`:
- Around line 94-95: The `sdReadDir()` function in lib_file.cpp now depends on
checking the filesystem's current working directory (dir->obj.fs->cdir) to
determine whether to inject the ".." entry, but this assumption is broken
because callers in filechoice.cpp line 132 and theme_manager.cpp line 324 open
directories without first synchronizing the CWD via f_chdir(). Either refactor
the ".." injection logic in sdReadDir() to check the opened directory's own path
state instead of the global filesystem CWD, or add f_chdir() calls in the
filechoice.cpp and theme_manager.cpp callers immediately before they invoke
sdReadDir() to ensure the CWD matches the directory being read.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2be12740-7fbf-48b1-ace0-6cfd22d68c01
📒 Files selected for processing (5)
radio/src/gui/colorlcd/libui/file_browser.cppradio/src/gui/colorlcd/libui/file_browser.hradio/src/gui/colorlcd/radio/radio_sdmanager.cppradio/src/gui/common/stdlcd/radio_sdmanager.cppradio/src/lib_file.cpp
| FileBrowser::FileBrowser(Window* parent, const rect_t& rect, const char* dir) : | ||
| TableField(parent, rect) | ||
| TableField(parent, rect), currentPath((dir && dir[0]) ? dir : "/") | ||
| { | ||
| f_chdir(dir); | ||
| f_chdir(currentPath.c_str()); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Handle f_chdir failures before committing tracked path changes.
Line 136 and Line 256 ignore f_chdir results. On failure, currentPath can drift from FatFs CWD, causing later callbacks/actions to target the wrong directory.
Suggested fix
FileBrowser::FileBrowser(Window* parent, const rect_t& rect, const char* dir) :
TableField(parent, rect), currentPath((dir && dir[0]) ? dir : "/")
{
- f_chdir(currentPath.c_str());
+ if (f_chdir(currentPath.c_str()) != FR_OK) {
+ currentPath = "/";
+ f_chdir(currentPath.c_str());
+ }
setAutoEdit(); void FileBrowser::onPress(const char* name, bool is_dir)
{
if (is_dir) {
+ std::string nextPath = currentPath;
if (strcmp(name, "..") == 0) {
// Go up: trim last segment (f_chdir("..") / f_getcwd are no-ops on exFAT).
- auto pos = currentPath.find_last_of('/');
- currentPath = (pos == 0 || pos == std::string::npos) ? "/"
- : currentPath.substr(0, pos);
+ auto pos = nextPath.find_last_of('/');
+ nextPath = (pos == 0 || pos == std::string::npos) ? "/"
+ : nextPath.substr(0, pos);
} else {
setFullPath(name);
- currentPath = fullPathBuf;
+ nextPath = fullPathBuf;
}
- f_chdir(currentPath.c_str());
+ if (f_chdir(nextPath.c_str()) != FR_OK) {
+ return;
+ }
+ currentPath = nextPath;
if (fileSelected) fileSelected(nullptr, nullptr, nullptr, is_dir);
selected = nullptr;
refresh();
return;
}Also applies to: 246-257
🤖 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 `@radio/src/gui/colorlcd/libui/file_browser.cpp` around lines 133 - 136, The
FileBrowser constructor and the path change logic (around line 256) both call
f_chdir without checking its return value, which means currentPath can be
updated even when the directory change fails, causing a mismatch between the
tracked path and the actual FatFs working directory. Fix this by checking the
return value of f_chdir before updating currentPath - only commit the new path
to currentPath if f_chdir successfully returns (typically returns 0 on success),
otherwise handle the error appropriately by logging it or throwing an exception
to prevent silent failures that cause incorrect directory targeting in
subsequent operations.
| strncpy(clipboard.data.sd.filename, name, CLIPBOARD_PATH_LEN - 1); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Ensure clipboard filename is always NUL-terminated.
At Line 473, strncpy(..., CLIPBOARD_PATH_LEN - 1) can leave clipboard.data.sd.filename unterminated for long names. That can break paste target logic and trigger undefined reads.
Suggested fix
strncpy(clipboard.data.sd.filename, name, CLIPBOARD_PATH_LEN - 1);
+ clipboard.data.sd.filename[CLIPBOARD_PATH_LEN - 1] = '\0';📝 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.
| strncpy(clipboard.data.sd.filename, name, CLIPBOARD_PATH_LEN - 1); | |
| }); | |
| strncpy(clipboard.data.sd.filename, name, CLIPBOARD_PATH_LEN - 1); | |
| clipboard.data.sd.filename[CLIPBOARD_PATH_LEN - 1] = '\0'; | |
| }); |
🤖 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 `@radio/src/gui/colorlcd/radio/radio_sdmanager.cpp` around lines 473 - 474, The
strncpy call copying to clipboard.data.sd.filename does not guarantee null
termination when the source string reaches the size limit. After the strncpy
statement that copies with CLIPBOARD_PATH_LEN - 1, explicitly set the final byte
of clipboard.data.sd.filename to the null terminator character to ensure the
string is always properly terminated, preventing undefined behavior when the
filename is later accessed or read.
| static void sdManagerChdir(const char* name) | ||
| { | ||
| if (!strcmp(name, "..")) { | ||
| // go up: drop the last path segment | ||
| char* sep = strrchr(sdManagerPath, '/'); | ||
| if (sep == sdManagerPath) sep[1] = '\0'; // parent is the root | ||
| else if (sep) *sep = '\0'; | ||
| } else { | ||
| // descend into 'name' | ||
| if (sdManagerPath[1] != '\0') strcat(sdManagerPath, "/"); | ||
| strcat(sdManagerPath, name); | ||
| } | ||
| f_chdir(sdManagerPath); // absolute path: resolves on FAT and exFAT alike |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify sizes and all C-string writes touching sdManagerPath / clipboard paths.
set -euo pipefail
fd -a "radio_sdmanager.cpp" radio/src | sort
echo "---- constants ----"
rg -n "FF_MAX_LFN|CLIPBOARD_PATH_LEN|sdManagerPath" radio/src/gui/common/stdlcd/radio_sdmanager.cpp radio/src/gui/colorlcd/radio/radio_sdmanager.cpp
echo "---- risky concatenations/copies ----"
rg -n "strcat\\(|strcpy\\(|strncpy\\(" radio/src/gui/common/stdlcd/radio_sdmanager.cpp radio/src/gui/colorlcd/radio/radio_sdmanager.cppRepository: EdgeTX/edgetx
Length of output: 5644
🏁 Script executed:
# Verify getSelectionFullPath and the strncpy at line 144
sed -n '82,87p' radio/src/gui/common/stdlcd/radio_sdmanager.cpp
echo "---"
sed -n '140,146p' radio/src/gui/common/stdlcd/radio_sdmanager.cpp
echo "---"
# Find FF_MAX_LFN definition
rg -n "define FF_MAX_LFN" radio/src/Repository: EdgeTX/edgetx
Length of output: 754
🏁 Script executed:
# Check if there's NUL termination after line 144
sed -n '144,146p' radio/src/gui/common/stdlcd/radio_sdmanager.cppRepository: EdgeTX/edgetx
Length of output: 258
sdManagerChdir and getSelectionFullPath have unbounded string operations that can overflow buffers.
-
Lines 76–77:
strcat(sdManagerPath, "/")andstrcat(sdManagerPath, name)lack bounds checking againstsdManagerPath[FF_MAX_LFN + 1](255 bytes). -
Line 79:
f_chdir(sdManagerPath)result is ignored. If the call fails, the tracked path diverges from the filesystem's actual CWD, causing wrong operations. -
Lines 84–86 in
getSelectionFullPath:strcpy(lfn, sdManagerPath)followed by twostrcatcalls without bounds checks onlfn. -
Line 144:
strncpy(clipboard.data.sd.filename, line, CLIPBOARD_PATH_LEN-1)lacks explicit NUL termination (cf. line 143 which properly terminatesdirectory).
Use bounded operations, validate before writing, and only update sdManagerPath after f_chdir succeeds.
Suggested fix
static void sdManagerChdir(const char* name)
{
+ char candidate[FF_MAX_LFN + 1];
+ strncpy(candidate, sdManagerPath, FF_MAX_LFN);
+ candidate[FF_MAX_LFN] = '\0';
+
if (!strcmp(name, "..")) {
// go up: drop the last path segment
- char* sep = strrchr(sdManagerPath, '/');
- if (sep == sdManagerPath) sep[1] = '\0'; // parent is the root
- else if (sep) *sep = '\0';
+ char* sep = strrchr(candidate, '/');
+ if (sep == candidate) sep[1] = '\0'; // parent is the root
+ else if (sep) *sep = '\0';
} else {
// descend into 'name'
- if (sdManagerPath[1] != '\0') strcat(sdManagerPath, "/");
- strcat(sdManagerPath, name);
+ size_t used = strlen(candidate);
+ if (candidate[1] != '\0') {
+ if (used + 1 >= sizeof(candidate)) return;
+ candidate[used++] = '/';
+ candidate[used] = '\0';
+ }
+ size_t remain = sizeof(candidate) - used - 1;
+ if (strlen(name) > remain) return;
+ strcat(candidate, name);
}
- f_chdir(sdManagerPath); // absolute path: resolves on FAT and exFAT alike
+ if (f_chdir(candidate) == FR_OK) {
+ strncpy(sdManagerPath, candidate, FF_MAX_LFN);
+ sdManagerPath[FF_MAX_LFN] = '\0';
+ }
}Add NUL termination at line 145 and apply bounds checking to getSelectionFullPath.
📝 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.
| static void sdManagerChdir(const char* name) | |
| { | |
| if (!strcmp(name, "..")) { | |
| // go up: drop the last path segment | |
| char* sep = strrchr(sdManagerPath, '/'); | |
| if (sep == sdManagerPath) sep[1] = '\0'; // parent is the root | |
| else if (sep) *sep = '\0'; | |
| } else { | |
| // descend into 'name' | |
| if (sdManagerPath[1] != '\0') strcat(sdManagerPath, "/"); | |
| strcat(sdManagerPath, name); | |
| } | |
| f_chdir(sdManagerPath); // absolute path: resolves on FAT and exFAT alike | |
| static void sdManagerChdir(const char* name) | |
| { | |
| char candidate[FF_MAX_LFN + 1]; | |
| strncpy(candidate, sdManagerPath, FF_MAX_LFN); | |
| candidate[FF_MAX_LFN] = '\0'; | |
| if (!strcmp(name, "..")) { | |
| // go up: drop the last path segment | |
| char* sep = strrchr(candidate, '/'); | |
| if (sep == candidate) sep[1] = '\0'; // parent is the root | |
| else if (sep) *sep = '\0'; | |
| } else { | |
| // descend into 'name' | |
| size_t used = strlen(candidate); | |
| if (candidate[1] != '\0') { | |
| if (used + 1 >= sizeof(candidate)) return; | |
| candidate[used++] = '/'; | |
| candidate[used] = '\0'; | |
| } | |
| size_t remain = sizeof(candidate) - used - 1; | |
| if (strlen(name) > remain) return; | |
| strcat(candidate, name); | |
| } | |
| if (f_chdir(candidate) == FR_OK) { | |
| strncpy(sdManagerPath, candidate, FF_MAX_LFN); | |
| sdManagerPath[FF_MAX_LFN] = '\0'; | |
| } | |
| } |
🧰 Tools
🪛 ast-grep (0.44.0)
[error] 75-75: Use of an unbounded buffer function that can overflow the destination; use a size-bounded equivalent (fgets, strncpy/strlcpy, strncat/strlcat, snprintf).
Context: strcat(sdManagerPath, "/")
Note: [CWE-120] Buffer Copy without Checking Size of Input ('Classic Buffer Overflow').
(dangerous-buffer-functions-cpp)
[error] 76-76: Use of an unbounded buffer function that can overflow the destination; use a size-bounded equivalent (fgets, strncpy/strlcpy, strncat/strlcat, snprintf).
Context: strcat(sdManagerPath, name)
Note: [CWE-120] Buffer Copy without Checking Size of Input ('Classic Buffer Overflow').
(dangerous-buffer-functions-cpp)
🤖 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 `@radio/src/gui/common/stdlcd/radio_sdmanager.cpp` around lines 67 - 79, In the
sdManagerChdir function, replace the unbounded strcat calls (lines 76-77) with
bounded string operations that validate the resulting path length does not
exceed FF_MAX_LFN (255 bytes) before concatenating. Additionally, check the
return value of f_chdir on line 79 and only update sdManagerPath if the call
succeeds; if f_chdir fails, the path should remain unchanged to keep internal
tracking consistent with the filesystem's actual current directory. Similarly,
in the getSelectionFullPath function, replace the unbounded strcpy and strcat
operations with bounded alternatives that check against buffer limits. Finally,
after the strncpy call at line 144 for clipboard.data.sd.filename, explicitly
add NUL termination (similar to how directory is terminated at line 143) to
ensure the string is properly null-terminated.
Source: Linters/SAST tools
| void getSelectionFullPath(char * lfn) | ||
| { | ||
| f_getcwd(lfn, FF_MAX_LFN); | ||
| strcpy(lfn, sdManagerPath); | ||
| strcat(lfn, "/"); | ||
| strcat(lfn, reusableBuffer.sdManager.lines[menuVerticalPosition - HEADER_LINE - menuVerticalOffset]); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "radio_sdmanager.cpp" | head -5Repository: EdgeTX/edgetx
Length of output: 252
🏁 Script executed:
fd "radio_sdmanager" --type fRepository: EdgeTX/edgetx
Length of output: 295
🏁 Script executed:
rg "FF_MAX_LFN" --type cpp --type c -A 2 -B 2Repository: EdgeTX/edgetx
Length of output: 151
🏁 Script executed:
rg "getSelectionFullPath" --type cpp --type c -B 5 -A 10Repository: EdgeTX/edgetx
Length of output: 151
🏁 Script executed:
cat -n radio/src/gui/common/stdlcd/radio_sdmanager.cpp | sed -n '75,95p'Repository: EdgeTX/edgetx
Length of output: 825
🏁 Script executed:
rg "MAX_LFN|LFN_MAX|FF_LFN" --type cpp --type c --type h | head -20Repository: EdgeTX/edgetx
Length of output: 151
🏁 Script executed:
rg "sdManagerPath" --type cpp --type c --type h -B 2 -A 2 | head -30Repository: EdgeTX/edgetx
Length of output: 151
🏁 Script executed:
grep -r "FF_MAX_LFN\|LFN_MAX\|MAX_LFN" --include="*.h" --include="*.cpp" --include="*.c" | head -20Repository: EdgeTX/edgetx
Length of output: 2133
🏁 Script executed:
rg "define FF_MAX_LFN|FF_MAX_LFN\s*=" --type h --type hpp | head -10Repository: EdgeTX/edgetx
Length of output: 179
🏁 Script executed:
find . -type f -name "*.h" -o -name "*.hpp" | xargs grep -l "FF_MAX_LFN" | head -10Repository: EdgeTX/edgetx
Length of output: 219
🏁 Script executed:
rg "getSelectionFullPath\(" --type cpp --type c -B 2 -A 2Repository: EdgeTX/edgetx
Length of output: 151
🏁 Script executed:
cat -n radio/src/gui/common/stdlcd/radio_sdmanager.cpp | head -50Repository: EdgeTX/edgetx
Length of output: 2142
🏁 Script executed:
rg "FF_MAX_LFN" radio/src/thirdparty/FatFs/ffconf.h -A 2 -B 2Repository: EdgeTX/edgetx
Length of output: 885
🏁 Script executed:
rg "sdManagerPath\s*\[|char.*sdManagerPath" radio/src/gui/common/stdlcd/radio_sdmanager.cppRepository: EdgeTX/edgetx
Length of output: 307
🏁 Script executed:
cat -n radio/src/gui/common/stdlcd/radio_sdmanager.cpp | grep -A 5 -B 5 "getSelectionFullPath"Repository: EdgeTX/edgetx
Length of output: 6173
🏁 Script executed:
rg "getSelectionFullPath" radio/src/gui/common/stdlcd/ radio/src/gui/colorlcd/ -B 3 -A 3Repository: EdgeTX/edgetx
Length of output: 9870
Use snprintf to prevent buffer overflow when composing file path.
The strcpy and strcat operations at lines 84-86 lack bounds checking. Since sdManagerPath can reach 255 characters, combined with "/" and the selected filename, the resulting path can exceed the size of the lfn buffer passed by callers. Use snprintf instead:
Suggested fix
void getSelectionFullPath(char * lfn)
{
- strcpy(lfn, sdManagerPath);
- strcat(lfn, "/");
- strcat(lfn, reusableBuffer.sdManager.lines[menuVerticalPosition - HEADER_LINE - menuVerticalOffset]);
+ snprintf(lfn, FF_MAX_LFN + 1, "%s/%s",
+ sdManagerPath,
+ reusableBuffer.sdManager.lines[menuVerticalPosition - HEADER_LINE - menuVerticalOffset]);
}🧰 Tools
🪛 ast-grep (0.44.0)
[error] 83-83: Use of an unbounded buffer function that can overflow the destination; use a size-bounded equivalent (fgets, strncpy/strlcpy, strncat/strlcat, snprintf).
Context: strcpy(lfn, sdManagerPath)
Note: [CWE-120] Buffer Copy without Checking Size of Input ('Classic Buffer Overflow').
(dangerous-buffer-functions-cpp)
[error] 84-84: Use of an unbounded buffer function that can overflow the destination; use a size-bounded equivalent (fgets, strncpy/strlcpy, strncat/strlcat, snprintf).
Context: strcat(lfn, "/")
Note: [CWE-120] Buffer Copy without Checking Size of Input ('Classic Buffer Overflow').
(dangerous-buffer-functions-cpp)
[error] 85-85: Use of an unbounded buffer function that can overflow the destination; use a size-bounded equivalent (fgets, strncpy/strlcpy, strncat/strlcat, snprintf).
Context: strcat(lfn, reusableBuffer.sdManager.lines[menuVerticalPosition - HEADER_LINE - menuVerticalOffset])
Note: [CWE-120] Buffer Copy without Checking Size of Input ('Classic Buffer Overflow').
(dangerous-buffer-functions-cpp)
🤖 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 `@radio/src/gui/common/stdlcd/radio_sdmanager.cpp` around lines 82 - 87, In the
getSelectionFullPath function, replace the unsafe strcpy and strcat operations
with snprintf to safely compose the file path with bounds checking. Use snprintf
with the appropriate buffer size limit for the lfn parameter to ensure the
composed path (sdManagerPath concatenated with "/" and the selected filename)
does not exceed the buffer capacity and prevent potential buffer overflow.
Source: Linters/SAST tools
There was a problem hiding this comment.
valid concern, but the suggested direction would break a caller
Not a regression. The original code keyed off CWD too (isCwdAtRoot() → f_getcwd()), so my fs->cdir version had the same CWD-coupling — just made to work on exFAT. So nothing got worse.
Their implied fix (key off the opened dir, sclust) would actually break theme_manager. It opens /THEMES (no chdir) and calls scanThemeFolder(fno.fname) for every AM_DIR entry (theme_manager.cpp:330). If ".." were injected there, it'd recurse into the parent. With a pure opened-dir check, /THEMES is non-root → ".." always injected → broken.
filechoice is immune regardless — it continues on every AM_DIR (filechoice.cpp:135).
The actually-correct rule
Fake ".." only when we're browsing the current dir itself and it isn't root:
if (firstTime && dir->obj.fs->cdir != 0 && dir->obj.sclust == dir->obj.fs->cdir) {
- obj.sclust = opened dir's start cluster, fs->cdir = CWD's (both 0 = root, FAT and exFAT).
- The navigable browsers all f_chdir(...) then f_opendir("."), so sclust == cdir → ".." appears correctly.
- theme_manager/filechoice open an arbitrary path without chdir, so sclust != cdir → no ".." injected. This is more robust than the old code, which only stayed safe because CWD happened to be root at scan time.
So the fix now works on exFAT and removes the latent CWD-coupling the reviewer flagged.
There was a problem hiding this comment.
@3djc — the two concerns are separate; let me address each:
getSelectionFullPath buffer overflow (this comment's original concern): The snprintf suggestion stands independently of the ..-injection logic. sdManagerPath is FF_MAX_LFN + 1 bytes, and lfn is declared as TCHAR lfn[FF_MAX_LFN + 1] at all call sites, so the bounds are consistent — but there's no enforcement that they remain so. A single snprintf(lfn, FF_MAX_LFN + 1, "%s/%s", ...) makes that contract explicit and static-analysis-clean at no cost to callers.
sdReadDir ..-injection condition (your actual response): Your sclust == cdir refinement is technically sound. The current PR uses dir->obj.fs->cdir != 0 alone, which is true for any non-root opened directory — including theme_manager's /THEMES scan. Adding the sclust == cdir guard narrows injection to the case where the opened dir is the CWD (i.e., f_opendir(".") after f_chdir()), which is exactly what the SD manager does. Theme_manager and filechoice open arbitrary paths without chdir, so sclust != cdir → no .. injected. That's a genuine improvement in correctness over both the old isCwdAtRoot() approach and the current cdir != 0 check. Is the plan to update lib_file.cpp with this condition in this PR?
| strncpy(clipboard.data.sd.filename, line, CLIPBOARD_PATH_LEN-1); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Mirror directory handling: NUL-terminate copied filename.
At Line 144, clipboard.data.sd.filename is copied with strncpy but not explicitly terminated. This has the same edge-case breakage risk as in the colorlcd manager.
Suggested fix
strncpy(clipboard.data.sd.filename, line, CLIPBOARD_PATH_LEN-1);
+ clipboard.data.sd.filename[CLIPBOARD_PATH_LEN - 1] = '\0';🤖 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 `@radio/src/gui/common/stdlcd/radio_sdmanager.cpp` around lines 144 - 145, The
strncpy call for clipboard.data.sd.filename does not explicitly NUL-terminate
the destination buffer, which can leave the string unterminated when the source
string is at or exceeds the buffer size. Add a line immediately after the
strncpy statement to explicitly NUL-terminate clipboard.data.sd.filename at
index CLIPBOARD_PATH_LEN-1 by setting it to '\0', ensuring the copied filename
is always properly terminated regardless of the input length.
|
bot gone stupid because of my reposne at the wrong place |
1 participant
On exFAT-formatted SD cards the file browser was broken: the ".." (go up) entry never appeared, you couldn't navigate out of a sub-directory, and copy/paste targeted the wrong location. FAT32 cards were unaffected. As exemple, you could not open txt file for reading in SD manager.
Root cause
Two FatFs calls the browsers relied on don't work on exFAT, because exFAT directories carry no on-disk "." / ".." entries:
f_getcwd()always returns/— it rebuilds the path by walking up the ".." chain, which doesn't exist on exFAT(FatFs documents this: "Cannot do getcwd on exFAT and returns root path").
This broke ".." detection, the current-path display, nested navigation and clipboard paths.
f_chdir("..")is a no-op — with no ".." entry, FatFs "stays there" (ff.c), so "go up" did nothing.This fixes all of it for both the colorLCD and B&W SD managers.
I have taken great care of not breaking fat32, but I would appreciate some external tests
Summary by CodeRabbit