Merge 7575db0214 into 07d354fa02

src/FSCommon.cpp

@@ -101,6 +101,98 @@ bool renameFile(const char *pathFrom, const char *pathTo)
 
 #include <vector>
 
+#define MAX_FILES_IN_MANIFEST 50 // Reduced to be more conservative with memory
+#define MAX_PATH_LENGTH 200      // Maximum allowed path length to prevent overflow
+
+/**
+ * @brief Helper function to validate and get file path for current platform
+ *
+ * @param file The file object
+ * @return const char* Valid path or nullptr if invalid
+ */
+static const char *getValidFilePath(File &file)
+{
+#ifdef ARCH_ESP32
+    const char *filePath = file.path();
+    return (filePath && strlen(filePath) < MAX_PATH_LENGTH) ? filePath : nullptr;
+#else
+    const char *fileName = file.name();
+    return (fileName && strlen(fileName) < MAX_PATH_LENGTH) ? fileName : nullptr;
+#endif
+}
+
+/**
+ * @brief Get the list of files in a directory (internal recursive helper).
+ *
+ * This function recursively lists files in the specified directory, subject to safety constraints
+ * to protect memory and stack usage:
+ * - Limits the total number of files collected to MAX_FILES_IN_MANIFEST (currently 50) to prevent memory overflow.
+ * - Limits the maximum allowed path length to MAX_PATH_LENGTH (currently 200) to prevent buffer overflow.
+ * - Limits recursion depth via the 'levels' parameter to avoid stack exhaustion.
+ *
+ * If any of these limits are reached, the function will stop collecting further files or recursing into subdirectories.
+ *
+ * @param dirname The name of the directory.
+ * @param levels The number of levels of subdirectories to list (recursion depth).
+ * @param filenames Reference to vector to populate with file info.
+ */
+static void getFilesRecursive(const char *dirname, uint8_t levels, std::vector<meshtastic_FileInfo> &filenames)
+{
+#ifdef FSCom
+    if (filenames.size() >= MAX_FILES_IN_MANIFEST) {
+        return; // Prevent memory overflow
+    }
+
+    File root = FSCom.open(dirname, FILE_O_READ);
+    if (!root || !root.isDirectory()) {
+        if (root)
+            root.close();
+        return;
+    }
+
+    File file = root.openNextFile();
+    while (file && filenames.size() < MAX_FILES_IN_MANIFEST) {
+        // Check if file name is valid
+        const char *fileName = file.name();
+        if (!fileName || strlen(fileName) == 0) {
+            file.close();
+            file = root.openNextFile();
+            continue;
+        }
+
+        if (file.isDirectory() && !String(fileName).endsWith(".")) {
+            if (levels > 0) { // Limit recursion depth
+                const char *validPath = getValidFilePath(file);
+                if (validPath) {
+                    getFilesRecursive(validPath, levels - 1, filenames);
+                }
+            }
+            file.close();
+        } else {
+            meshtastic_FileInfo fileInfo = {"", static_cast<uint32_t>(file.size())};
+#ifdef ARCH_ESP32
+            const char *filePath = file.path();
+            if (filePath && strlen(filePath) < sizeof(fileInfo.file_name)) {
+                strncpy(fileInfo.file_name, filePath, sizeof(fileInfo.file_name) - 1);
+                fileInfo.file_name[sizeof(fileInfo.file_name) - 1] = '\0';
+            }
+#else
+            if (strlen(fileName) < sizeof(fileInfo.file_name)) {
+                strncpy(fileInfo.file_name, fileName, sizeof(fileInfo.file_name) - 1);
+                fileInfo.file_name[sizeof(fileInfo.file_name) - 1] = '\0';
+            }
+#endif
+            if (strlen(fileInfo.file_name) > 0 && !String(fileInfo.file_name).endsWith(".")) {
+                filenames.push_back(fileInfo);
+            }
+            file.close();
+        }
+        file = root.openNextFile();
+    }
+    root.close();
+#endif
+}
+
 /**
  * @brief Get the list of files in a directory.
  *
@@ -113,42 +205,9 @@ bool renameFile(const char *pathFrom, const char *pathTo)
  */
 std::vector<meshtastic_FileInfo> getFiles(const char *dirname, uint8_t levels)
 {
-    std::vector<meshtastic_FileInfo> filenames = {};
+    std::vector<meshtastic_FileInfo> filenames;
-#ifdef FSCom
+    filenames.reserve(std::min((size_t)32, (size_t)MAX_FILES_IN_MANIFEST)); // Reserve space to reduce reallocations
-    File root = FSCom.open(dirname, FILE_O_READ);
+    getFilesRecursive(dirname, levels, filenames);
-    if (!root)
-        return filenames;
-    if (!root.isDirectory())
-        return filenames;
-
-    File file = root.openNextFile();
-    while (file) {
-        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.close();
-            }
-        } else {
-            meshtastic_FileInfo fileInfo = {"", static_cast<uint32_t>(file.size())};
-#ifdef ARCH_ESP32
-            strcpy(fileInfo.file_name, file.path());
-#else
-            strcpy(fileInfo.file_name, file.name());
-#endif
-            if (!String(fileInfo.file_name).endsWith(".")) {
-                filenames.push_back(fileInfo);
-            }
-            file.close();
-        }
-        file = root.openNextFile();
-    }
-    root.close();
-#endif
     return filenames;
 }
 

