Skip to content

fix(radio): exFat directory handling #7484

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
3djc wants to merge 2 commits into from
+68 −46
Closed

fix(radio): exFat directory handling #7484

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dc7f534
fix: exFat directory handling
Jun 23, 2026
391804d
improve based on review
Jun 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 26 additions & 29 deletions radio/src/gui/colorlcd/libui/file_browser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,22 +29,6 @@
#define CELL_CTRL_DIR LV_TABLE_CELL_CTRL_CUSTOM_1
#define CELL_CTRL_FILE LV_TABLE_CELL_CTRL_CUSTOM_2

static const char* getFullPath(const char* filename)
{
static char full_path[FF_MAX_LFN + 1];
f_getcwd((TCHAR*)full_path, FF_MAX_LFN);
strcat(full_path, "/");
strcat(full_path, filename);
return full_path;
}

static const char* getCurrentPath()
{
static char path[FF_MAX_LFN + 1];
f_getcwd((TCHAR*)path, FF_MAX_LFN);
return path;
}

static int strnatcasecmp(char const *s1, char const *s2)
{
int i1, i2;
Expand Down Expand Up @@ -138,10 +122,18 @@ static int scan_files(std::list<std::string>& files,
return 0;
}

void FileBrowser::setFullPath(const char* name)
{
if (currentPath == "/")
fullPathBuf = std::string("/") + name;
else
fullPathBuf = currentPath + "/" + name;
}

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());
Comment on lines 133 to +136

@coderabbitai coderabbitai Bot Jun 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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.


setAutoEdit();

Expand Down Expand Up @@ -244,18 +236,24 @@ void FileBrowser::onSelected(const char* name, bool is_dir)
return;
}

const char* path = getCurrentPath();
const char* fullpath = getFullPath(name);
if (fileSelected) fileSelected(path, name, fullpath, is_dir);
setFullPath(name);
if (fileSelected) fileSelected(currentPath.c_str(), name, fullPathBuf.c_str(), is_dir);
selected = name;
}

void FileBrowser::onPress(const char* name, bool is_dir)
{
const char* path = getCurrentPath();
const char* fullpath = getFullPath(name);
if (is_dir) {
f_chdir(fullpath);
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);
} else {
setFullPath(name);
currentPath = fullPathBuf;
}
f_chdir(currentPath.c_str());
if (fileSelected) fileSelected(nullptr, nullptr, nullptr, is_dir);
selected = nullptr;
refresh();
Expand All @@ -268,20 +266,19 @@ void FileBrowser::onPress(const char* name, bool is_dir)
}

if (fileAction){
fileAction(path, name, fullpath, is_dir);
setFullPath(name);
fileAction(currentPath.c_str(), name, fullPathBuf.c_str(), is_dir);
}
}

void FileBrowser::onPressLong(const char* name, bool is_dir)
{
const char* path = getCurrentPath();
const char* fullpath = getFullPath(name);

if (!selected || (selected != name)) {
onSelected(name, is_dir);
}

if (fileAction){
fileAction(path, name, fullpath, is_dir);
setFullPath(name);
fileAction(currentPath.c_str(), name, fullPathBuf.c_str(), is_dir);
}
}
8 changes: 8 additions & 0 deletions radio/src/gui/colorlcd/libui/file_browser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@

#pragma once

#include <string>

#include "table.h"

class FileBrowser : public TableField
Expand Down Expand Up @@ -52,6 +54,12 @@ class FileBrowser : public TableField
void onPressLong(const char* name, bool is_dir);

private:
// Current dir, tracked explicitly because f_getcwd() is broken on exFAT.
std::string currentPath;
// Outlives the file-action call: callbacks capture the pointer for later use.
std::string fullPathBuf;
void setFullPath(const char* name);

