fix(phone-api): skip manifest scan for node-info-only config requests (#10754)

* fix(fs): bound getFiles() result size and harden traversal The recursive filesystem traversal in getFiles() could grow its result vector without limit, contributing to heap pressure on memory-constrained devices during client config starts. The implementation also relied on unbounded strcpy and per-entry String() allocations for dot-suffix filtering, and leaked the directory handle when the opened path was not a directory. Rework the traversal to address all of those at once: - Add optional maxCount (default 64) and wasLimited out-param. The walk stops once maxCount entries have been collected and wasLimited is set, so callers can distinguish a complete listing from a truncated one. Default arguments preserve backward compatibility for existing callers. - Extract the recursive walk into a collectFiles() helper in an anonymous namespace, accumulating into a single caller-owned vector instead of building and inserting temporary vectors at every recursion level. - Replace strcpy with strlcpy so file_name writes are bounded by the destination buffer. - Replace String(name).endsWith(".") with a stack-based pathEndsWithDot() helper to avoid a heap allocation per entry. - Close the directory handle when the opened path is not a directory, and close the file in both branches of recursion instead of only the leaf branch (the previous code leaked the handle when depth was exhausted). - Compile-gate the std::bad_alloc reserve probe-down behind __cpp_exceptions / __EXCEPTIONS so the fallback only compiles where bad_alloc is actually catchable. No-exception targets (nRF52, RP2040, STM32, ESP32-C6) skip the probe and rely on the bounded maxCount alone, which is safe because the default cap is small enough to fit comfortably in available RAM on every supported platform. * fix(phone-api): skip manifest scan for node-info-only config requests A client can request a node-info-only config response by sending want_config_id with the SPECIAL_NONCE_ONLY_NODES nonce. The manifest walk in handleStartConfig() ran unconditionally even though STATE_SEND_FILEMANIFEST short-circuits past the manifest on that path, so the SPI lock and recursive filesystem traversal were wasted work. On memory-constrained devices this extra heap pressure was enough to trigger allocation aborts and spontaneous reboots before the node-info response could complete. Gate the manifest call behind config_nonce != SPECIAL_NONCE_ONLY_NODES so the node-info-only path no longer touches the filesystem at all, and release any stale manifest storage left over from a previous run. For normal full-config requests, also tighten the scan limits from depth 10 / unbounded to depth 3 / max 64 entries via the new getFiles() parameters. A 10-level recursive listing was never useful for a client file picker and amplified heap pressure for no benefit. Log a warning when the listing was truncated so operators can distinguish a bounded result from a genuinely empty filesystem. A few small cleanups fall out of the above: - Replace raw spiLock->lock()/unlock() with a LockGuard so the lock is released even if getFiles() throws on exception-enabled targets. - Use a releaseFilesManifest() helper (swap-with-empty idiom) in close() and the STATE_SEND_FILEMANIFEST drain path instead of the previous clear()+shrink_to_fit() pair. Equivalent behavior, but the swap idiom is the canonical vector-freeing pattern and centralizing it avoids divergence between call sites. - Use %zu for the size_t log arguments. * fix(fs): harden bounded directory traversal Close each child directory entry before recursive descent so bounded manifest scans do not stack file handles on constrained filesystems. Treat null or overlong paths as truncated listings, handle reserve length failures like allocation failures, and keep manifest constants aligned with repository naming. * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Ben Meadors <benmmeadors@gmail.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
2026-06-23 11:26:52 -05:00
parent 640002dc93
commit 54e0d8d0ab
3 changed files with 328 additions and 238 deletions
@@ -1,20 +1,18 @@
 /**
 * @file FSCommon.cpp
- * @brief This file contains functions for common filesystem operations such as
- * copying, renaming, listing and deleting files and directories.
+ * @brief This file contains functions for common filesystem operations such as copying, renaming, listing and deleting files and
+ * directories.
 *
- * The functions in this file are used to perform common filesystem operations
- * such as copying, renaming, listing and deleting files and directories. These
- * functions are used in the Meshtastic-device project to manage files and
- * directories on the device's filesystem.
+ * The functions in this file are used to perform common filesystem operations such as copying, renaming, listing and deleting
+ * files and directories. These functions are used in the Meshtastic-device project to manage files and directories on the
+ * device's filesystem.
 *
 */
 #include "FSCommon.h"
 #include "SPILock.h"
 #include "configuration.h"

-// Software SPI is used by MUI so disable SD card here until it's also
-// implemented
+// Software SPI is used by MUI so disable SD card here until it's also implemented
 #if defined(HAS_SDCARD) && !defined(SDCARD_USE_SOFT_SPI)
 #include <SD.h>
 #include <SPI.h>
@@ -39,7 +37,8 @@ SPIClass SPI_HSPI(HSPI);
 * @param to The path of the destination file.
 * @return true if the file was successfully copied, false otherwise.
 */
-bool copyFile(const char *from, const char *to) {
+bool copyFile(const char *from, const char *to)
+{
 #ifdef FSCom
    // take SPI Lock
    concurrency::LockGuard g(spiLock);
@@ -77,7 +76,8 @@ bool copyFile(const char *from, const char *to) {
 *
 * @return True if the file was successfully renamed, false otherwise.
 */
-bool renameFile(const char *pathFrom, const char *pathTo) {
+bool renameFile(const char *pathFrom, const char *pathTo)
+{
 #ifdef FSCom

 #ifdef ARCH_ESP32
@@ -99,54 +99,87 @@ bool renameFile(const char *pathFrom, const char *pathTo) {
 #endif
 }

+#include <cstring>
+#include <new>
+#include <stdexcept>
 #include <vector>

-/**
- * @brief Get the list of files in a directory.
- *
- * This function returns a list of files in a directory. The list includes the
- * full path of each file. We can't use SPILOCK here because of recursion.
- * Callers of this function should use SPILOCK.
- *
- * @param dirname The name of the directory.
- * @param levels The number of levels of subdirectories to list.
- * @return A vector of strings containing the full path of each file in the
- * directory.
- */
-std::vector<meshtastic_FileInfo> getFiles(const char *dirname, uint8_t levels) {
-  std::vector<meshtastic_FileInfo> filenames = {};
 #ifdef FSCom
+namespace
+{
+bool pathEndsWithDot(const char *path)
+{
+    if (!path)
+        return false;
+
+    size_t length = strlen(path);
+    return length > 0 && path[length - 1] == '.';
+}
+
+bool copyFilePath(char *dest, size_t destSize, const char *path, bool *wasLimited)
+{
+    if (!path || destSize == 0) {
+        if (wasLimited)
+            *wasLimited = true;
+        return false;
+    }
+
+    if (strlcpy(dest, path, destSize) >= destSize) {
+        if (wasLimited)
+            *wasLimited = true;
+        return false;
+    }
+
+    return true;
+}
+
+void collectFiles(const char *dirname, uint8_t levels, size_t maxCount, std::vector<meshtastic_FileInfo> &filenames,
+                  bool *wasLimited)
+{
+    if (!dirname)
+        return;
+
    File root = FSCom.open(dirname, FILE_O_READ);
    if (!root)
-    return filenames;
-  if (!root.isDirectory())
-    return filenames;
+        return;
+    if (!root.isDirectory()) {
+        root.close();
+        return;
+    }

    File file = root.openNextFile();
-  while (file &&
-         file.name()[0]) { // file.name()[0] check is a workaround for a bug in
-                           // the Adafruit LittleFS nrf52 glue (see issue 4395)
-    if (file.isDirectory() && !String(file.name()).endsWith(".")) {
-      if (levels) {
-#ifdef ARCH_ESP32
-        std::vector<meshtastic_FileInfo> subDirFilenames =
-            getFiles(file.path(), levels - 1);
-#else
-        std::vector<meshtastic_FileInfo> subDirFilenames =
-            getFiles(file.name(), levels - 1);
-#endif
-        filenames.insert(filenames.end(), subDirFilenames.begin(),
-                         subDirFilenames.end());
+    // file.name()[0] check is a workaround for a bug in the Adafruit LittleFS nrf52 glue (see issue 4395)
+    while (file && file.name()[0]) {
+        if (filenames.size() >= maxCount) {
+            if (wasLimited)
+                *wasLimited = true;
            file.close();
+            break;
+        }
+        const char *fileName = file.name();
+        if (file.isDirectory() && !pathEndsWithDot(fileName)) {
+            char pathBuffer[sizeof(((meshtastic_FileInfo *)nullptr)->file_name)] = {};
+#ifdef ARCH_ESP32
+            const char *subDirPath = file.path();
+#else
+            const char *subDirPath = fileName;
+#endif
+            bool hasSubDirPath = copyFilePath(pathBuffer, sizeof(pathBuffer), subDirPath, wasLimited);
+            file.close();
+
+            if (levels && hasSubDirPath) {
+                collectFiles(pathBuffer, levels - 1, maxCount, filenames, wasLimited);
+            } else if (wasLimited) {
+                *wasLimited = true;
            }
        } else {
            meshtastic_FileInfo fileInfo = {"", static_cast<uint32_t>(file.size())};
 #ifdef ARCH_ESP32
-      strcpy(fileInfo.file_name, file.path());
+            bool hasFilePath = copyFilePath(fileInfo.file_name, sizeof(fileInfo.file_name), file.path(), wasLimited);
 #else
-      strcpy(fileInfo.file_name, file.name());
+            bool hasFilePath = copyFilePath(fileInfo.file_name, sizeof(fileInfo.file_name), file.name(), wasLimited);
 #endif
-      if (!String(fileInfo.file_name).endsWith(".")) {
+            if (hasFilePath && !pathEndsWithDot(fileInfo.file_name)) {
                filenames.push_back(fileInfo);
            }
            file.close();
@@ -154,21 +187,55 @@ std::vector<meshtastic_FileInfo> getFiles(const char *dirname, uint8_t levels) {
        file = root.openNextFile();
    }
    root.close();
+}
+} // namespace
+#endif
+
+// Callers must hold the SPI lock; recursion prevents taking it here.
+std::vector<meshtastic_FileInfo> getFiles(const char *dirname, uint8_t levels, size_t maxCount, bool *wasLimited)
+{
+    std::vector<meshtastic_FileInfo> filenames = {};
+    if (wasLimited)
+        *wasLimited = false;
+#ifdef FSCom
+#if defined(__cpp_exceptions) || defined(__EXCEPTIONS)
+    size_t reservedCount = maxCount;
+    while (reservedCount > 0) {
+        try {
+            filenames.reserve(reservedCount);
+            break;
+        } catch (const std::bad_alloc &) {
+            reservedCount /= 2;
+        } catch (const std::length_error &) {
+            reservedCount /= 2;
+        }
+    }
+    if (reservedCount == 0) {
+        if (wasLimited)
+            *wasLimited = true;
+        return filenames;
+    }
+    if (reservedCount < maxCount) {
+        if (wasLimited)
+            *wasLimited = true;
+        maxCount = reservedCount;
+    }
+#endif
+    collectFiles(dirname, levels, maxCount, filenames, wasLimited);
 #endif
    return filenames;
 }

 /**
 * Lists the contents of a directory.
- * We can't use SPILOCK here because of recursion. Callers of this function
- * should use SPILOCK.
+ * We can't use SPILOCK here because of recursion. Callers of this function should use SPILOCK.
 *
 * @param dirname The name of the directory to list.
 * @param levels The number of levels of subdirectories to list.
- * @param del Whether or not to delete the contents of the directory after
- * listing.
+ * @param del Whether or not to delete the contents of the directory after listing.
 */
-void listDir(const char *dirname, uint8_t levels, bool del) {
+void listDir(const char *dirname, uint8_t levels, bool del)
+{
 #ifdef FSCom
 #if (defined(ARCH_ESP32) || defined(ARCH_RP2040) || defined(ARCH_PORTDUINO))
    char buffer[255];
@@ -184,8 +251,7 @@ void listDir(const char *dirname, uint8_t levels, bool del) {
    File file = root.openNextFile();
    while (
        file &&
-      file.name()[0]) { // This file.name() check is a workaround for a bug in
-                        // the Adafruit LittleFS nrf52 glue (see issue 4395)
+        file.name()[0]) { // This file.name() check is a workaround for a bug in the Adafruit LittleFS nrf52 glue (see issue 4395)
        if (file.isDirectory() && !String(file.name()).endsWith(".")) {
            if (levels) {
 #ifdef ARCH_ESP32
@@ -269,12 +335,12 @@ void listDir(const char *dirname, uint8_t levels, bool del) {
 /**
 * @brief Removes a directory and all its contents.
 *
- * This function recursively removes a directory and all its contents, including
- * subdirectories and files.
+ * This function recursively removes a directory and all its contents, including subdirectories and files.
 *
 * @param dirname The name of the directory to remove.
 */
-void rmDir(const char *dirname) {
+void rmDir(const char *dirname)
+{
 #ifdef FSCom

 #if (defined(ARCH_ESP32) || defined(ARCH_RP2040) || defined(ARCH_PORTDUINO))
@@ -292,7 +358,8 @@ void rmDir(const char *dirname) {
 */
 __attribute__((weak, noinline)) void preFSBegin() {}

-void fsInit() {
+void fsInit()
+{
 #ifdef FSCom
    concurrency::LockGuard g(spiLock);
    preFSBegin();
@@ -301,8 +368,7 @@ void fsInit() {
        // assert(0); This auto-formats the partition, so no need to fail here.
    }
 #if defined(ARCH_ESP32)
-  LOG_DEBUG("Filesystem files (%d/%d Bytes):", FSCom.usedBytes(),
-            FSCom.totalBytes());
+    LOG_DEBUG("Filesystem files (%d/%d Bytes):", FSCom.usedBytes(), FSCom.totalBytes());
 #else
    LOG_DEBUG("Filesystem files:");
 #endif
@@ -313,7 +379,8 @@ void fsInit() {
 /**
 * Initializes the SD card and mounts the file system.
 */
-void setupSDCard() {
+void setupSDCard()
+{
 #if defined(HAS_SDCARD) && !defined(SDCARD_USE_SOFT_SPI)
    concurrency::LockGuard g(spiLock);
    SDHandler.begin(SPI_SCK, SPI_MISO, SPI_MOSI);
@@ -52,7 +52,7 @@ void fsInit();
 void fsListFiles();
 bool copyFile(const char *from, const char *to);
 bool renameFile(const char *pathFrom, const char *pathTo);
-std::vector<meshtastic_FileInfo> getFiles(const char *dirname, uint8_t levels);
+std::vector<meshtastic_FileInfo> getFiles(const char *dirname, uint8_t levels, size_t maxCount = 64, bool *wasLimited = nullptr);
 void listDir(const char *dirname, uint8_t levels, bool del = false);
 void rmDir(const char *dirname);
 void setupSDCard();
@@ -36,6 +36,17 @@
 // Flag to indicate a heartbeat was received and we should send queue status
 bool heartbeatReceived = false;

+namespace
+{
+constexpr uint8_t FILES_MANIFEST_LEVELS = 3;
+constexpr size_t FILES_MANIFEST_MAX_COUNT = 64;
+
+void releaseFilesManifest(std::vector<meshtastic_FileInfo> &filesManifest)
+{
+    std::vector<meshtastic_FileInfo>().swap(filesManifest);
+}
+} // namespace
+
 PhoneAPI::PhoneAPI()
 {
    lastContactMsec = millis();
@@ -70,10 +81,23 @@ void PhoneAPI::handleStartConfig()
        state = STATE_SEND_MY_INFO;
    }
    pauseBluetoothLogging = true;
-    spiLock->lock();
-    filesManifest = getFiles("/", 10);
-    spiLock->unlock();
-    LOG_DEBUG("Got %d files in manifest", filesManifest.size());
+    // Manifest is never read on the node-info-only path (STATE_SEND_FILEMANIFEST
+    // short-circuits to sendConfigComplete), so skip the SPI lock + FS walk.
+    if (config_nonce != SPECIAL_NONCE_ONLY_NODES) {
+        bool filesManifestLimited = false;
+        {
+            concurrency::LockGuard guard(spiLock);
+            filesManifest = getFiles("/", FILES_MANIFEST_LEVELS, FILES_MANIFEST_MAX_COUNT, &filesManifestLimited);
+        }
+        if (filesManifestLimited) {
+            LOG_WARN("Got %zu files in manifest (limited to %zu entries/depth %u)", filesManifest.size(),
+                     FILES_MANIFEST_MAX_COUNT, static_cast<unsigned>(FILES_MANIFEST_LEVELS));
+        } else {
+            LOG_DEBUG("Got %zu files in manifest", filesManifest.size());
+        }
+    } else {
+        releaseFilesManifest(filesManifest);
+    }

    LOG_INFO("Start API client config millis=%u", millis());
    // Protect against concurrent BLE callbacks: they run in NimBLE's FreeRTOS task and also touch nodeInfoQueue.
@@ -122,8 +146,7 @@ void PhoneAPI::close()
            nodeInfoQueue.clear();
        }
        packetForPhone = NULL;
-        filesManifest.clear();
-        filesManifest.shrink_to_fit();
+        releaseFilesManifest(filesManifest);
        lastPortNumToRadio.clear();
        fromRadioNum = 0;
        config_nonce = 0;
@@ -532,7 +555,7 @@ size_t PhoneAPI::getFromRadio(uint8_t *buf)
        if (config_state == filesManifest.size() ||
            config_nonce == SPECIAL_NONCE_ONLY_NODES) { // also handles an empty filesManifest
            config_state = 0;
-            filesManifest.clear();
+            releaseFilesManifest(filesManifest);
            // Skip to complete packet
            sendConfigComplete();
        } else {