File uploads: check the file against the CRC16 given in ufStart

components/comms/FileStreamProtocols/FileUploadOKTOProtocol.cpp

@@ -143,6 +143,11 @@ RaftRetCode FileUploadOKTOProtocol::handleDataFrame(const RICRESTMsg& ricRESTReq
                 else
                     transferCancel("failOTAWrite");
             }
+            else if (rslt == RAFT_CRC_MISMATCH)
+            {
+                Raft::setJsonBoolResult(ricRESTReqMsg.getReq().c_str(), respMsg, false, R"("cmdName":"ufStatus","reason":"CRCMismatch")");
+                transferCancel("failCRC");
+            }
             else
             {
                 Raft::setJsonBoolResult(ricRESTReqMsg.getReq().c_str(), respMsg, false, R"("cmdName":"ufStatus","reason":"FileWriteFailed")");
@@ -213,7 +218,10 @@ void FileUploadOKTOProtocol::loop()
     else if (writeStatus != RAFT_OK)
     {
         LOG_W(MODULE_PREFIX, "loop background write failed %s", Raft::getRetCodeStr(writeStatus));
-        transferCancel(_fileStreamContentType == FILE_STREAM_CONTENT_TYPE_FIRMWARE ? "failOTAWrite" : "failFileWrite");
+        if (_fileStreamContentType == FILE_STREAM_CONTENT_TYPE_FIRMWARE)
+            transferCancel("failOTAWrite");
+        else
+            transferCancel(writeStatus == RAFT_CRC_MISMATCH ? "failCRC" : "failFileWrite");
         return;
     }
     else if (_ackDeferred)

components/comms/ProtocolExchange/FileStreamSession.cpp

@@ -15,6 +15,7 @@
 #include "RestAPIEndpointManager.h"
 #include "RICRESTMsg.h"
 #include "MiniHDLC.h"
+#include "FileSystem.h"
 #include "SpiramAwareAllocator.h"
 #include "FileSystemWorker.h"
 #include <algorithm>
@@ -596,6 +597,14 @@ RaftRetCode FileStreamSession::writeFileBlock(FileStreamBlock& fileStreamBlock)
     if (!_pFileChunker)
         return RAFT_INVALID_OPERATION;
 
+    // The CRC to check the file against (given with every block; taken from the first)
+    if (fileStreamBlock.firstBlock)
+    {
+        _crcCheck = fileStreamBlock.crc16Valid;
+        _crcExpected = fileStreamBlock.crc16;
+        _crcWritten = MiniHDLC::crcInitCCITT();
+    }
+
     // Written here (not in the background)
     if (!_writeInBackground)
     {
@@ -605,7 +614,9 @@ RaftRetCode FileStreamSession::writeFileBlock(FileStreamBlock& fileStreamBlock)
                         bytesWritten, fileStreamBlock.finalBlock);
         _totalBytes += bytesWritten;
         _totalWriteTimeUs += micros() - startUs;
-        return chunkerRslt ? RAFT_OK : RAFT_OTHER_FAILURE;
+        if (!chunkerRslt)
+            return RAFT_OTHER_FAILURE;
+        return checkWrittenCRC(fileStreamBlock.pBlock, fileStreamBlock.blockLen, fileStreamBlock.finalBlock);
     }
 
     // Add the block to the data waiting to be written
@@ -680,13 +691,16 @@ void FileStreamSession::writeJob()
         _writePendingFinal = false;
         RaftMutex_unlock(_writeMutex);
 
-        // Write it
+        // Write it (and check it against the expected CRC)
         RaftRetCode rslt = writeToFile(toWrite.data(), toWrite.size(), isFinal);
+        if (rslt == RAFT_OK)
+            rslt = checkWrittenCRC(toWrite.data(), toWrite.size(), isFinal);
         toWrite.clear();
         if (rslt != RAFT_OK)
         {
 #ifdef WARN_ON_BACKGROUND_WRITE_FAILED
-            LOG_W(MODULE_PREFIX, "writeJob write failed filename %s", _fileStreamName.c_str());
+            if (rslt != RAFT_CRC_MISMATCH)
+                LOG_W(MODULE_PREFIX, "writeJob write failed filename %s", _fileStreamName.c_str());
 #endif
             RaftMutex_lock(_writeMutex, RAFT_MUTEX_WAIT_FOREVER);
             _writeError = rslt;
@@ -695,6 +709,30 @@ void FileStreamSession::writeJob()
     }
 }
 