const char* selected = nullptr;
FileAction fileAction;
FileAction fileSelected;
Expand Down
8 changes: 6 additions & 2 deletions radio/src/gui/colorlcd/radio/radio_sdmanager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,14 +467,18 @@ void RadioSdManagerPage::fileAction(const char* path, const char* name,
}
menu->addLine(STR_COPY_FILE, [=]() {
clipboard.type = CLIPBOARD_TYPE_SD_FILE;
f_getcwd(clipboard.data.sd.directory, CLIPBOARD_PATH_LEN);
// f_getcwd() is a no-op on exFAT; use the tracked path.
strncpy(clipboard.data.sd.directory, path, CLIPBOARD_PATH_LEN - 1);
clipboard.data.sd.directory[CLIPBOARD_PATH_LEN - 1] = '\0';
strncpy(clipboard.data.sd.filename, name, CLIPBOARD_PATH_LEN - 1);
});
Comment on lines 473 to 474

@coderabbitai coderabbitai Bot Jun 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.

Suggested change
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.

if (clipboard.type == CLIPBOARD_TYPE_SD_FILE) {
menu->addLine(STR_PASTE, [=]() {
static char lfn[FF_MAX_LFN + 1]; // TODO optimize that!
char destFileName[2 * CLIPBOARD_PATH_LEN + 1];
f_getcwd((TCHAR*)lfn, FF_MAX_LFN);
// f_getcwd() is a no-op on exFAT; use the tracked path.
strncpy(lfn, path, FF_MAX_LFN);
lfn[FF_MAX_LFN] = '\0';
// prevent copying to the same directory with the same name
char* destNamePtr = clipboard.data.sd.filename;
if (!strcmp(clipboard.data.sd.directory, lfn)) {
Expand Down
28 changes: 24 additions & 4 deletions radio/src/gui/common/stdlcd/radio_sdmanager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,27 @@ inline bool isFilenameLower(bool isfile, const char * fn, const char * line)
return (!isfile && IS_FILE(line)) || (isfile==IS_FILE(line) && strcasecmp(fn, line) < 0);
}

// Current dir, tracked explicitly: on exFAT f_getcwd() and f_chdir("..") are no-ops.
static char sdManagerPath[FF_MAX_LFN + 1] = "/";

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
Comment on lines +67 to +79

@coderabbitai coderabbitai Bot Jun 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.cpp

Repository: 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.cpp

Repository: EdgeTX/edgetx

Length of output: 258

sdManagerChdir and getSelectionFullPath have unbounded string operations that can overflow buffers.

  1. Lines 76–77: strcat(sdManagerPath, "/") and strcat(sdManagerPath, name) lack bounds checking against sdManagerPath[FF_MAX_LFN + 1] (255 bytes).

  2. 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.

  3. Lines 84–86 in getSelectionFullPath: strcpy(lfn, sdManagerPath) followed by two strcat calls without bounds checks on lfn.

  4. Line 144: strncpy(clipboard.data.sd.filename, line, CLIPBOARD_PATH_LEN-1) lacks explicit NUL termination (cf. line 143 which properly terminates directory).

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.

Suggested change
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]);
}
Comment on lines 82 to 87

@coderabbitai coderabbitai Bot Jun 23, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find . -type f -name "radio_sdmanager.cpp" | head -5

Repository: EdgeTX/edgetx

Length of output: 252

🏁 Script executed:

fd "radio_sdmanager" --type f

Repository: EdgeTX/edgetx

Length of output: 295

🏁 Script executed:

rg "FF_MAX_LFN" --type cpp --type c -A 2 -B 2

Repository: EdgeTX/edgetx

Length of output: 151

🏁 Script executed:

rg "getSelectionFullPath" --type cpp --type c -B 5 -A 10

Repository: 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 -20

Repository: EdgeTX/edgetx

Length of output: 151

🏁 Script executed:

rg "sdManagerPath" --type cpp --type c --type h -B 2 -A 2 | head -30

Repository: EdgeTX/edgetx

Length of output: 151

🏁 Script executed:

grep -r "FF_MAX_LFN\|LFN_MAX\|MAX_LFN" --include="*.h" --include="*.cpp" --include="*.c" | head -20

Repository: EdgeTX/edgetx

Length of output: 2133

🏁 Script executed:

rg "define FF_MAX_LFN|FF_MAX_LFN\s*=" --type h --type hpp | head -10

Repository: EdgeTX/edgetx

Length of output: 179

🏁 Script executed:

find . -type f -name "*.h" -o -name "*.hpp" | xargs grep -l "FF_MAX_LFN" | head -10

