Issue 4270 add crc chk to prm db #4586

odacindy-fprime · 2025-12-20T00:18:21Z


*Related Issue(s)*
*Has Unit Tests (y/n)*
*Documentation Included (y/n)*
*Generative AI was used in this contribution (y/n)*

Change Description

Save and read a CRC to validate the contents of the PrmDb file.

Rationale

Provides protection against corruption

Testing/Review Recommendations

made minor updates to TestFile::open, seek, read, and write in order track to the StaticData::data.positionResult, which is used by TestFile::position() to return the current position of the pointer in the paramFile.
added file CRC to the beginning of the paramFile
added a few unit test cases to check CRC_SIZE, CRC, and CRC_BUFFER on PrmFileReadError, and CRC_PLACE on PrmWriteError; and PrmFileBadCrc

Future Work

none

AI Usage (see policy)

…uns, but without changing the existing unit tests, when I run them, some are now breaking, so will checkpoint now before making changes to existing unit tests

- Change the type for the buff argument in the computeCrc() declaration from "co nst U8*" to "const BYTE*" Svc/PrmDb/test/ut/PrmDbTester.cpp - Update PrmDbTester::runFileReadError() for the following: - add a test case for CRC_SIZE when reading the paramFile. - add test cases for CRC and CRC_BUFFER for a failed paramFile read due to the file not existing and the file not being opened - add a test case for FILE_DATA_ERROR having a bad file CRC when reading a paramFile - Update PrmDbTester::runFileWriteError() for the following: - add test cases for a write failures for CRC when sending the PRM_SAVE_FILE command. - Update PrmDbTester::PrmDbTestFile::read()'s FILE_SIZE_ERROR case, so that it only increments size (we increment the size to introduce a size error on a paramFile read, which is used for several unit test cases) if size is not zero. If we don't do this, then the unit test will go into an infinite loop, since we may loop multiple times when reading the CRC buffer (which is part of the paramFile upon which we calculate the CRC. In other words the CRC buffer is the paramFile minus the CRC). The CRC buffer contents contain the param database entries.

- add CURR_POSITION, SEEK_ZERO, and SEEK_POSITION to PrmWriteError enum - added return status checking for paramFile.seek() and paramFile.position() calls in PrmDbImpl::PRM_SAVE_FILE_cmdHandler()

Svc/PrmDb/PrmDbImpl.cpp