+/////////////////////////////////////////////////////////////////////////////////////////////////////////////////
+// Update the CRC of the data written and, after the final block, compare it with the CRC the client gave
+// (a file that doesn't match is deleted)
+/////////////////////////////////////////////////////////////////////////////////////////////////////////////////
+
+RaftRetCode FileStreamSession::checkWrittenCRC(const uint8_t* pBuf, uint32_t len, bool isFinal)
+{
+    if (!_crcCheck)
+        return RAFT_OK;
+    if (pBuf && (len > 0))
+        _crcWritten = MiniHDLC::crcUpdateCCITT(_crcWritten, pBuf, len);
+    if (!isFinal)
+        return RAFT_OK;
+    if (_crcWritten == _crcExpected)
+    {
+        _crcMatched = true;
+        return RAFT_OK;
+    }
+    LOG_W(MODULE_PREFIX, "upload CRC mismatch filename %s expected %04x written %04x - file deleted",
+                _fileStreamName.c_str(), _crcExpected, _crcWritten);
+    fileSystem.deleteFile("", _fileStreamName);
+    return RAFT_CRC_MISMATCH;
+}
+
 /////////////////////////////////////////////////////////////////////////////////////////////////////////////////
 // Write to the file in pieces (the file system lock is released between them), closing it after the final block
 /////////////////////////////////////////////////////////////////////////////////////////////////////////////////

components/comms/ProtocolExchange/FileStreamSession.h

@@ -163,6 +163,28 @@ class FileStreamSession
     void writeJob();
     RaftRetCode writeToFile(const uint8_t* pBuf, uint32_t len, bool isFinal);
 
+    // Check of an uploaded file against the CRC16 (CRC16-CCITT, as filehash reports) the client gave in ufStart:
+    // updated as the data is written, in order (on the file system worker for background writes), and compared
+    // after the final block. A mismatch fails the upload (RAFT_CRC_MISMATCH) and the file is deleted
+    bool _crcCheck = false;
+    uint16_t _crcExpected = 0;
+    uint16_t _crcWritten = 0;
+    std::atomic<bool> _crcMatched{false};
+    RaftRetCode checkWrittenCRC(const uint8_t* pBuf, uint32_t len, bool isFinal);
+
+public:
+    // The CRC16 the uploaded file was checked against - false unless it was given, the whole file has been written
+    // and it matched
+    bool getCheckedCRC(uint16_t& crc) const
+    {
+        if (!_crcMatched.load())
+            return false;
+        crc = _crcExpected;
+        return true;
+    }
+
+private:
+
     // Background reads (file downloads): a block is read on the file system worker and the next one read ahead.
     // fileStreamBlockRead() returns RAFT_BUSY until the block asked for has been read. The job sets the _read...
     // results, then clears _readJobOutstanding; the main task only uses them when no job is outstanding

components/comms/ProtocolExchange/ProtocolExchange.cpp

@@ -94,6 +94,18 @@ void ProtocolExchange::loop()
                 _lastUploadWriteStatus.push_back({pSession->getChannelID(), writeStatus});
                 if (_lastUploadWriteStatus.size() > LAST_UPLOAD_WRITE_STATUS_MAX)
                     _lastUploadWriteStatus.pop_front();
+
+                // And the CRC it was checked against, for the ufEnd reply
+                uint16_t checkedCRC = 0;
+                if (pSession->getCheckedCRC(checkedCRC))
+                {
+                    _checkedUploadCRCs.remove_if([pSession](const CheckedUploadCRC& entry) {
+                        return entry.channelID == pSession->getChannelID();
+                    });
+                    _checkedUploadCRCs.push_back({pSession->getChannelID(), pSession->getFileStreamName(), checkedCRC});
+                    if (_checkedUploadCRCs.size() > CHECKED_UPLOAD_CRCS_MAX)
+                        _checkedUploadCRCs.pop_front();
+                }
             }
 
             // Tidy up inactive session
@@ -515,10 +527,21 @@ RaftRetCode ProtocolExchange::processRICRESTCmdFrame(RICRESTMsg& ricRESTReqMsg,
         case FileStreamBase::FILE_STREAM_MSG_TYPE_UPLOAD_END:
             pSession = getFileStreamExistingSession(fileStreamName.c_str(), channelID, streamID);
             // If session already cleaned up (e.g. after final block), respond OK
-            // so the client's numbered message gets acknowledged (avoiding timeout)
+            // so the client's numbered message gets acknowledged (avoiding timeout) - with the CRC the file was
+            // checked against, if it was
             if (!pSession)
             {
-                Raft::setJsonBoolResult(ricRESTReqMsg.getReq().c_str(), respMsg, true);
+                uint16_t checkedCRC = 0;
+                if (takeCheckedUploadCRC(channelID, fileStreamName, checkedCRC))
+                {
+                    char crcJson[30];
+                    snprintf(crcJson, sizeof(crcJson), R"("crc16":"%04x")", (unsigned)checkedCRC);
+                    Raft::setJsonBoolResult(ricRESTReqMsg.getReq().c_str(), respMsg, true, crcJson);
+                }
+                else
+                {
+                    Raft::setJsonBoolResult(ricRESTReqMsg.getReq().c_str(), respMsg, true);
+                }
                 return RAFT_OK;
             }
             break;
@@ -569,7 +592,37 @@ RaftRetCode ProtocolExchange::processRICRESTCmdFrame(RICRESTMsg& ricRESTReqMsg,
     }
 
     // Session is valid so send message to it
-    return pSession->handleCmdFrame(fileStreamMsgType, ricRESTReqMsg, respMsg, endpointMsg);
+    RaftRetCode rslt = pSession->handleCmdFrame(fileStreamMsgType, ricRESTReqMsg, respMsg, endpointMsg);
+
+    // A ufEnd for a file already checked against its CRC (the session ends once the final block is written, so
+    // this is the rarer case) says so
+    uint16_t checkedCRC = 0;
+    if ((fileStreamMsgType == FileStreamBase::FILE_STREAM_MSG_TYPE_UPLOAD_END) && (rslt == RAFT_OK) &&
+                pSession->getCheckedCRC(checkedCRC))
+    {
+        char crcJson[30];
+        snprintf(crcJson, sizeof(crcJson), R"("crc16":"%04x")", (unsigned)checkedCRC);
+        Raft::setJsonBoolResult(ricRESTReqMsg.getReq().c_str(), respMsg, true, crcJson);
+    }
+    return rslt;
+}
+
+/////////////////////////////////////////////////////////////////////////////////////////////////////////////////
+// Take the CRC an upload (on this channel, of this file) was checked against, if it was
+/////////////////////////////////////////////////////////////////////////////////////////////////////////////////
+
+bool ProtocolExchange::takeCheckedUploadCRC(uint32_t channelID, const String& fileName, uint16_t& crc)
+{
+    for (auto it = _checkedUploadCRCs.begin(); it != _checkedUploadCRCs.end(); ++it)
+    {
+        if ((it->channelID == channelID) && (it->fileName == fileName))
+        {
+            crc = it->crc;
+            _checkedUploadCRCs.erase(it);
+            return true;
+        }
+    }
+    return false;
 }
 
 /////////////////////////////////////////////////////////////////////////////////////////////////////////////////

components/comms/ProtocolExchange/ProtocolExchange.h

@@ -79,6 +79,18 @@ class ProtocolExchange : public RaftSysMod
     std::list<std::pair<uint32_t, RaftRetCode>> _lastUploadWriteStatus;
     static const uint32_t LAST_UPLOAD_WRITE_STATUS_MAX = 4;
 
+    // Uploads checked against the CRC16 the client gave (by channel and file name), kept after the session ends
+    // so the ufEnd reply (which usually comes after it) can say the file was checked
+    struct CheckedUploadCRC
+    {
+        uint32_t channelID;
+        String fileName;
+        uint16_t crc;
+    };
+    std::list<CheckedUploadCRC> _checkedUploadCRCs;
+    static const uint32_t CHECKED_UPLOAD_CRCS_MAX = 4;
+    bool takeCheckedUploadCRC(uint32_t channelID, const String& fileName, uint16_t& crc);
+
     // Activity last reported through the file stream activity hook (to keep SysManager up-to-date)
     bool _activityReportedMainFWUpdate = false;
     bool _activityReportedFileSystem = false;