Repository: EdgeTX/edgetx

Length of output: 219

🏁 Script executed:

rg "getSelectionFullPath\(" --type cpp --type c -B 2 -A 2

Repository: EdgeTX/edgetx

Length of output: 151

🏁 Script executed:

cat -n radio/src/gui/common/stdlcd/radio_sdmanager.cpp | head -50

Repository: EdgeTX/edgetx

Length of output: 2142

🏁 Script executed:

rg "FF_MAX_LFN" radio/src/thirdparty/FatFs/ffconf.h -A 2 -B 2

Repository: EdgeTX/edgetx

Length of output: 885

🏁 Script executed:

rg "sdManagerPath\s*\[|char.*sdManagerPath" radio/src/gui/common/stdlcd/radio_sdmanager.cpp

Repository: 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 3

Repository: 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

@3djc 3djc Jun 23, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coderabbitai coderabbitai Bot Jun 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Expand Down Expand Up @@ -121,12 +139,13 @@ void onSdManagerMenu(const char * result)
}
else if (result == STR_COPY_FILE) {
clipboard.type = CLIPBOARD_TYPE_SD_FILE;
f_getcwd(clipboard.data.sd.directory, CLIPBOARD_PATH_LEN);
strncpy(clipboard.data.sd.directory, sdManagerPath, CLIPBOARD_PATH_LEN - 1);
clipboard.data.sd.directory[CLIPBOARD_PATH_LEN - 1] = '\0';
strncpy(clipboard.data.sd.filename, line, CLIPBOARD_PATH_LEN-1);
}
Comment on lines 144 to 145

@coderabbitai coderabbitai Bot Jun 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.

else if (result == STR_PASTE) {
char destFileName[2 * CLIPBOARD_PATH_LEN + 1];
f_getcwd(lfn, FF_MAX_LFN);
strcpy(lfn, sdManagerPath);
// if destination is dir, copy into that dir
if (IS_DIRECTORY(line)) {
strcat(lfn, "/");
Expand Down Expand Up @@ -279,6 +298,7 @@ void menuRadioSdManager(event_t _event)

if (_event == EVT_ENTRY) {
f_chdir(ROOT_PATH);
strcpy(sdManagerPath, ROOT_PATH);
#if LCD_DEPTH > 1
lastPos = -1;
#endif
Expand Down Expand Up @@ -325,7 +345,7 @@ void menuRadioSdManager(event_t _event)
else {
int index = menuVerticalPosition - HEADER_LINE - menuVerticalOffset;
if (IS_DIRECTORY(reusableBuffer.sdManager.lines[index])) {
f_chdir(reusableBuffer.sdManager.lines[index]);
sdManagerChdir(reusableBuffer.sdManager.lines[index]);
menuVerticalOffset = 0;
menuVerticalPosition = HEADER_LINE;
REFRESH_FILES();
Expand Down
15 changes: 4 additions & 11 deletions radio/src/lib_file.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,16 +83,6 @@ bool isExtensionMatching(const char * extension, const char * pattern, char * ma
return false;
}

// returns true if current working dir is at the root level
bool isCwdAtRoot()
{
char path[10];
if (f_getcwd(path, sizeof(path)-1) == FR_OK) {
return (strcasecmp("/", path) == 0);
}
return false;
}

/*
Wrapper around the f_readdir() function which
also returns ".." entry for sub-dirs. (FatFS 0.12 does
Expand All @@ -101,7 +91,10 @@ bool isCwdAtRoot()
FRESULT sdReadDir(DIR * dir, FILINFO * fno, bool & firstTime)
{
FRESULT res;
if (firstTime && !isCwdAtRoot()) {
// Fake ".." only when browsing the non-root CWD itself (sclust == cdir, 0 == root).
// Filesystem-CWD based so it works on exFAT, where f_getcwd() always returns root;
// the sclust check keeps callers that open an arbitrary path (no chdir) unaffected.
if (firstTime && dir->obj.fs->cdir != 0 && dir->obj.sclust == dir->obj.fs->cdir) {
// fake parent directory entry
strcpy(fno->fname, "..");
fno->fattrib = AM_DIR;
Expand Down