feat: Reduce risk of corrupt radio.yml (#1481)

* reduce risk of corrupt radio.yml #1412

Adds CRC16 checksum to radio.yml when saved.
Adds save to temporary file, folowed by rename to radio.yml
Adds check on load, with fall-back to temporary file

* Made sure that only the first read block of radio.yml is checked for the 'checksum' value.
Added checksum generation on radio.yml produced by conversion.

* Added namespace.
Github actions triggered build failed on this - compiler difference.

* Fixed typo in code not run locally.

* Made checksum result code a enum class.
Fixed null-poionter dereference bug.

* Added check and handling for radio.yml file not read (will be handled as if checksum failed)
Added evaluation of checksum when radio_new.yml is read.
Added f_unlink() calls before f_rename() to ensure the target filename does not exist.

* Added radio.yml cheksum handling to Simulator/Companion.
Cleaned radio firmware handling of checksum field up to be newline agnostic.

* Added handling of files without checksum.
- Letting files pass the check if no checksum field is present (old files) and the size is realistic (25 bytes)

* Fixed bug causing new conversion from binary if no radio.yml BUT a radio_new.yaml
is present in the filesystem.

Now the conversion is only performed if neither file exsits.

* repair rebase problems

* rerun yaml generator

* fix another rebase problem

* update text in companion to be more meaningful when the radio.yml file has a checksum error
fix wrong error message in trace

* add trigger to check radio config when leavin menu_radio

* fix: add handling for temporary and error yaml radio settings files to simu/simulator/libsimulator

* fix corner cases when comverting non crc-ed files

* chore: Tweak radio data warnings for screen sizes
- Changed the define to be a little shorter
- Added missing define for TW
- Three string lengths to suit 128, 212 and basically colorlcd

Co-authored-by: gagarinlg <malte@langermann.net>
Co-authored-by: Peter Feerick <peter.feerick@gmail.com>

radio/src/storage/sdcard_yaml.cpp

@@ -30,6 +30,8 @@
 #include "yaml/yaml_tree_walker.h"
 #include "yaml/yaml_parser.h"
 #include "yaml/yaml_datastructs.h"
+#include "yaml/yaml_bits.h"
+
 
 #if defined(EEPROM_RLC)
  #include "storage/eeprom_common.h"
@@ -38,32 +40,85 @@
 
 #include "storage/conversions/conversions.h"
 
-const char * readYamlFile(const char* fullpath, const YamlParserCalls* calls, void* parser_ctx)
+const char * readYamlFile(const char* fullpath, const YamlParserCalls* calls, void* parser_ctx, ChecksumResult* checksum_result)
 {
     FIL  file;
     UINT bytes_read;
+    UINT total_bytes = 0;
 
     FRESULT result = f_open(&file, fullpath, FA_OPEN_EXISTING | FA_READ);
     if (result != FR_OK) {
         return SDCARD_ERROR(result);
     }
-      
+
     YamlParser yp; //TODO: move to re-usable buffer
     yp.init(calls, parser_ctx);
 
-    char buffer[32];
-    while (f_read(&file, buffer, sizeof(buffer), &bytes_read) == FR_OK) {
+    uint16_t calculated_checksum = 0xFFFF;
+    uint16_t file_checksum = 0;
 
-      // reached EOF?
-      if (bytes_read == 0)
+    bool first_block = true;
+    char buffer[32];
+    while (f_read(&file, buffer, sizeof(buffer)-1, &bytes_read) == FR_OK) {
+      if (bytes_read == 0)  // EOF
         break;
-      
+      total_bytes += bytes_read;
+
+      uint16_t skip = 0;
+      if(first_block) {
+        // Get the 'checksum' value and skip from further YAML processing
+        // The checksum must be first in the first buffer read from file
+        first_block = false;
+        const char *skipValue = "checksum: ";
+        if(strncmp(buffer, skipValue, strlen(skipValue)) == 0) {
+          skip = 10;
+          char* startPos = buffer + strlen(skipValue);
+          char* endPos = startPos;
+          // Advance through the value
+          while((*endPos != '\r') && (*endPos != '\n')) {
+            if (endPos > buffer + bytes_read) {
+              return SDCARD_ERROR(	FR_INT_ERR );
+            }
+            endPos++;
+          }
+          // Skip trailing newline
+          while((*endPos == '\r') || (*endPos == '\n')) {
+            *endPos = 0;
+            endPos++;
+          }
+
+          file_checksum = atoi(startPos);
+          skip = endPos - buffer;
+        }
+      }
+
+      // Calculate checksum on read block only if we are called with a pointer to write the resulting checksum
+      if (checksum_result != NULL) {
+        calculated_checksum = crc16(0, (const uint8_t *)buffer + skip, bytes_read - skip, calculated_checksum);
+      }
+
       if (f_eof(&file)) yp.set_eof();
-      if (yp.parse(buffer, bytes_read) != YamlParser::CONTINUE_PARSING)
+      if (yp.parse(buffer + skip, bytes_read - skip) != YamlParser::CONTINUE_PARSING)
         break;
     }
-
     f_close(&file);
+
+    if (checksum_result != NULL) {
+      // Special case to handle "old" files with no checksum field
+      // 25 was arbitrarily chosen as the minimum realistic file size
+      // - The issue is to allow old files to pass, while still detecting garbled files
+      if ( (file_checksum == 0) && (total_bytes > 25) ) {
+        *checksum_result = ChecksumResult::Success;
+      } else {
+        // Normal case - compare read and calculated checksum
+        if (calculated_checksum == file_checksum) {
+          *checksum_result = ChecksumResult::Success;
+        } else {
+          *checksum_result = ChecksumResult::Failed;
+        }
+      }
+    }
+
     return NULL;
 }
 
@@ -92,21 +147,57 @@ void storageCreateModelsList()
 // SDCARD storage interface
 //
 
+static const char * attemptLoad(const char *filename, ChecksumResult* checksum_status)
+{
+  YamlTreeWalker tree;
+  tree.reset(get_radiodata_nodes(), (uint8_t*)&g_eeGeneral);
+  return readYamlFile(filename, YamlTreeWalker::get_parser_calls(), &tree, checksum_status);
+}
+
 const char * loadRadioSettingsYaml()
 {
     // YAML reader
     TRACE("YAML radio settings reader");
 
-    YamlTreeWalker tree;
-    tree.reset(get_radiodata_nodes(), (uint8_t*)&g_eeGeneral);
+    ChecksumResult checksum_status;
+    const char* p = attemptLoad(RADIO_SETTINGS_YAML_PATH, &checksum_status);
 
-    return readYamlFile(RADIO_SETTINGS_YAML_PATH, YamlTreeWalker::get_parser_calls(), &tree);
+    if((p != NULL) || (checksum_status != ChecksumResult::Success) ) {
+      // Read failed or checksum check failed
+      FRESULT result = FR_OK;
+      TRACE("radio settings: Reading failed");
+      if ( (p == NULL) && g_eeGeneral.manuallyEdited) {
+        // Read sussessfull, checksum failed, manuallyEdited set
+        TRACE("File has been manually edited - ignoring checksum mismatch");
+        g_eeGeneral.manuallyEdited = 0;
+        storageDirty(EE_GENERAL);   // Trigger a save on sucessfull recovery
+      } else {
+        TRACE("File is corrupted, attempting alternative file");
+        f_unlink(RADIO_SETTINGS_ERRORFILE_YAML_PATH);
+        result = f_rename(RADIO_SETTINGS_YAML_PATH, RADIO_SETTINGS_ERRORFILE_YAML_PATH); // Save corrupted file for later analysis
+        p = attemptLoad(RADIO_SETTINGS_TMPFILE_YAML_PATH, &checksum_status);
+        if (p == NULL && (checksum_status == ChecksumResult::Success)) {
+            f_unlink(RADIO_SETTINGS_YAML_PATH);
+            result = f_rename(RADIO_SETTINGS_TMPFILE_YAML_PATH, RADIO_SETTINGS_YAML_PATH);  // Rename previously saved file to active file
+            if (result != FR_OK) {
+              ALERT(STR_STORAGE_WARNING, TR_RADIO_DATA_UNRECOVERABLE, AU_BAD_RADIODATA);
+              return SDCARD_ERROR(result);
+            }
+        }
+        TRACE("Unable to recover radio data");
+        ALERT(STR_STORAGE_WARNING, p == NULL ? TR_RADIO_DATA_RECOVERED : TR_RADIO_DATA_UNRECOVERABLE, AU_BAD_RADIODATA);
+      }
+    }
+    return p;
 }
 
 const char * loadRadioSettings()
 {
     FILINFO fno;
-    if (f_stat(RADIO_SETTINGS_YAML_PATH, &fno) != FR_OK) {
+
+    if ( (f_stat(RADIO_SETTINGS_YAML_PATH, &fno) != FR_OK) && ((f_stat(RADIO_SETTINGS_TMPFILE_YAML_PATH, &fno) != FR_OK)) ) {
+      // If neither the radio configuraion YAML file or the temporary file generated on write exist, this must be a first run with YAML support.
+      // - thus requiring a conversion from binary to YAML.
 #if STORAGE_CONVERSIONS < 221
 #if defined(STORAGE_MODELSLIST)
       uint8_t version;
@@ -142,6 +233,47 @@ const char * loadRadioSettings()
     return error;
 }
 
+
+
+struct yaml_checksummer_ctx {
+    FRESULT result;
+    uint16_t checksum;
+    bool checksum_invalid;
+};
+
+static bool yaml_checksummer(void* opaque, const char* str, size_t len)
+{
+    yaml_checksummer_ctx* ctx = (yaml_checksummer_ctx*)opaque;
+
+    ctx->checksum = crc16(0, (const uint8_t *) str, len, ctx->checksum);
+    return true;
+}
+
+bool YamlFileChecksum(const YamlNode* root_node, uint8_t* data, uint16_t* checksum)
+{
+    YamlTreeWalker tree;
+    tree.reset(root_node, data);
+
+    yaml_checksummer_ctx ctx;
+    ctx.result = FR_OK;
+    ctx.checksum = 0xFFFF;
+    ctx.checksum_invalid = false;
+
+    if (!tree.generate(yaml_checksummer, &ctx)) {
+        if (ctx.result != FR_OK) {
+          ctx.checksum_invalid = true;
+          return false;
+        }
+    }
+
+    if(checksum != NULL) {
+      *checksum = ctx.checksum;
+    }
+
+    return true;
+}
+
+
 struct yaml_writer_ctx {
     FIL*    file;
     FRESULT result;
@@ -160,7 +292,7 @@ static bool yaml_writer(void* opaque, const char* str, size_t len)
     return (ctx->result == FR_OK) && (bytes_written == len);
 }
 
-const char* writeFileYaml(const char* path, const YamlNode* root_node, uint8_t* data)
+const char* writeFileYaml(const char* path, const YamlNode* root_node, uint8_t* data, uint16_t checksum)
 {
     FIL file;
 
@@ -168,14 +300,24 @@ const char* writeFileYaml(const char* path, const YamlNode* root_node, uint8_t*
     if (result != FR_OK) {
         return SDCARD_ERROR(result);
     }
-      
     YamlTreeWalker tree;
     tree.reset(root_node, data);
 
     yaml_writer_ctx ctx;
     ctx.file = &file;
     ctx.result = FR_OK;
-    
+
+    // Try to add CRC
+    if (checksum != 0) {
+      if (!yaml_writer(&ctx, YAMLFILE_CHECKSUM_TAG_NAME, strlen(YAMLFILE_CHECKSUM_TAG_NAME))) return NULL;
+      if (!yaml_writer(&ctx, ": ", 2)) return SDCARD_ERROR(FR_INVALID_PARAMETER);
+      const char* p_out = NULL;
+      p_out = yaml_unsigned2str((int)checksum);
+      if (p_out && !yaml_writer(&ctx, p_out, strlen(p_out))) return SDCARD_ERROR(FR_INVALID_PARAMETER);
+      yaml_writer(&ctx, "\r\n", 2);
+    }
+
+
     if (!tree.generate(yaml_writer, &ctx)) {
         if (ctx.result != FR_OK) {
             f_close(&file);
@@ -190,8 +332,25 @@ const char* writeFileYaml(const char* path, const YamlNode* root_node, uint8_t*
 const char * writeGeneralSettings()
 {
     TRACE("YAML radio settings writer");
-    return writeFileYaml(RADIO_SETTINGS_YAML_PATH, get_radiodata_nodes(),
-                         (uint8_t*)&g_eeGeneral);
+    uint16_t file_checksum = 0;
+
+    YamlFileChecksum(get_radiodata_nodes(), (uint8_t*)&g_eeGeneral, &file_checksum);
+    g_eeGeneral.manuallyEdited = false;
+
+    const char *p = writeFileYaml(RADIO_SETTINGS_TMPFILE_YAML_PATH, get_radiodata_nodes(),
+                         (uint8_t*)&g_eeGeneral, file_checksum);
+    TRACE("generalSettings written with checksum %u", file_checksum);
+
+    if (p != NULL) {
+        return p;
+    }
+    f_unlink(RADIO_SETTINGS_YAML_PATH);
+  
+    FRESULT result = f_rename(RADIO_SETTINGS_TMPFILE_YAML_PATH, RADIO_SETTINGS_YAML_PATH);
+    if(result != FR_OK)
+        return SDCARD_ERROR(result);
+
+    return nullptr;
 }
 
 
@@ -213,7 +372,7 @@ const char * readModelYaml(const char * filename, uint8_t * buffer, uint32_t siz
         TRACE("cannot find YAML data nodes for object size (size=%d)", size);
         return "YAML size error";
     }
-    
+
     char path[256];
     getModelPath(path, filename, pathName);
 
@@ -241,7 +400,7 @@ const char * readModelYaml(const char * filename, uint8_t * buffer, uint32_t siz
       // md->swashR.elevatorWeight   = 100;
     }
 
-    return readYamlFile(path, YamlTreeWalker::get_parser_calls(), &tree);
+    return readYamlFile(path, YamlTreeWalker::get_parser_calls(), &tree, NULL);
 }
 
 static const char _wrongExtentionError[] = "wrong file extension";
@@ -261,7 +420,7 @@ const char * writeModelYaml(const char* filename)
     TRACE("YAML model writer");
     char path[256];
     getModelPath(path, filename);
-    return writeFileYaml(path, get_modeldata_nodes(), (uint8_t*)&g_model);
+    return writeFileYaml(path, get_modeldata_nodes(), (uint8_t*)&g_model,0 );
 }
 
 #if !defined(STORAGE_MODELSLIST)
@@ -329,7 +488,7 @@ bool copyModel(uint8_t dst, uint8_t src)
   char model_idx_dst[MODELIDX_STRLEN];
   getModelNumberStr(src, model_idx_src);
   getModelNumberStr(dst, model_idx_dst);
-  
+
   GET_FILENAME(fname_src, MODELS_PATH, model_idx_src, YAML_EXT);
   GET_FILENAME(fname_dst, MODELS_PATH, model_idx_dst, YAML_EXT);
 
@@ -350,7 +509,7 @@ void swapModels(uint8_t id1, uint8_t id2)
   char model_idx_2[MODELIDX_STRLEN];
   getModelNumberStr(id1, model_idx_1);
   getModelNumberStr(id2, model_idx_2);
-  
+
   GET_FILENAME(fname1, MODELS_PATH, model_idx_1, YAML_EXT);
   GET_FILENAME(fname1_tmp, MODELS_PATH, model_idx_1, ".tmp");
   GET_FILENAME(fname2, MODELS_PATH, model_idx_2, YAML_EXT);
@@ -451,7 +610,7 @@ const char * backupModel(uint8_t idx)
   char model_idx[MODELIDX_STRLEN + sizeof(YAML_EXT)];
   getModelNumberStr(idx, model_idx);
   strcat(model_idx, STR_YAML_EXT);
-  
+
   return sdCopyFile(model_idx, STR_MODELS_PATH, buf, STR_BACKUP_PATH);
 }