src/mesh/PhoneAPI.cpp

@@ -2,6 +2,10 @@
 #if !MESHTASTIC_EXCLUDE_GPS
 #include "GPS.h"
 #endif
+#ifdef ARCH_ESP32
+#include <esp_heap_caps.h>
+#define MIN_HEAP_FOR_FILE_MANIFEST 8192 // Minimum heap required before generating file manifest
+#endif
 
 #include "Channels.h"
 #include "Default.h"
@@ -69,10 +73,22 @@ void PhoneAPI::handleStartConfig()
         state = STATE_SEND_MY_INFO;
     }
     pauseBluetoothLogging = true;
-    spiLock->lock();
+
-    filesManifest = getFiles("/", 10);
+    // Check available heap before getting files to prevent crash
-    spiLock->unlock();
+#ifdef ARCH_ESP32
-    LOG_DEBUG("Got %d files in manifest", filesManifest.size());
+    size_t freeHeap = ESP.getFreeHeap();
+    if (freeHeap < MIN_HEAP_FOR_FILE_MANIFEST) {
+        LOG_WARN("Low memory (%d bytes), skipping file manifest", freeHeap);
+        filesManifest.clear();
+    } else {
+#endif
+        spiLock->lock();
+        filesManifest = getFiles("/", 10);
+        spiLock->unlock();
+        LOG_DEBUG("Got %d files in manifest", filesManifest.size());
+#ifdef ARCH_ESP32
+    }
+#endif
 
     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.

test_memory_fix/platformio.ini

@@ -0,0 +1,9 @@
+[env:memory_fix_test]
+platform = native
+framework = 
+test_framework = unity
+build_flags = 
+    -std=c++17
+lib_deps = 
+    throwtheswitch/Unity@^2.5.2
+build_src_filter = +<*> -<../src/> -<../test/test_crypto/> -<../test/test_meshpacket_serializer/> -<../test/test_mqtt/> -<../test/test_serial/>

test_memory_fix/test/test_memory/test_main.cpp

@@ -0,0 +1,63 @@
+#include <cstdio>
+#include <cstring>
+#include <unity.h>
+#include <vector>
+
+struct meshtastic_FileInfo {
+    char file_name[256];
+    uint32_t size_bytes;
+};
+
+std::vector<meshtastic_FileInfo> mock_getFiles(const char *dirname, uint8_t levels, size_t max_files = 50)
+{
+    std::vector<meshtastic_FileInfo> files;
+    files.reserve(std::min((size_t)32, max_files));
+
+    if (strcmp(dirname, "/nonexistent") == 0) {
+        return files;
+    }
+
+    size_t file_count = std::min((size_t)10, max_files);
+    for (size_t i = 0; i < file_count && files.size() < max_files; i++) {
+        meshtastic_FileInfo info = {"", 100};
+        snprintf(info.file_name, sizeof(info.file_name), "/file%zu.txt", i);
+        files.push_back(info);
+    }
+
+    return files;
+}
+
+bool mock_check_memory_limit(size_t free_heap, size_t min_required = 8192)
+{
+    return free_heap >= min_required;
+}
+
+void test_file_limit()
+{
+    std::vector<meshtastic_FileInfo> files = mock_getFiles("/", 10, 50);
+    TEST_ASSERT_TRUE(files.size() <= 50);
+}
+
+void test_empty_directory()
+{
+    std::vector<meshtastic_FileInfo> files = mock_getFiles("/nonexistent", 1);
+    TEST_ASSERT_EQUAL(0, files.size());
+}
+
+void test_memory_protection()
+{
+    TEST_ASSERT_FALSE(mock_check_memory_limit(4096));
+    TEST_ASSERT_TRUE(mock_check_memory_limit(16384));
+}
+
+void setUp(void) {}
+void tearDown(void) {}
+
+int main()
+{
+    UNITY_BEGIN();
+    RUN_TEST(test_file_limit);
+    RUN_TEST(test_empty_directory);
+    RUN_TEST(test_memory_protection);
+    return UNITY_END();
+}