+U32 PrmDbImpl::computeCrc(U32 crc, const BYTE* buff, FwSizeType size) {
+    for (FwSizeType byte = 0; byte < size; byte++) {
+        crc = static_cast<U32>(update_crc_32(crc, static_cast<char>(buff[byte])));


Svc/PrmDb/PrmDbImpl.cpp


+U32 PrmDbImpl::computeCrc(U32 crc, const BYTE* buff, FwSizeType size) {
+    for (FwSizeType byte = 0; byte < size; byte++) {
+        crc = static_cast<U32>(update_crc_32(crc, static_cast<char>(buff[byte])));


Svc/PrmDb/PrmDbImpl.cpp

+    for (FwSizeType byte = 0; byte < size; byte++) {
+        crc = static_cast<U32>(update_crc_32(crc, static_cast<char>(buff[byte])));
+    }   
+    return crc;


Svc/PrmDb/PrmDbImpl.cpp

+    stat = paramFile.write(reinterpret_cast<const U8*>(&crc), writeSize, Os::File::WaitType::WAIT);
+    if (stat != Os::File::OP_OK) {
+        this->log_WARNING_HI_PrmFileWriteError(PrmWriteError::CRC_REAL, 0, stat);
+        this->cmdResponse_out(opCode, cmdSeq, Fw::CmdResponse::EXECUTION_ERROR);


Svc/PrmDb/PrmDbImpl.cpp

+    }
+
+    // Restore pointer to end of paramFile
+    paramFile.seek(static_cast<FwSignedSizeType>(currPosInParamFile), Os::File::SeekType::ABSOLUTE);


Svc/PrmDb/PrmDbImpl.cpp

+    paramFile.seek(static_cast<FwSignedSizeType>(currPosInParamFile), Os::File::SeekType::ABSOLUTE);
+    if (stat != Os::File::OP_OK) {
+        this->log_WARNING_HI_PrmFileWriteError(PrmWriteError::SEEK_POSITION, 0, stat);
+        this->cmdResponse_out(opCode, cmdSeq, Fw::CmdResponse::EXECUTION_ERROR);


Svc/PrmDb/PrmDbImpl.cpp

+    paramFile.seek(static_cast<FwSignedSizeType>(currPosInParamFile), Os::File::SeekType::ABSOLUTE);
+    if (stat != Os::File::OP_OK) {
+        this->log_WARNING_HI_PrmFileWriteError(PrmWriteError::SEEK_POSITION, 0, stat);
+        this->cmdResponse_out(opCode, cmdSeq, Fw::CmdResponse::EXECUTION_ERROR);


Svc/PrmDb/PrmDbImpl.cpp

+    while (readSize != 0) {
+        readSize = PRMDB_CRC_BUFFER_SIZE;
+        Os::File::Status fStat = paramFile.read(this->m_crcBuffer, readSize, Os::File::NO_WAIT);
+        if (fStat != Os::File::OP_OK) {
+            this->log_WARNING_HI_PrmFileReadError(PrmReadError::CRC_BUFFER, static_cast<I32>(crcChunk), fStat);
+            return  PrmLoadStatus::ERROR;
+
+        }
+
+        crc = this->computeCrc(crc, this->m_crcBuffer, readSize);
+
+        crcChunk++;
+    }


Svc/PrmDb/PrmDbImpl.cpp

+    // seek to beginning and write CRC value
+    paramFile.seek(0, Os::File::SeekType::ABSOLUTE);
+    if (stat != Os::File::OP_OK) {
+        this->log_WARNING_HI_PrmFileWriteError(PrmWriteError::SEEK_ZERO, 0, stat);


Svc/PrmDb/PrmDbImpl.cpp

+    // seek to beginning and write CRC value
+    paramFile.seek(0, Os::File::SeekType::ABSOLUTE);
+    if (stat != Os::File::OP_OK) {
+        this->log_WARNING_HI_PrmFileWriteError(PrmWriteError::SEEK_ZERO, 0, stat);


LeStarch

Minor change to the CRC calculation. Hopefully the tests will make this switch easy.

LeStarch · 2025-12-20T00:32:59Z

Svc/PrmDb/PrmDbImpl.cpp

+    U32 crc = 0xFFFFFFFF;
+    // read into CRC buffer for checking
+
+    while (readSize != 0) {


We should probably avoid this unbounded while loop. I think this whole section could be replaced with:

FwSizeType crcChunk = 0; U32 crc = 0xFFFFFFFF; Os::File::Status status = paramFile.calculateCrc(crc);

As I believe that function is designed to calculate the CRC from the current position onward.

LeStarch · 2025-12-20T00:33:17Z

Svc/PrmDb/PrmDbImpl.cpp

+        crcChunk++;
+    }
+
+    if (fileCrc != crc) {


I'd then pick back up here.

LeStarch · 2025-12-20T00:35:22Z

Svc/PrmDb/PrmDbImpl.hpp

        }
    };

+    U8 m_crcBuffer[PRMDB_CRC_BUFFER_SIZE];  //!< working buffer for computing CRC


If you move to the file.calculateCrc approach, you can avoid this buffer....and it has built-in chunking.

odacindy-fprime added 4 commits December 5, 2025 17:56

issue-4270 Updated Svc/PrmDb to implement CRC. Currently builds and r…

95247fb

…uns, but without changing the existing unit tests, when I run them, some are now breaking, so will checkpoint now before making changes to existing unit tests

removed a status check since the current call doesn't returnn a failure

a513e73

fprime/Svc/PrmDb/PrmDb.fpp:

46a7f6c

- add CURR_POSITION, SEEK_ZERO, and SEEK_POSITION to PrmWriteError enum - added return status checking for paramFile.seek() and paramFile.position() calls in PrmDbImpl::PRM_SAVE_FILE_cmdHandler()

github-advanced-security bot found potential problems Dec 20, 2025

View reviewed changes

ran the code formatter on Svc/PrmDb

8a33f34

github-advanced-security bot found potential problems Dec 20, 2025

View reviewed changes

LeStarch requested changes Dec 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Issue 4270 add crc chk to prm db #4586

Issue 4270 add crc chk to prm db #4586

odacindy-fprime commented Dec 20, 2025

Uh oh!

Check warning

Check warning

Check warning

Uh oh!

Uh oh!

Check warning

Check warning

Check warning

Check warning

Check warning

Check warning

Check warning

LeStarch left a comment

Uh oh!

LeStarch Dec 20, 2025

Uh oh!

LeStarch Dec 20, 2025

Uh oh!

LeStarch Dec 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Issue 4270 add crc chk to prm db #4586

Are you sure you want to change the base?

Issue 4270 add crc chk to prm db #4586

Conversation

odacindy-fprime commented Dec 20, 2025

Change Description

Rationale

Testing/Review Recommendations

Future Work

AI Usage (see policy)

Uh oh!

Check warning

Check warning

Check warning

Uh oh!

Uh oh!

Check warning

Check warning

Uh oh!

Check warning

Check warning

Check warning

Check warning

Check warning

LeStarch left a comment

Choose a reason for hiding this comment

Uh oh!

LeStarch Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

LeStarch Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

LeStarch Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants