fix(storage): correctness bugs in ModelMap::writeModelLabels

radio/src/storage/modelslist.cpp

@@ -714,7 +714,7 @@ bool ModelMap::renameLabel(const std::string &from, std::string to,
       memcpy(g_model.header.labels, partial.header.labels, LABELS_LENGTH);
       storageDirty(EE_MODEL);
     } else {
-      fault = writeModelLabels(modcell, partial.header.labels);
+      fault = !writeModelLabels(modcell, partial.header.labels);
     }
 #if defined(SIMU)
     sleep_ms(100);
@@ -855,12 +855,16 @@ bool ModelMap::writeModelLabels(ModelCell* cell, const char* labels)
     return false;
   }
 
+  // Bound searches/writes below by bytes actually read, not sizeof(buf) -
+  // the rest of buf is uninitialized for a file shorter than 512 bytes.
+  int len = (int)bytes_cnt;
+
   // Find header section
   int n = 0;
-  while (n < sizeof(buf) - 7 && strncmp(&buf[n], "header:", 7) != 0)
+  while (n < len - 7 && strncmp(&buf[n], "header:", 7) != 0)
     n += 1;
 
-  if (n >= sizeof(buf) - 7) {
+  if (n >= len - 7) {
     TRACE("ERROR model header not found in %s", cell->modelFilename);
     f_close(&out);
     f_close(&file);
@@ -871,31 +875,38 @@ bool ModelMap::writeModelLabels(ModelCell* cell, const char* labels)
   // Skip header section - look for next section after 'header:'
   do {
     // Skip current line
-    while (n < sizeof(buf) && buf[n] != '\n') n += 1;
+    while (n < len && buf[n] != '\n') n += 1;
     n += 1;
-  } while ((n < sizeof(buf)) && buf[n] == ' ');
+  } while ((n < len) && buf[n] == ' ');
 
-  if (n >= sizeof(buf)) {
+  if (n >= len) {
     TRACE("ERROR could not match model header in %s", cell->modelFilename);
     f_close(&out);
     f_close(&file);
     f_unlink(tempPath);
     return false;
   }
 
-  // Write remainder of first buffer after header
-  result = f_write(&out, &buf[n], sizeof(buf) - n, &bytes_cnt);
+  // Write remainder of first buffer after header - check for short
+  // writes (e.g. SD full), which f_write() can return FR_OK for.
+  UINT written;
+  UINT to_write = (UINT)(len - n);
+  result = f_write(&out, &buf[n], to_write, &written);
+  bool short_write = (result == FR_OK && written != to_write);
+
   // Block copy the rest of the original file to the temp file
-  while (result == FR_OK && bytes_cnt != 0) {
+  while (result == FR_OK && !short_write && bytes_cnt != 0) {
     result = f_read(&file, buf, sizeof(buf), &bytes_cnt);
-    if (result == FR_OK && bytes_cnt != 0)
-      result = f_write(&out, buf, bytes_cnt, &bytes_cnt);
+    if (result == FR_OK && bytes_cnt != 0) {
+      result = f_write(&out, buf, bytes_cnt, &written);
+      short_write = (result == FR_OK && written != bytes_cnt);
+    }
   }
 
   f_close(&out);
   f_close(&file);
 
-  if (result != FR_OK) {
+  if (result != FR_OK || short_write) {
     TRACE("ERROR copying to temp file");
     f_unlink(tempPath);
     return false;
@@ -904,7 +915,10 @@ bool ModelMap::writeModelLabels(ModelCell* cell, const char* labels)
   // Delete original file and rename temp file
   getModelPath(buf, cell->modelFilename);
   f_unlink(buf);
-  f_rename(tempPath, buf);
+  if (f_rename(tempPath, buf) != FR_OK) {
+    TRACE("ERROR renaming temp file to %s", cell->modelFilename);
+    return false;
+  }
 
   return true;
 }
@@ -930,7 +944,7 @@ bool ModelMap::updateModelFile(ModelCell *cell)
     return false;
   }
 
-  bool fault = writeModelLabels(cell, ModelMap::toCSV(getLabelsByModel(cell)).c_str());
+  bool fault = !writeModelLabels(cell, ModelMap::toCSV(getLabelsByModel(cell)).c_str());
 
 #if defined(DEBUG_TIMERS)
   DEBUG_TIMER_SAMPLE(debugTimerYamlScan);

radio/src/tests/modelslist.cpp

@@ -164,3 +164,86 @@ TEST(PartialModel, LabelsFieldRetainsFullCsvUpToItsOwnCapacity)
          "worth.";
 }
 #endif // defined(STORAGE_MODELSLIST)
+
+// Real FatFS round-trip tests for ModelMap::writeModelLabels() itself -
+// the raw file-surgery function these PartialModel tests don't exercise.
+#if defined(SIMU) && defined(STORAGE_MODELSLIST)
+
+#include "storage/modelslist.h"
+#include "storage/sdcard_common.h"
+
+#include <filesystem>
+#include <fstream>
+
+static void writeRawModelFile(const char* modelFilename, const std::string& content)
+{
+  char path[256];
+  getModelPath(path, modelFilename);
+  std::filesystem::create_directories(
+      std::filesystem::path(simuFatfsGetRealPath(path)).parent_path());
+  std::ofstream f(simuFatfsGetRealPath(path), std::ios::binary);
+  f << content;
+}
+
+static size_t modelFileSize(const char* modelFilename)
+{
+  char path[256];
+  getModelPath(path, modelFilename);
+  return std::filesystem::file_size(simuFatfsGetRealPath(path));
+}
+
+// writeModelLabels() locates "header:" then raw-copies everything from the
+// next unindented line onward, so a top-level "body:" key after the header
+// is enough to pin that boundary without needing a real, schema-valid
+// model file.
+TEST(ModelsList, WriteModelLabelsPreservesBodySize)
+{
+  ModelMap map;
+
+  const char* fileA = "wml_test_a.yml";
+  const char* fileB = "wml_test_b.yml";
+
+  // Both fixtures are well under 512 bytes (a single short f_read/EOF), and
+  // differ only in body length. writeModelLabels() must reproduce that
+  // exact size difference in its output - if it instead always pads the
+  // first chunk out to sizeof(buf), both outputs collapse to the same size
+  // regardless of the real body length (the uninitialized-memory bug).
+  std::string bodyA = "body:\n  marker: AAAA\n";
+  std::string bodyB = bodyA + std::string(300, 'X');
+  ASSERT_LT(bodyA.size() + 40, 512u);
+  ASSERT_LT(bodyB.size() + 40, 512u);
+
+  writeRawModelFile(fileA, "header:\n  name: OldName\n  labels: \n" + bodyA);
+  writeRawModelFile(fileB, "header:\n  name: OldName\n  labels: \n" + bodyB);
+
+  ModelCell cellA(fileA);
+  ModelCell cellB(fileB);
+
+  EXPECT_TRUE(map.writeModelLabels(&cellA, "NewLabel"));
+  EXPECT_TRUE(map.writeModelLabels(&cellB, "NewLabel"));
+
+  size_t sizeA = modelFileSize(fileA);
+  size_t sizeB = modelFileSize(fileB);
+
+  // The two headers are generated from identical inputs, so the size
+  // delta between the outputs must equal the body length delta exactly.
+  EXPECT_EQ(sizeB - sizeA, bodyB.size() - bodyA.size());
+
+  std::filesystem::remove(simuFatfsGetRealPath(std::string(MODELS_PATH) + "/" + fileA));
+  std::filesystem::remove(simuFatfsGetRealPath(std::string(MODELS_PATH) + "/" + fileB));
+}
+
+TEST(ModelsList, WriteModelLabelsFailsCleanlyWithoutHeader)
+{
+  ModelMap map;
+  const char* file = "wml_test_noheader.yml";
+
+  writeRawModelFile(file, "notheader:\n  foo: bar\n");
+  ModelCell cell(file);
+
+  EXPECT_FALSE(map.writeModelLabels(&cell, "NewLabel"));
+
+  std::filesystem::remove(simuFatfsGetRealPath(std::string(MODELS_PATH) + "/" + file));
+}
+
+#endif  // #if defined(SIMU) && defined(STORAGE_MODELSLIST)