components/core/Utils/RaftRetCode.h

@@ -51,4 +51,5 @@ enum RaftRetCode
     RAFT_MOTION_BUSY,
     RAFT_MOTION_HOMING_REQUIRED,
     RAFT_RESP_DEFERRED,
+    RAFT_CRC_MISMATCH,
 };

components/core/Utils/RaftUtils.cpp

@@ -1528,6 +1528,7 @@ const char* Raft::getRetCodeStr(RaftRetCode retc)
         case RAFT_MOTION_NO_STEPS: return "MOTION_NO_STEPS";
         case RAFT_MOTION_BUSY: return "MOTION_BUSY";
         case RAFT_MOTION_HOMING_REQUIRED: return "MOTION_HOMING_REQUIRED";
+        case RAFT_CRC_MISMATCH: return "CRC_MISMATCH";
         default: return "UNKNOWN";
     }
 };

linux_unit_tests/test_file_upload_background.cpp

@@ -5,7 +5,9 @@
 // A FileStreamSession for a file upload hands the blocks to the file system worker (FileSystemWorker) and the
 // upload protocol acknowledges blocks only once they are written. The worker runs its jobs when the test calls
 // runPending(), so each step can be checked: blocks waiting, written, the acks held back and sent, the file closed
-// after the final block, the session ending only then, and a cancelled upload dropping what wasn't written. A file
+// after the final block, the session ending only then, and a cancelled upload dropping what wasn't written. An
+// upload given a CRC16 in ufStart is checked against it once written: a mismatch cancels it (failCRC) and deletes
+// the file, and a match is reported by getCheckedCRC() (ProtocolExchange gives it in the ufEnd reply). A file
 // download's blocks are read on the worker too (RAFT_BUSY until read), each one reading the next ahead.
 //
 /////////////////////////////////////////////////////////////////////////////////////////////////////////////////
@@ -22,6 +24,7 @@
 #include "FileStreamSession.h"
 #include "FileSystemWorker.h"
 #include "FileSystem.h"
+#include "MiniHDLC.h"
 
 static const uint32_t TEST_CHANNEL_ID = 3;
 static const uint32_t BLOCK = 100;
@@ -72,7 +75,8 @@ class Upload
     FileStreamSession* pSession = nullptr;
     String startResp;
 
-    Upload(uint32_t fileLen, const char* name)
+    // crc16Field: the CRC16 given in ufStart (e.g. "0x1234"), or nullptr for none
+    Upload(uint32_t fileLen, const char* name, const char* crc16Field = nullptr)
     {
         char cwd[512];
         path = String(getcwd(cwd, sizeof(cwd))) + "/" + name;
@@ -84,7 +88,8 @@ class Upload
                     1, "", nullptr, fileLen);
         String startJson = R"({"cmdName":"ufStart","reqStr":"fileupload","fileType":"fs","fileName":")" + path +
                     R"(","fileLen":)" + String(fileLen) + R"(,"batchMsgSize":)" + String(BLOCK) +
-                    R"(,"batchAckSize":)" + String(BATCH) + "}";
+                    R"(,"batchAckSize":)" + String(BATCH) +
+                    (crc16Field ? String(R"(,"CRC16":")") + crc16Field + "\"" : String("")) + "}";
         CommsChannelMsg msg(TEST_CHANNEL_ID, MSG_PROTOCOL_RICREST, 1, MSG_TYPE_COMMAND);
         RICRESTMsg::encode(startJson, msg, RICRESTMsg::RICREST_ELEM_CODE_COMMAND_FRAME);
         RICRESTMsg req;
@@ -98,6 +103,47 @@ class Upload
         unlink(path.c_str());
     }
 
