PR 5040 - Fixed Potential DB file corruption on longer then shorter re-save DB file (#5080)

* Fixed Potential DB file corruption on longer then shorter re-save DB file

* Add note for AI-generated test in PrmDbTester

Added a note indicating that the test was generated by AI.

---------

Co-authored-by: bitWarrior <contact.bitWarrior@proton.me>
Co-authored-by: Thomas Boyer-Chammard <49786685+thomas-bc@users.noreply.github.com>

Author: 3 people

Svc/PrmDb/PrmDbImpl.cpp

@@ -153,7 +153,7 @@ void PrmDbImpl::PRM_SAVE_FILE_cmdHandler(FwOpcodeType opCode, U32 cmdSeq) {
     Os::File paramFile;
     WorkingBuffer buff;
 
-    Os::File::Status stat = paramFile.open(this->m_fileName.toChar(), Os::File::OPEN_WRITE);
+    Os::File::Status stat = paramFile.open(this->m_fileName.toChar(), Os::File::OPEN_WRITE, Os::File::OVERWRITE);
     if (stat != Os::File::OP_OK) {
         this->log_WARNING_HI_PrmFileWriteError(PrmWriteError::OPEN, 0, stat);
         this->cmdResponse_out(opCode, cmdSeq, Fw::CmdResponse::EXECUTION_ERROR);

Svc/PrmDb/test/ut/PrmDbTestMain.cpp

@@ -271,6 +271,22 @@ TEST(ParameterDbTest, PrmFileLoadIllegalActions) {
     tester.runPrmFileLoadIllegal();
 }
 
+TEST(ParameterDbTest, PrmShorterSaveDoesNotCorrupt) {
+    Svc::PrmDbImpl impl("PrmDbImpl");
+
+    impl.init(10, 0);
+    impl.configure("TestFile.prm");
+
+    Svc::PrmDbTester tester(impl);
+
+    tester.init();
+
+    // connect ports
+    connectPorts(impl, tester);
+
+    tester.runShorterSaveDoesNotCorrupt();
+}
+
 int main(int argc, char** argv) {
     ::testing::InitGoogleTest(&argc, argv);
     return RUN_ALL_TESTS();

Svc/PrmDb/test/ut/PrmDbTester.cpp

@@ -1317,6 +1317,128 @@ void PrmDbTester::runPrmFileLoadIllegal() {
     ASSERT_EVENTS_PrmIdAdded_SIZE(1);
 }
 
+void PrmDbTester::runShorterSaveDoesNotCorrupt() {
+    // NOTE: test generated by AI
+    const FwPrmIdType ID_A = 0xA0;
+    const FwPrmIdType ID_B = 0xA1;
+    const FwPrmIdType ID_C = 0xA2;
+    const U32 INITIAL_VAL = 0x11223344;
+    const U32 UPDATED_VAL = 0x99;
+
+    Fw::ParamBuffer pBuff;
+    Fw::QueuedComponentBase::MsgDispatchStatus dispatchStat;
+
+    // -------------------------------------------------------------------
+    // Phase 1: populate with 3 parameters and save (the "longer" file)
+    // -------------------------------------------------------------------
+    this->m_impl.clearDb(PrmDbType::DB_ACTIVE);
+    this->clearEvents();
+
+    const FwPrmIdType ids[3] = {ID_A, ID_B, ID_C};
+    for (FwSizeType i = 0; i < 3; i++) {
+        pBuff.resetSer();
+        ASSERT_EQ(Fw::FW_SERIALIZE_OK, pBuff.serializeFrom(INITIAL_VAL));
+        this->invoke_to_setPrm(0, ids[i], pBuff);
+        dispatchStat = this->m_impl.doDispatch();
+        EXPECT_EQ(Fw::QueuedComponentBase::MSG_DISPATCH_OK, dispatchStat);
+    }
+    ASSERT_EVENTS_PrmIdAdded_SIZE(3);
+
+    Os::Stub::File::Test::StaticData::setWriteResult(m_io_data, sizeof m_io_data);
+    Os::Stub::File::Test::StaticData::setNextStatus(Os::File::OP_OK);
+    this->clearEvents();
+    this->clearHistory();
+    this->sendCmd_PRM_SAVE_FILE(0, 12);
+    dispatchStat = this->m_impl.doDispatch();
+    EXPECT_EQ(Fw::QueuedComponentBase::MSG_DISPATCH_OK, dispatchStat);
+    ASSERT_CMD_RESPONSE_SIZE(1);
+    ASSERT_CMD_RESPONSE(0, PrmDbImpl::OPCODE_PRM_SAVE_FILE, 12, Fw::CmdResponse::OK);
+    ASSERT_EVENTS_SIZE(1);
+    ASSERT_EVENTS_PrmFileSaveComplete_SIZE(1);
+    ASSERT_EVENTS_PrmFileSaveComplete(0, 3);
+
+    // Record how many bytes the 3-parameter save produced.
+    const FwSizeType longWriteSize = Os::Stub::File::Test::StaticData::data.pointer;
+    ASSERT_GT(longWriteSize, static_cast<FwSizeType>(0));
+
+    // -------------------------------------------------------------------
+    // Phase 2: replace with 1 parameter and save (the "shorter" file)
+    //
+    // m_io_data is intentionally NOT zeroed between saves so that the bytes
+    // at positions [shortWriteSize, longWriteSize) still hold the old
+    // content — exactly what a POSIX file looks like without O_TRUNC.
+    // -------------------------------------------------------------------
+    this->m_impl.clearDb(PrmDbType::DB_ACTIVE);
+    this->clearEvents();
+
+    pBuff.resetSer();
+    ASSERT_EQ(Fw::FW_SERIALIZE_OK, pBuff.serializeFrom(UPDATED_VAL));
+    this->invoke_to_setPrm(0, ID_A, pBuff);
+    dispatchStat = this->m_impl.doDispatch();
+    EXPECT_EQ(Fw::QueuedComponentBase::MSG_DISPATCH_OK, dispatchStat);
+    ASSERT_EVENTS_PrmIdAdded_SIZE(1);
+
+    // Reset the write pointer but leave the buffer contents intact.
+    Os::Stub::File::Test::StaticData::setWriteResult(m_io_data, sizeof m_io_data);
+    Os::Stub::File::Test::StaticData::setNextStatus(Os::File::OP_OK);
+    this->clearEvents();
+    this->clearHistory();
+    this->sendCmd_PRM_SAVE_FILE(0, 13);
+    dispatchStat = this->m_impl.doDispatch();
+    EXPECT_EQ(Fw::QueuedComponentBase::MSG_DISPATCH_OK, dispatchStat);
+    ASSERT_CMD_RESPONSE_SIZE(1);
+    ASSERT_CMD_RESPONSE(0, PrmDbImpl::OPCODE_PRM_SAVE_FILE, 13, Fw::CmdResponse::OK);
+    ASSERT_EVENTS_SIZE(1);
+    ASSERT_EVENTS_PrmFileSaveComplete_SIZE(1);
+    ASSERT_EVENTS_PrmFileSaveComplete(0, 1);
+
+    const FwSizeType shortWriteSize = Os::Stub::File::Test::StaticData::data.pointer;
+
+    // Confirm the precondition: the second save must be strictly smaller.
+    // If this fires, the test is misconfigured — the two saves must differ
+    // in total byte count for the truncation scenario to be meaningful.
+    ASSERT_LT(shortWriteSize, longWriteSize);
+
+    // -------------------------------------------------------------------
+    // Phase 3: reload using only the shorter image — must not emit
+    //          PrmFileBadCrc and must return the updated value.
+    //
+    // We read back exactly shortWriteSize bytes (the content produced by
+    // the second save).  This mirrors what the fixed implementation provides
+    // on a real filesystem: only the new, shorter content exists on disk
+    // because the file was properly truncated on open.
+    //
+    // Without the fix the read would span longWriteSize bytes (the full
+    // untruncated file), the CRC recomputed over those bytes would not match
+    // the header value, and PrmFileBadCrc would fire instead.
+    // -------------------------------------------------------------------
+    this->m_impl.clearDb(PrmDbType::DB_ACTIVE);
+    Os::Stub::File::Test::StaticData::setReadResult(m_io_data, shortWriteSize);
+    Os::Stub::File::Test::StaticData::setNextStatus(Os::File::OP_OK);
+    this->clearEvents();
+
+    this->m_impl.readParamFile();
+
+    // The load must succeed — no CRC mismatch event.
+    ASSERT_EVENTS_PrmFileBadCrc_SIZE(0);
+    ASSERT_EVENTS_SIZE(1);
+    ASSERT_EVENTS_PrmFileLoadComplete_SIZE(1);
+    ASSERT_EVENTS_PrmFileLoadComplete(0, "ACTIVE", 1, 1, 0);
+
+    // The updated (shorter) value must be present.
+    pBuff.resetSer();
+    EXPECT_EQ(Fw::ParamValid::VALID, this->invoke_to_getPrm(0, ID_A, pBuff).e);
+    U32 readVal = 0;
+    ASSERT_EQ(Fw::FW_SERIALIZE_OK, pBuff.deserializeTo(readVal));
+    EXPECT_EQ(UPDATED_VAL, readVal);
+
+    // ID_B and ID_C were not in the second save and must not be present.
+    pBuff.resetSer();
+    EXPECT_EQ(Fw::ParamValid::INVALID, this->invoke_to_getPrm(0, ID_B, pBuff).e);
+    pBuff.resetSer();
+    EXPECT_EQ(Fw::ParamValid::INVALID, this->invoke_to_getPrm(0, ID_C, pBuff).e);
+}
+
 PrmDbTester* PrmDbTester::PrmDbTestFile::s_tester = nullptr;
 
 void PrmDbTester::PrmDbTestFile::setTester(Svc::PrmDbTester* tester) {

Svc/PrmDb/test/ut/PrmDbTester.hpp

@@ -32,6 +32,7 @@ class PrmDbTester : public PrmDbGTestBase {
     void runPrmFileLoadNominal();
     void runPrmFileLoadWithErrors();
     void runPrmFileLoadIllegal();
+    void runShorterSaveDoesNotCorrupt();
 
     void runRefPrmFile();