diff --git a/radio/src/storage/modelslist.cpp b/radio/src/storage/modelslist.cpp index a3681c544b2..f11b458493b 100644 --- a/radio/src/storage/modelslist.cpp +++ b/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,11 +875,11 @@ 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); @@ -883,19 +887,26 @@ bool ModelMap::writeModelLabels(ModelCell* cell, const char* labels) 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); diff --git a/radio/src/tests/modelslist.cpp b/radio/src/tests/modelslist.cpp index 1d2bb39884a..3f23eea4f8e 100644 --- a/radio/src/tests/modelslist.cpp +++ b/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 +#include + +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)