piotr/edgetx-gx12
1
0
Fork 0
Code Issues 2 Pull Requests Actions Packages Projects Releases Wiki Activity

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

Browse Source 
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Peter Feerick
2026-08-27 09:13:27 +10:00
committed by GitHub
co-authored by Claude Sonnet 5
parent d219690578
commit 3562502265
2 changed files with 111 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files
radio/src/storage/modelslist.cpp
+28 -14
View File
@@ -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);
radio/src/tests/modelslist.cpp
+83
View File
@@ -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)