+    // CRC16-CCITT of the contents, as filehash reports it, as "0x..."
+    String contentsCRC() const
+    {
+        char crcStr[10];
+        snprintf(crcStr, sizeof(crcStr), "0x%04x",
+                    MiniHDLC::crcUpdateCCITT(MiniHDLC::crcInitCCITT(), contents.data(), contents.size()));
+        return crcStr;
+    }
+
+    // Send ufEnd; returns the reply
+    String sendEnd()
+    {
+        String endJson = R"({"cmdName":"ufEnd","reqStr":"fileupload","fileType":"fs","fileName":")" + path +
+                    R"(","fileLen":)" + String(contents.size()) + "}";
+        CommsChannelMsg msg(TEST_CHANNEL_ID, MSG_PROTOCOL_RICREST, 2, MSG_TYPE_COMMAND);
+        RICRESTMsg::encode(endJson, msg, RICRESTMsg::RICREST_ELEM_CODE_COMMAND_FRAME);
+        RICRESTMsg req;
+        req.decode(msg.getBuf(), msg.getBufLen());
+        String resp;
+        pSession->handleCmdFrame(FileStreamBase::FILE_STREAM_MSG_TYPE_UPLOAD_END, req, resp, msg);
+        return resp;
+    }
+
+    // Upload every block, running the worker after each batch; returns the reasons of any cancels sent
+    std::vector<String> sendAll()
+    {
+        std::vector<String> cancels;
+        uint32_t numBlocks = (contents.size() + BLOCK - 1) / BLOCK;
+        for (uint32_t i = 0; i < numBlocks; i++)
+        {
+            sendBlock(i);
+            fileSystemWorker.runPending();
+            pSession->loop();
+            for (const String& msg : commsCore.sent)
+                if (RaftJson(msg).getString("cmdName", "") == "ufCancel")
+                    cancels.push_back(RaftJson(msg).getString("reason", ""));
+            commsCore.sent.clear();
+        }
+        return cancels;
+    }
+
     // Send the block at `blockIdx`; returns the okto in the reply, or -1 if there is none
     int32_t sendBlock(uint32_t blockIdx)
     {
@@ -207,6 +253,47 @@ static void testCancelDropsUnwritten()
     TEST_ASSERT(up.fileContents().size() == BLOCK, "blocks waiting at the cancel not written");
 }
 
+static void testCRCChecked()
+{
+    printf("Running testCRCChecked...\n");
+
+    // The right CRC: the upload completes and the session reports the CRC checked
+    {
+        Upload probe(12 * BLOCK - 30, "bg_upload_crc.bin");
+        String crc = probe.contentsCRC();
+        delete probe.pSession;
+        probe.pSession = nullptr;
+        Upload up(12 * BLOCK - 30, "bg_upload_crc.bin", crc.c_str());
+        uint16_t checked = 0;
+        TEST_ASSERT(!up.pSession->getCheckedCRC(checked), "not checked before it is written");
+        TEST_ASSERT(up.sendAll().empty(), "no cancel with the right CRC");
+        TEST_ASSERT(up.fileContents() == up.contents, "file written");
+        char checkedStr[10];
+        TEST_ASSERT(up.pSession->getCheckedCRC(checked), "checked once written");
+        snprintf(checkedStr, sizeof(checkedStr), "0x%04x", checked);
+        TEST_ASSERT(crc == checkedStr, "the CRC given");
+        TEST_ASSERT(RaftJson(up.sendEnd()).getString("rslt", "") == "ok", "ufEnd ok");
+    }
+
+    // A wrong CRC: cancelled once the final block is written (failCRC), and the file deleted
+    {
+        Upload up(12 * BLOCK - 30, "bg_upload_crc.bin", "0x1234");
+        std::vector<String> cancels = up.sendAll();
+        TEST_ASSERT(cancels == std::vector<String>({"failCRC"}), "cancelled with failCRC");
+        TEST_ASSERT(up.fileContents().empty(), "file deleted");
+        uint16_t checked = 0;
+        TEST_ASSERT(!up.pSession->getCheckedCRC(checked), "no checked CRC after a mismatch");
+    }
+
+    // No CRC given: no check
+    {
+        Upload up(3 * BLOCK, "bg_upload_crc.bin");
+        TEST_ASSERT(up.sendAll().empty(), "no cancel without a CRC");
+        uint16_t checked = 0;
+        TEST_ASSERT(!up.pSession->getCheckedCRC(checked), "nothing checked without a CRC");
+    }
+}
+
 static void testWorkerRunsJobsInOrder()
 {
     printf("Running testWorkerRunsJobsInOrder...\n");
@@ -282,6 +369,7 @@ int main()
     testWorkerRunsJobsInOrder();
     testBlocksWrittenByWorkerThenAcked();
     testCancelDropsUnwritten();
+    testCRCChecked();
     testDownloadBlocksReadByWorker();
     return testSummary("test_file_upload_background");
 }