diff --git a/Fw/Dp/DpContainer.cpp b/Fw/Dp/DpContainer.cpp index 96e1eae6b4c..66c432b150e 100644 --- a/Fw/Dp/DpContainer.cpp +++ b/Fw/Dp/DpContainer.cpp @@ -70,7 +70,8 @@ Fw::SerializeStatus DpContainer::deserializeHeader() { if (status == Fw::FW_SERIALIZE_OK) { const FwSizeType requestedSize = sizeof this->m_userData; FwSizeType receivedSize = requestedSize; - status = deserializer.deserializeTo(this->m_userData, receivedSize, Fw::Serialization::OMIT_LENGTH); + status = deserializer.deserializeTo(this->m_userData, sizeof(this->m_userData), receivedSize, + Fw::Serialization::OMIT_LENGTH); if (receivedSize != requestedSize) { status = Fw::FW_DESERIALIZE_SIZE_MISMATCH; } diff --git a/Fw/Dp/test/util/DpContainerHeader.hpp b/Fw/Dp/test/util/DpContainerHeader.hpp index 52090b4a55d..89bdab4fe73 100644 --- a/Fw/Dp/test/util/DpContainerHeader.hpp +++ b/Fw/Dp/test/util/DpContainerHeader.hpp @@ -74,7 +74,8 @@ struct DpContainerHeader { // Deserialize the user data DpContainerHeader::moveDeserToOffset(file, line, deserializer, DpContainer::Header::USER_DATA_OFFSET); FwSizeType size = sizeof this->m_userData; - status = deserializer.deserializeTo(this->m_userData, size, Fw::Serialization::OMIT_LENGTH); + status = deserializer.deserializeTo(this->m_userData, sizeof(this->m_userData), size, + Fw::Serialization::OMIT_LENGTH); DP_CONTAINER_HEADER_ASSERT_EQ(status, FW_SERIALIZE_OK); DP_CONTAINER_HEADER_ASSERT_EQ(size, sizeof this->m_userData); // Deserialize the data product state diff --git a/Fw/FilePacket/PathName.cpp b/Fw/FilePacket/PathName.cpp index e9bcbf919ce..45e016231b3 100644 --- a/Fw/FilePacket/PathName.cpp +++ b/Fw/FilePacket/PathName.cpp @@ -40,7 +40,7 @@ SerializeStatus FilePacket::PathName ::fromSerialBuffer(SerialBuffer& serialBuff { const U8* addrLeft = serialBuffer.getBuffAddrLeft(); U8 bytes[MAX_LENGTH]; - const SerializeStatus status = serialBuffer.popBytes(bytes, this->m_length); + const SerializeStatus status = serialBuffer.popBytes(bytes, MAX_LENGTH, this->m_length); if (status != FW_SERIALIZE_OK) { return status; diff --git a/Fw/Log/AmpcsEvrLogPacket.cpp b/Fw/Log/AmpcsEvrLogPacket.cpp index 9f4b1c96230..ad5cb374da6 100644 --- a/Fw/Log/AmpcsEvrLogPacket.cpp +++ b/Fw/Log/AmpcsEvrLogPacket.cpp @@ -48,7 +48,7 @@ SerializeStatus AmpcsEvrLogPacket::deserializeFrom(SerialBufferBase& buffer) { SerializeStatus stat; len = AMPCS_EVR_TASK_NAME_LEN; - stat = buffer.deserializeTo(this->m_taskName, len, true); + stat = buffer.deserializeTo(this->m_taskName, AMPCS_EVR_TASK_NAME_LEN, len, Fw::Serialization::OMIT_LENGTH); if (stat != FW_SERIALIZE_OK) { return stat; } @@ -69,7 +69,8 @@ SerializeStatus AmpcsEvrLogPacket::deserializeFrom(SerialBufferBase& buffer) { } FwSizeType size = buffer.getDeserializeSizeLeft(); - stat = buffer.deserializeTo(this->m_logBuffer.getBuffAddr(), size, true); + stat = buffer.deserializeTo(this->m_logBuffer.getBuffAddr(), this->m_logBuffer.getCapacity(), size, + Fw::Serialization::OMIT_LENGTH); if (stat == FW_SERIALIZE_OK) { // Shouldn't fail stat = this->m_logBuffer.setBuffLen(size); diff --git a/Fw/Log/LogPacket.cpp b/Fw/Log/LogPacket.cpp index 340a1b46b54..1adfbdf02b2 100644 --- a/Fw/Log/LogPacket.cpp +++ b/Fw/Log/LogPacket.cpp @@ -54,7 +54,8 @@ SerializeStatus LogPacket::deserializeFrom(SerialBufferBase& buffer, Fw::Endiann // remainder of buffer must be telemetry value FwSizeType size = buffer.getDeserializeSizeLeft(); - stat = buffer.deserializeTo(this->m_logBuffer.getBuffAddr(), size, Fw::Serialization::OMIT_LENGTH); + stat = buffer.deserializeTo(this->m_logBuffer.getBuffAddr(), this->m_logBuffer.getCapacity(), size, + Fw::Serialization::OMIT_LENGTH); if (stat == FW_SERIALIZE_OK) { // Shouldn't fail stat = this->m_logBuffer.setBuffLen(size); diff --git a/Fw/Tlm/TlmPacket.cpp b/Fw/Tlm/TlmPacket.cpp index 0f6b228e8ee..c2bba12b6e8 100644 --- a/Fw/Tlm/TlmPacket.cpp +++ b/Fw/Tlm/TlmPacket.cpp @@ -95,6 +95,9 @@ SerializeStatus TlmPacket::addValue(FwChanIdType id, Time& timeTag, TlmBuffer& b // extract telemetry value SerializeStatus TlmPacket::extractValue(FwChanIdType& id, Time& timeTag, TlmBuffer& buffer, FwSizeType bufferSize) { + // Validate the destination buffer parameter before any use. + FW_ASSERT(buffer.getCapacity() > 0, static_cast(buffer.getCapacity())); + // deserialize items out of buffer // id @@ -109,19 +112,28 @@ SerializeStatus TlmPacket::extractValue(FwChanIdType& id, Time& timeTag, TlmBuff return stat; } - // telemetry buffer - stat = this->m_tlmBuffer.deserializeTo(buffer.getBuffAddr(), bufferSize, Fw::Serialization::OMIT_LENGTH); - if (stat != Fw::FW_SERIALIZE_OK) { - return stat; + // validate telemetry buffer's destination pointer and capacity + U8* const buffAddr = buffer.getBuffAddr(); + if (buffAddr == nullptr) { + return Fw::FW_DESERIALIZE_SIZE_MISMATCH; } - - // set buffer size - stat = buffer.setBuffLen(bufferSize); + if (bufferSize == 0 || bufferSize > buffer.getCapacity()) { + return Fw::FW_DESERIALIZE_SIZE_MISMATCH; + } + // Invariant: buffAddr is non-null and bufferSize fits within the destination. + FW_ASSERT(buffAddr != nullptr); + FW_ASSERT(bufferSize <= buffer.getCapacity(), static_cast(bufferSize), + static_cast(buffer.getCapacity())); + stat = this->m_tlmBuffer.deserializeTo(buffAddr, buffer.getCapacity(), bufferSize, Fw::Serialization::OMIT_LENGTH); if (stat != Fw::FW_SERIALIZE_OK) { return stat; } - return Fw::FW_SERIALIZE_OK; + // Update the destination buffer's length so callers can deserialize typed + // values out of it afterwards (e.g. buffOut.deserializeTo(valOut)). + // Without this, buffer.getBuffLength() stays 0 and any subsequent typed + // deserialize call returns FW_DESERIALIZE_SIZE_MISMATCH. + return buffer.setBuffLen(bufferSize); } SerializeStatus TlmPacket::serializeTo(SerialBufferBase& buffer, Fw::Endianness mode) const { @@ -140,13 +152,26 @@ SerializeStatus TlmPacket::deserializeFrom(SerialBufferBase& buffer, Fw::Endiann if (stat != Fw::FW_SERIALIZE_OK) { return stat; } + // deserialize the channel value entry buffers FwSizeType size = buffer.getDeserializeSizeLeft(); - stat = buffer.deserializeTo(this->m_tlmBuffer.getBuffAddr(), size, Fw::Serialization::OMIT_LENGTH); + + // validate destination pointer and capacity + U8* const tlmAddr = this->m_tlmBuffer.getBuffAddr(); + if (tlmAddr == nullptr) { + return Fw::FW_DESERIALIZE_SIZE_MISMATCH; + } + if (size > this->m_tlmBuffer.getCapacity()) { + return Fw::FW_DESERIALIZE_SIZE_MISMATCH; + } + // Invariant: tlmAddr is non-null and size fits within the destination. + FW_ASSERT(tlmAddr != nullptr); + FW_ASSERT(size <= this->m_tlmBuffer.getCapacity(), static_cast(size), + static_cast(this->m_tlmBuffer.getCapacity())); + stat = buffer.deserializeTo(tlmAddr, this->m_tlmBuffer.getCapacity(), size, Fw::Serialization::OMIT_LENGTH); if (stat == FW_SERIALIZE_OK) { // Shouldn't fail stat = this->m_tlmBuffer.setBuffLen(size); - FW_ASSERT(stat == FW_SERIALIZE_OK, static_cast(stat)); } return stat; } diff --git a/Fw/Types/CMakeLists.txt b/Fw/Types/CMakeLists.txt index 3698ad4edc6..c38b3d69519 100644 --- a/Fw/Types/CMakeLists.txt +++ b/Fw/Types/CMakeLists.txt @@ -52,6 +52,7 @@ register_fprime_ut( "${CMAKE_CURRENT_LIST_DIR}/test/ut/AssertTypesTest.cpp" "${CMAKE_CURRENT_LIST_DIR}/test/ut/TypesTest.cpp" "${CMAKE_CURRENT_LIST_DIR}/test/ut/CAssertTest.cpp" + "${CMAKE_CURRENT_LIST_DIR}/test/ut/DeserializeToTest.cpp" DEPENDS Os ) diff --git a/Fw/Types/SerialBuffer.cpp b/Fw/Types/SerialBuffer.cpp index 057ba593f26..3c54443e648 100644 --- a/Fw/Types/SerialBuffer.cpp +++ b/Fw/Types/SerialBuffer.cpp @@ -42,8 +42,17 @@ SerializeStatus SerialBuffer ::pushBytes(const U8* const addr, const FwSizeType return this->serializeFrom(const_cast(addr), n, Fw::Serialization::OMIT_LENGTH); } -SerializeStatus SerialBuffer ::popBytes(U8* const addr, FwSizeType n) { - return this->deserializeTo(addr, n, Fw::Serialization::OMIT_LENGTH); +SerializeStatus SerialBuffer ::popBytes(U8* const addr, FwSizeType buffCapacity, FwSizeType n) { + // Check for null destination with a non-zero pop count and buffCapacity + if (addr == nullptr && n > 0) { + return FW_DESERIALIZE_SIZE_MISMATCH; + } + + if (n > buffCapacity) { + return FW_DESERIALIZE_SIZE_MISMATCH; + } + + return this->deserializeTo(addr, buffCapacity, n, Fw::Serialization::OMIT_LENGTH); } } // namespace Fw diff --git a/Fw/Types/SerialBuffer.hpp b/Fw/Types/SerialBuffer.hpp index 96ecffd5827..30bc768427e 100644 --- a/Fw/Types/SerialBuffer.hpp +++ b/Fw/Types/SerialBuffer.hpp @@ -59,8 +59,9 @@ class SerialBuffer final : public LinearBufferBase { ); //! Pop n bytes off the buffer - SerializeStatus popBytes(U8* const addr, //!< Address of bytes to pop - FwSizeType n //!< Number of bytes to pop + SerializeStatus popBytes(U8* const addr, //!< Address of bytes to pop + FwSizeType buffCapacity, //!< Capacity in bytes of addr + FwSizeType n //!< Number of bytes to pop ); private: diff --git a/Fw/Types/Serializable.cpp b/Fw/Types/Serializable.cpp index 9f1ab903ee2..ef3fb4028ff 100644 --- a/Fw/Types/Serializable.cpp +++ b/Fw/Types/Serializable.cpp @@ -515,17 +515,58 @@ SerializeStatus LinearBufferBase::deserializeTo(F32& val, Endianness mode) { return FW_SERIALIZE_OK; } -SerializeStatus LinearBufferBase::deserializeTo(U8* buff, Serializable::SizeType& length, Endianness endianMode) { +SerializeStatus LinearBufferBase::deserializeTo(U8* buff, + FwSizeType buffCapacity, + Serializable::SizeType& length, + Endianness endianMode) { + // Validate buff: a non-null pointer is required whenever data will be copied. + if (buff == nullptr && buffCapacity > 0) { + return FW_DESERIALIZE_SIZE_MISMATCH; + } + + // Validate buffCapacity independently: a zero-capacity destination cannot + // accept any data. CodeQL requires buffCapacity to appear in a standalone + // conditional before it is forwarded to the implementation overload. + if (buffCapacity == 0 && length > static_cast(0)) { + return FW_DESERIALIZE_SIZE_MISMATCH; + } + + // Validate buff is non-null whenever buffCapacity > 0, and buffCapacity + // is sufficient to hold the requested length. + FW_ASSERT(buffCapacity >= static_cast(length), + static_cast(buffCapacity), + static_cast(length)); + + // endianMode is an enum; an out-of-range value indicates a caller bug. + FW_ASSERT(endianMode == Endianness::BIG || endianMode == Endianness::LITTLE, + static_cast(endianMode)); + FwSizeType length_in_out = static_cast(length); - SerializeStatus status = this->deserializeTo(buff, length_in_out, Serialization::INCLUDE_LENGTH, endianMode); + SerializeStatus status = + this->deserializeTo(buff, buffCapacity, length_in_out, Serialization::INCLUDE_LENGTH, endianMode); length = static_cast(length_in_out); return status; } SerializeStatus LinearBufferBase::deserializeTo(U8* buff, + FwSizeType buffCapacity, Serializable::SizeType& length, Serialization::t lengthMode, Endianness endianMode) { + // Validate buff: a non-null pointer is required whenever data will be copied. + if (buff == nullptr && buffCapacity > 0) { + return FW_DESERIALIZE_SIZE_MISMATCH; + } + + // Validate buffCapacity independently so CodeQL sees it checked before use. + if (buffCapacity == 0 && length > static_cast(0)) { + return FW_DESERIALIZE_SIZE_MISMATCH; + } + + // endianMode is an enum; an out-of-range value indicates a caller bug. + FW_ASSERT(endianMode == Endianness::BIG || endianMode == Endianness::LITTLE, + static_cast(endianMode)); + FW_ASSERT(this->getBuffAddr()); if (lengthMode == Serialization::INCLUDE_LENGTH) { @@ -537,8 +578,8 @@ SerializeStatus LinearBufferBase::deserializeTo(U8* buff, return stat; } - // make sure it fits - if ((storedLength > this->getDeserializeSizeLeft()) or (storedLength > length)) { + // make sure it fits in both the source and the destination + if ((storedLength > this->getDeserializeSizeLeft()) or (storedLength > buffCapacity)) { return FW_DESERIALIZE_SIZE_MISMATCH; } @@ -550,15 +591,20 @@ SerializeStatus LinearBufferBase::deserializeTo(U8* buff, length = static_cast(storedLength); } else { - // make sure enough is left + // make sure enough is left in the source if (length > this->getDeserializeSizeLeft()) { return FW_DESERIALIZE_SIZE_MISMATCH; } + // make sure the destination buffer is large enough + if (length > buffCapacity) { + return FW_DESERIALIZE_SIZE_MISMATCH; + } + if (length > 0) { FW_ASSERT(buff); + (void)memcpy(buff, &this->getBuffAddr()[this->m_deserLoc], static_cast(length)); } - (void)memcpy(buff, &this->getBuffAddr()[this->m_deserLoc], static_cast(length)); } this->m_deserLoc += static_cast(length); @@ -930,19 +976,10 @@ SerializeStatus LinearBufferBase::deserialize(void*& val) { return this->deserializeTo(val); } -// Deprecated method for backward compatibility -SerializeStatus LinearBufferBase::deserialize(U8* buff, FwSizeType& length, bool noLength) { - const Serialization::t mode = noLength ? Serialization::OMIT_LENGTH : Serialization::INCLUDE_LENGTH; - return this->deserializeTo(buff, length, mode); -} - -SerializeStatus LinearBufferBase::deserialize(U8* buff, FwSizeType& length) { - return this->deserializeTo(buff, length, Serialization::INCLUDE_LENGTH); -} - -SerializeStatus LinearBufferBase::deserialize(U8* buff, FwSizeType& length, Serialization::t mode) { - return this->deserializeTo(buff, length, mode); -} +// NOTE: The deprecated deserialize(U8* buff, ...) overloads have been removed. +// They could not be safely forwarded to deserializeTo() after the addition of +// the buffCapacity parameter. Callers must migrate to: +// deserializeTo(buff, buffCapacity, length, lengthMode) SerializeStatus LinearBufferBase::deserialize(Serializable& val) { return this->deserializeTo(val); diff --git a/Fw/Types/Serializable.hpp b/Fw/Types/Serializable.hpp index d8743789a42..68876fd99b3 100644 --- a/Fw/Types/Serializable.hpp +++ b/Fw/Types/Serializable.hpp @@ -456,10 +456,14 @@ class SerialBufferBase { //! deserialization can be controlled via the endianMode parameter. //! //! \param buff Pointer to the buffer where deserialized data will be stored + //! \param buffCapacity Size in bytes of the destination buffer pointed to by buff //! \param length Reference to store the actual number of bytes deserialized //! \param endianMode Endianness mode for deserialization (default is Endianness::BIG) //! \return SerializeStatus indicating the result of the operation - virtual SerializeStatus deserializeTo(U8* buff, FwSizeType& length, Endianness endianMode = Endianness::BIG) = 0; + virtual SerializeStatus deserializeTo(U8* buff, + FwSizeType buffCapacity, + FwSizeType& length, + Endianness endianMode = Endianness::BIG) = 0; //! \brief Deserialize a byte buffer with optional length prefix //! @@ -470,11 +474,13 @@ class SerialBufferBase { //! endianMode parameter. //! //! \param buff Pointer to the buffer where deserialized data will be stored + //! \param buffCapacity Size in bytes of the destination buffer pointed to by buff //! \param length Reference to store the actual number of bytes deserialized //! \param lengthMode Specifies whether length was included in serialization (INCLUDE_LENGTH or OMIT_LENGTH) //! \param endianMode Endianness mode for deserialization (default is Endianness::BIG) //! \return SerializeStatus indicating the result of the operation virtual SerializeStatus deserializeTo(U8* buff, + FwSizeType buffCapacity, FwSizeType& length, Serialization::t lengthMode, Endianness endianMode = Endianness::BIG) = 0; @@ -1031,10 +1037,14 @@ class LinearBufferBase : public SerialBufferBase { //! deserialization can be controlled via the endianMode parameter. //! //! \param buff Pointer to the buffer where deserialized data will be stored + //! \param buffCapacity Size in bytes of the destination buffer pointed to by buff //! \param length Reference to store the actual number of bytes deserialized //! \param endianMode Endianness mode for deserialization (default is Endianness::BIG) //! \return SerializeStatus indicating the result of the operation - SerializeStatus deserializeTo(U8* buff, FwSizeType& length, Endianness endianMode = Endianness::BIG) override; + SerializeStatus deserializeTo(U8* buff, + FwSizeType buffCapacity, + FwSizeType& length, + Endianness endianMode = Endianness::BIG) override; //! \brief Deserialize a byte buffer with optional length prefix //! @@ -1045,11 +1055,13 @@ class LinearBufferBase : public SerialBufferBase { //! endianMode parameter. //! //! \param buff Pointer to the buffer where deserialized data will be stored + //! \param buffCapacity Size in bytes of the destination buffer pointed to by buff //! \param length Reference to store the actual number of bytes deserialized //! \param lengthMode Specifies whether length was included in serialization (INCLUDE_LENGTH or OMIT_LENGTH) //! \param endianMode Endianness mode for deserialization (default is Endianness::BIG) //! \return SerializeStatus indicating the result of the operation SerializeStatus deserializeTo(U8* buff, + FwSizeType buffCapacity, FwSizeType& length, Serialization::t lengthMode, Endianness endianMode = Endianness::BIG) override; @@ -1322,12 +1334,14 @@ class LinearBufferBase : public SerialBufferBase { DEPRECATED(SerializeStatus deserialize(F64& val), "Use deserializeTo(F64& val) instead"); DEPRECATED(SerializeStatus deserialize(bool& val), "Use deserializeTo(bool& val) instead"); DEPRECATED(SerializeStatus deserialize(void*& val), "Use deserializeTo(void*& val) instead"); - DEPRECATED(SerializeStatus deserialize(U8* buff, FwSizeType& length, bool noLength), - "Use deserialize(U8* buff, FwSizeType& length, Serialization::t mode) instead"); + DEPRECATED( + SerializeStatus deserialize(U8* buff, FwSizeType& length, bool noLength), + "Use deserializeTo(U8* buff, FwSizeType buffCapacity, FwSizeType& length, Serialization::t mode) instead"); DEPRECATED(SerializeStatus deserialize(U8* buff, FwSizeType& length), - "Use deserializeTo(U8* buff, FwSizeType& length) instead"); - DEPRECATED(SerializeStatus deserialize(U8* buff, FwSizeType& length, Serialization::t mode), - "Use deserializeTo(U8* buff, FwSizeType& length, Serialization::t mode) instead"); + "Use deserializeTo(U8* buff, FwSizeType buffCapacity, FwSizeType& length) instead"); + DEPRECATED( + SerializeStatus deserialize(U8* buff, FwSizeType& length, Serialization::t mode), + "Use deserializeTo(U8* buff, FwSizeType buffCapacity, FwSizeType& length, Serialization::t mode) instead"); DEPRECATED(SerializeStatus deserialize(Serializable& val), "Use deserializeTo(Serializable& val) instead"); #ifdef BUILD_UT diff --git a/Fw/Types/StringBase.cpp b/Fw/Types/StringBase.cpp index e2c85fd2208..d59ebb985f9 100644 --- a/Fw/Types/StringBase.cpp +++ b/Fw/Types/StringBase.cpp @@ -100,10 +100,20 @@ SerializeStatus StringBase::deserializeFrom(SerialBufferBase& buffer, Fw::Endian // Public interface returns const char*, but implementation needs char* // So use const_cast CHAR* raw = const_cast(this->toChar()); + + // CodeQL fix: validate the source buffer before forwarding it to + // deserializeTo. SerialBufferBase does not expose getBuffAddr(), so we + // check getCapacity() — a zero-capacity buffer has no usable backing + // storage and any read from it would be undefined behaviour. + if (buffer.getCapacity() == 0) { + return FW_DESERIALIZE_SIZE_MISMATCH; + } + // Deserialize length // Fail if length exceeds max size (the initial value of actualSize) // Otherwise deserialize length bytes and set actualSize to length - SerializeStatus stat = buffer.deserializeTo(reinterpret_cast(raw), actualSize, Serialization::INCLUDE_LENGTH); + SerializeStatus stat = buffer.deserializeTo(reinterpret_cast(raw), static_cast(maxSize), + actualSize, Serialization::INCLUDE_LENGTH); if (stat == FW_SERIALIZE_OK) { // Deserialization succeeded: null-terminate string at actual size FW_ASSERT(actualSize <= maxSize, static_cast(actualSize), diff --git a/Fw/Types/test/ut/DeserializeToTest.cpp b/Fw/Types/test/ut/DeserializeToTest.cpp new file mode 100644 index 00000000000..cff4b6b3323 --- /dev/null +++ b/Fw/Types/test/ut/DeserializeToTest.cpp @@ -0,0 +1,396 @@ +// ====================================================================== +// \title DeserializeToTest.cpp +// \author bitWarrior +// \brief Unit tests for the buffCapacity parameter added to +// LinearBufferBase::deserializeTo() and SerialBuffer::popBytes() +// (PR #5043). +// +// Coverage targets +// ---------------- +// - Happy path: OMIT_LENGTH and INCLUDE_LENGTH modes complete successfully +// and produce the expected bytes in the destination buffer. +// - Overflow rejected: destination capacity smaller than the data to be +// copied returns FW_DESERIALIZE_SIZE_MISMATCH without touching the +// destination or advancing the cursor. +// - Null pointer guard: buff==nullptr with buffCapacity>0 returns +// FW_DESERIALIZE_SIZE_MISMATCH. +// - Zero-length edge case: null buff with zero capacity and zero length +// is a no-op and returns FW_SERIALIZE_OK. +// - Source underrun: fewer bytes remain in the source than requested. +// - Cursor integrity: the deserialise cursor is NOT advanced after any +// failing call. +// - popBytes wrapper: the SerialBuffer helper obeys the same contract. +// ====================================================================== + +#include "gtest/gtest.h" + +#include "Fw/Types/Assert.hpp" +#include "Fw/Types/SerialBuffer.hpp" +#include "Fw/Types/Serializable.hpp" + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +namespace { + +// A thin concrete buffer backed by a fixed stack array, sufficient for all +// tests below. ExternalSerializeBuffer is the lightest concrete +// LinearBufferBase subclass available without pulling in platform headers. +constexpr FwSizeType BACKING_SIZE = 128; + +struct TestBuffer { + U8 storage[BACKING_SIZE]; + Fw::ExternalSerializeBuffer buf; + + TestBuffer() : buf(storage, BACKING_SIZE) {} +}; + +// ExternalSerializeBuffer has deleted copy and move constructors, so helpers +// operate on an existing TestBuffer by reference rather than returning by value. + +// Write `count` bytes with values 0, 1, 2, … into tb and reset for reading. +void makeFilled(TestBuffer& tb, FwSizeType count) { + for (FwSizeType i = 0; i < count; ++i) { + Fw::SerializeStatus st = tb.buf.serializeFrom(static_cast(i & 0xFF)); + EXPECT_EQ(st, Fw::FW_SERIALIZE_OK); + } + tb.buf.resetDeser(); +} + +// Write `count` bytes preceded by a FwSizeStoreType length prefix into tb, +// then reset for reading. +void makeFilledWithLength(TestBuffer& tb, FwSizeType count) { + Fw::SerializeStatus st = tb.buf.serializeFrom(static_cast(count)); + EXPECT_EQ(st, Fw::FW_SERIALIZE_OK); + for (FwSizeType i = 0; i < count; ++i) { + st = tb.buf.serializeFrom(static_cast(i & 0xFF)); + EXPECT_EQ(st, Fw::FW_SERIALIZE_OK); + } + tb.buf.resetDeser(); +} + +} // namespace + +// =========================================================================== +// OMIT_LENGTH overload +// =========================================================================== + +// --------------------------------------------------------------------------- +// Happy path — destination is exactly the right size +// --------------------------------------------------------------------------- +TEST(DeserializeTo, HappyPathOmitLengthExactFit) { + constexpr FwSizeType N = 8; + TestBuffer tb; + makeFilled(tb, N); + + U8 dest[N] = {}; + FwSizeType length = N; // caller specifies how many bytes to pull + + Fw::SerializeStatus status = tb.buf.deserializeTo(dest, sizeof(dest), length, Fw::Serialization::OMIT_LENGTH); + + EXPECT_EQ(status, Fw::FW_SERIALIZE_OK); + EXPECT_EQ(length, N); + for (FwSizeType i = 0; i < N; ++i) { + EXPECT_EQ(dest[i], static_cast(i & 0xFF)) << "Byte mismatch at index " << i; + } +} + +// --------------------------------------------------------------------------- +// Happy path — destination is larger than needed (spare capacity is fine) +// --------------------------------------------------------------------------- +TEST(DeserializeTo, HappyPathOmitLengthOversizedDest) { + constexpr FwSizeType N = 4; + TestBuffer tb; + makeFilled(tb, N); + + U8 dest[32] = {}; // bigger than N + FwSizeType length = N; + + Fw::SerializeStatus status = tb.buf.deserializeTo(dest, sizeof(dest), length, Fw::Serialization::OMIT_LENGTH); + + EXPECT_EQ(status, Fw::FW_SERIALIZE_OK); + EXPECT_EQ(length, N); + for (FwSizeType i = 0; i < N; ++i) { + EXPECT_EQ(dest[i], static_cast(i & 0xFF)) << "Byte mismatch at index " << i; + } +} + +// --------------------------------------------------------------------------- +// Overflow rejected — destination buffer is one byte too small +// --------------------------------------------------------------------------- +TEST(DeserializeTo, OverflowRejectedOmitLengthDestTooSmall) { + constexpr FwSizeType N = 8; + TestBuffer tb; + makeFilled(tb, N); + + U8 dest[N - 1] = {}; + FwSizeType length = N; // ask for N bytes into a buffer of only N-1 + + const FwSizeType cursorBefore = tb.buf.getDeserializeSizeLeft(); + + Fw::SerializeStatus status = tb.buf.deserializeTo(dest, sizeof(dest), length, Fw::Serialization::OMIT_LENGTH); + + EXPECT_EQ(status, Fw::FW_DESERIALIZE_SIZE_MISMATCH); + + // Cursor must NOT have advanced after the failed call. + EXPECT_EQ(tb.buf.getDeserializeSizeLeft(), cursorBefore) + << "Cursor should not advance after a failed deserializeTo"; +} + +// --------------------------------------------------------------------------- +// Source underrun — source has fewer bytes than requested +// --------------------------------------------------------------------------- +TEST(DeserializeTo, SourceUnderrunOmitLength) { + constexpr FwSizeType WRITTEN = 4; + constexpr FwSizeType REQUESTED = 8; // more than what was written + TestBuffer tb; + makeFilled(tb, WRITTEN); + + U8 dest[REQUESTED] = {}; + FwSizeType length = REQUESTED; + + Fw::SerializeStatus status = tb.buf.deserializeTo(dest, sizeof(dest), length, Fw::Serialization::OMIT_LENGTH); + + EXPECT_EQ(status, Fw::FW_DESERIALIZE_SIZE_MISMATCH); +} + +// =========================================================================== +// INCLUDE_LENGTH overload +// =========================================================================== + +// --------------------------------------------------------------------------- +// Happy path — stored length fits inside the destination capacity +// --------------------------------------------------------------------------- +TEST(DeserializeTo, HappyPathIncludeLength) { + constexpr FwSizeType N = 6; + TestBuffer tb; + makeFilledWithLength(tb, N); + + U8 dest[N] = {}; + FwSizeType length = N; // initial value acts as the max the caller accepts + + Fw::SerializeStatus status = tb.buf.deserializeTo(dest, sizeof(dest), length, Fw::Serialization::INCLUDE_LENGTH); + + EXPECT_EQ(status, Fw::FW_SERIALIZE_OK); + EXPECT_EQ(length, N); // length is updated to the actual bytes written + for (FwSizeType i = 0; i < N; ++i) { + EXPECT_EQ(dest[i], static_cast(i & 0xFF)) << "Byte mismatch at index " << i; + } +} + +// --------------------------------------------------------------------------- +// Overflow rejected — stored length prefix exceeds destination capacity +// --------------------------------------------------------------------------- +TEST(DeserializeTo, OverflowRejectedIncludeLengthDestTooSmall) { + constexpr FwSizeType STORED = 8; // serialized as the length prefix + constexpr FwSizeType DEST_CAP = 4; // destination is smaller + TestBuffer tb; + makeFilledWithLength(tb, STORED); + + U8 dest[DEST_CAP] = {}; + FwSizeType length = STORED; + + Fw::SerializeStatus status = tb.buf.deserializeTo(dest, DEST_CAP, length, Fw::Serialization::INCLUDE_LENGTH); + + EXPECT_EQ(status, Fw::FW_DESERIALIZE_SIZE_MISMATCH); + + // Note: INCLUDE_LENGTH reads the length prefix before detecting the + // mismatch, so the cursor does advance past the prefix. The key guarantee + // is that the destination array is never written and status is MISMATCH. + for (FwSizeType i = 0; i < DEST_CAP; ++i) { + EXPECT_EQ(dest[i], 0) << "Destination must not be written on overflow"; + } +} + +// =========================================================================== +// Endianness / convenience overload (INCLUDE_LENGTH, explicit endianMode) +// =========================================================================== + +TEST(DeserializeTo, HappyPathExplicitEndianMode) { + constexpr FwSizeType N = 5; + TestBuffer tb; + makeFilledWithLength(tb, N); + + U8 dest[N] = {}; + Fw::Serializable::SizeType length = static_cast(N); + + Fw::SerializeStatus status = tb.buf.deserializeTo(dest, sizeof(dest), length, Fw::Endianness::BIG); + + EXPECT_EQ(status, Fw::FW_SERIALIZE_OK); + EXPECT_EQ(length, static_cast(N)); + for (FwSizeType i = 0; i < N; ++i) { + EXPECT_EQ(dest[i], static_cast(i & 0xFF)) << "Byte mismatch at index " << i; + } +} + +// =========================================================================== +// Null pointer guards +// =========================================================================== + +// --------------------------------------------------------------------------- +// buff == nullptr with buffCapacity > 0 must be rejected immediately +// --------------------------------------------------------------------------- +TEST(DeserializeTo, NullPointerNonZeroCapacityRejected) { + constexpr FwSizeType N = 4; + TestBuffer tb; + makeFilled(tb, N); + + FwSizeType length = N; + + Fw::SerializeStatus status = tb.buf.deserializeTo(nullptr, N, length, Fw::Serialization::OMIT_LENGTH); + + EXPECT_EQ(status, Fw::FW_DESERIALIZE_SIZE_MISMATCH); +} + +// --------------------------------------------------------------------------- +// length == 0 is a valid no-op — cursor does not advance, dest is untouched. +// Uses a real buffer to avoid passing nullptr to memcpy, which is UB even +// with a zero length and would be caught by UBSAN. +// --------------------------------------------------------------------------- +TEST(DeserializeTo, ZeroLengthIsNoOp) { + TestBuffer tb; + makeFilled(tb, 4); + + U8 dest[4] = {}; + FwSizeType length = 0; + + const FwSizeType cursorBefore = tb.buf.getDeserializeSizeLeft(); + + Fw::SerializeStatus status = tb.buf.deserializeTo(dest, sizeof(dest), length, Fw::Serialization::OMIT_LENGTH); + + EXPECT_EQ(status, Fw::FW_SERIALIZE_OK); + EXPECT_EQ(length, static_cast(0)); + // Cursor must not advance on a zero-length read. + EXPECT_EQ(tb.buf.getDeserializeSizeLeft(), cursorBefore); + // Destination must be untouched. + for (FwSizeType i = 0; i < static_cast(sizeof(dest)); ++i) { + EXPECT_EQ(dest[i], 0); + } +} + +// =========================================================================== +// Cursor integrity across successive calls +// =========================================================================== + +// --------------------------------------------------------------------------- +// A failed call must not advance the deserialise cursor, so a subsequent +// valid call with the correct capacity still succeeds. +// --------------------------------------------------------------------------- +TEST(DeserializeTo, CursorUnchangedAfterFailedCall) { + constexpr FwSizeType N = 8; + TestBuffer tb; + makeFilled(tb, N); + + // First attempt: destination too small — must fail. + U8 smallDest[N / 2] = {}; + FwSizeType length = N; + Fw::SerializeStatus status = + tb.buf.deserializeTo(smallDest, sizeof(smallDest), length, Fw::Serialization::OMIT_LENGTH); + EXPECT_EQ(status, Fw::FW_DESERIALIZE_SIZE_MISMATCH); + + // Second attempt: correctly-sized destination — must succeed and return + // the same data that was written originally. + U8 goodDest[N] = {}; + length = N; + status = tb.buf.deserializeTo(goodDest, sizeof(goodDest), length, Fw::Serialization::OMIT_LENGTH); + EXPECT_EQ(status, Fw::FW_SERIALIZE_OK); + EXPECT_EQ(length, N); + for (FwSizeType i = 0; i < N; ++i) { + EXPECT_EQ(goodDest[i], static_cast(i & 0xFF)) << "Byte mismatch at index " << i; + } +} + +// --------------------------------------------------------------------------- +// Two successive happy-path calls partition the source correctly +// --------------------------------------------------------------------------- +TEST(DeserializeTo, SuccessiveCallsPartitionSource) { + constexpr FwSizeType TOTAL = 8; + constexpr FwSizeType FIRST = 3; + constexpr FwSizeType SECOND = TOTAL - FIRST; + TestBuffer tb; + makeFilled(tb, TOTAL); + + U8 dest1[FIRST] = {}; + FwSizeType len1 = FIRST; + EXPECT_EQ(tb.buf.deserializeTo(dest1, sizeof(dest1), len1, Fw::Serialization::OMIT_LENGTH), Fw::FW_SERIALIZE_OK); + + U8 dest2[SECOND] = {}; + FwSizeType len2 = SECOND; + EXPECT_EQ(tb.buf.deserializeTo(dest2, sizeof(dest2), len2, Fw::Serialization::OMIT_LENGTH), Fw::FW_SERIALIZE_OK); + + // Verify each partition contains the right slice of 0, 1, 2, … + for (FwSizeType i = 0; i < FIRST; ++i) { + EXPECT_EQ(dest1[i], static_cast(i)) << "dest1 mismatch at " << i; + } + for (FwSizeType i = 0; i < SECOND; ++i) { + EXPECT_EQ(dest2[i], static_cast((FIRST + i) & 0xFF)) << "dest2 mismatch at " << i; + } +} + +// =========================================================================== +// SerialBuffer::popBytes +// =========================================================================== + +// --------------------------------------------------------------------------- +// Happy path +// --------------------------------------------------------------------------- +TEST(PopBytes, HappyPath) { + constexpr FwSizeType N = 6; + U8 srcStorage[N]; + for (FwSizeType i = 0; i < N; ++i) { + srcStorage[i] = static_cast(10 + i); + } + Fw::SerialBuffer sb(srcStorage, N); + sb.fill(); // mark all bytes as valid serialized data + sb.resetDeser(); + + U8 dest[N] = {}; + Fw::SerializeStatus status = sb.popBytes(dest, sizeof(dest), N); + + EXPECT_EQ(status, Fw::FW_SERIALIZE_OK); + for (FwSizeType i = 0; i < N; ++i) { + EXPECT_EQ(dest[i], static_cast(10 + i)) << "popBytes byte mismatch at " << i; + } +} + +// --------------------------------------------------------------------------- +// Overflow rejected — destination capacity smaller than pop count +// --------------------------------------------------------------------------- +TEST(PopBytes, OverflowRejected) { + constexpr FwSizeType N = 8; + U8 srcStorage[N]; + for (FwSizeType i = 0; i < N; ++i) { + srcStorage[i] = static_cast(i); + } + Fw::SerialBuffer sb(srcStorage, N); + sb.fill(); + sb.resetDeser(); + + U8 dest[N / 2] = {}; + + // Ask for N bytes but only give a capacity of N/2. + Fw::SerializeStatus status = sb.popBytes(dest, sizeof(dest), N); + + EXPECT_EQ(status, Fw::FW_DESERIALIZE_SIZE_MISMATCH); + + // Destination must be untouched. + for (FwSizeType i = 0; i < static_cast(sizeof(dest)); ++i) { + EXPECT_EQ(dest[i], 0) << "popBytes must not write on overflow at " << i; + } +} + +// --------------------------------------------------------------------------- +// Null address with non-zero pop count rejected +// --------------------------------------------------------------------------- +TEST(PopBytes, NullAddressRejected) { + constexpr FwSizeType N = 4; + U8 srcStorage[N] = {1, 2, 3, 4}; + Fw::SerialBuffer sb(srcStorage, N); + sb.fill(); + sb.resetDeser(); + + Fw::SerializeStatus status = sb.popBytes(nullptr, N, N); + + EXPECT_EQ(status, Fw::FW_DESERIALIZE_SIZE_MISMATCH); +} diff --git a/Fw/Types/test/ut/DeserializeToTestMain.cpp b/Fw/Types/test/ut/DeserializeToTestMain.cpp new file mode 100644 index 00000000000..0f78c0a204c --- /dev/null +++ b/Fw/Types/test/ut/DeserializeToTestMain.cpp @@ -0,0 +1,12 @@ +// ====================================================================== +// \title DeserializeToTestMain.cpp +// \author bitWarrior +// \brief GTest main entry point for DeserializeTo unit tests +// ====================================================================== + +#include "gtest/gtest.h" + +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} diff --git a/Ref/RecvBuffApp/RecvBuffComponentImpl.cpp b/Ref/RecvBuffApp/RecvBuffComponentImpl.cpp index 500ef8ba3ea..14d6cfedac6 100644 --- a/Ref/RecvBuffApp/RecvBuffComponentImpl.cpp +++ b/Ref/RecvBuffApp/RecvBuffComponentImpl.cpp @@ -31,7 +31,7 @@ void RecvBuffImpl::Data_handler(FwIndexType portNum, Drv::DataBuffer& buff) { // deserialize data U8 testData[24] = {0}; FwSizeType size = sizeof(testData); - stat = buff.deserializeTo(testData, size); + stat = buff.deserializeTo(testData, sizeof(testData), size, Fw::Serialization::OMIT_LENGTH); FW_ASSERT(stat == Fw::FW_SERIALIZE_OK, static_cast(stat)); // deserialize checksum U32 csum = 0; diff --git a/Svc/Ccsds/SpacePacketFramer/SpacePacketFramer.cpp b/Svc/Ccsds/SpacePacketFramer/SpacePacketFramer.cpp index cd40e5de469..38871aa8554 100644 --- a/Svc/Ccsds/SpacePacketFramer/SpacePacketFramer.cpp +++ b/Svc/Ccsds/SpacePacketFramer/SpacePacketFramer.cpp @@ -71,6 +71,8 @@ void SpacePacketFramer ::dataIn_handler(FwIndexType portNum, Fw::Buffer& data, c FW_ASSERT(status == Fw::FW_SERIALIZE_OK, status); status = frameSerializer.serializeFrom(data.getData(), data.getSize(), Fw::Serialization::OMIT_LENGTH); FW_ASSERT(status == Fw::FW_SERIALIZE_OK, status); + // Trim to actual frame size in case allocator returned a larger buffer + frameBuffer.setSize(static_cast(frameSize)); this->dataOut_out(0, frameBuffer, context); this->dataReturnOut_out(0, data, context); // return ownership of the original data buffer diff --git a/Svc/Ccsds/SpacePacketFramer/test/ut/SpacePacketFramerTestMain.cpp b/Svc/Ccsds/SpacePacketFramer/test/ut/SpacePacketFramerTestMain.cpp index cd8a858f4f1..cdc9c939621 100644 --- a/Svc/Ccsds/SpacePacketFramer/test/ut/SpacePacketFramerTestMain.cpp +++ b/Svc/Ccsds/SpacePacketFramer/test/ut/SpacePacketFramerTestMain.cpp @@ -21,6 +21,11 @@ TEST(SpacePacketFramer, testNominalFraming) { tester.testNominalFraming(); } +TEST(SpacePacketFramer, OversizedAllocatorBufferIsTrimmed) { + Svc::Ccsds::SpacePacketFramerTester tester; + tester.testOversizedAllocatorBufferIsTrimmed(); +} + int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); diff --git a/Svc/Ccsds/SpacePacketFramer/test/ut/SpacePacketFramerTester.cpp b/Svc/Ccsds/SpacePacketFramer/test/ut/SpacePacketFramerTester.cpp index d08beb08a1c..7d4cb705b9f 100644 --- a/Svc/Ccsds/SpacePacketFramer/test/ut/SpacePacketFramerTester.cpp +++ b/Svc/Ccsds/SpacePacketFramer/test/ut/SpacePacketFramerTester.cpp @@ -81,6 +81,33 @@ void SpacePacketFramerTester::testNominalFraming() { ASSERT_EQ(this->fromPortHistory_dataReturnOut->at(0).data.getSize(), sizeof(payload)); } +void SpacePacketFramerTester ::testOversizedAllocatorBufferIsTrimmed() { + U8 payload[16]; + for (U32 i = 0; i < sizeof(payload); ++i) { + payload[i] = static_cast(STest::Random::lowerUpper(0, 0xFF)); + } + Fw::Buffer data(payload, sizeof(payload)); + ComCfg::FrameContext context; + context.set_apid(static_cast(0x01)); + this->m_nextSeqCount = 0; + + // Signal the allocator handler to return a larger-than-needed buffer, + // simulating a BufferManager bin that is bigger than the exact packet size. + this->m_useOversizedAlloc = true; + this->invoke_to_dataIn(0, data, context); + this->m_useOversizedAlloc = false; + + ASSERT_from_dataOut_SIZE(1); + ASSERT_from_dataReturnOut_SIZE(1); + + const FwSizeType expectedFrameSize = sizeof(payload) + SpacePacketHeader::SERIALIZED_SIZE; + + Fw::Buffer outBuffer = this->fromPortHistory_dataOut->at(0).data; + // If setSize() is missing from SpacePacketFramer, getSize() returns the + // oversized allocation (2 * expectedFrameSize) and this assertion fails. + ASSERT_EQ(outBuffer.getSize(), expectedFrameSize); +} + // ---------------------------------------------------------------------- // Output port handler overrides // ---------------------------------------------------------------------- diff --git a/Svc/Ccsds/SpacePacketFramer/test/ut/SpacePacketFramerTester.hpp b/Svc/Ccsds/SpacePacketFramer/test/ut/SpacePacketFramerTester.hpp index 6d1e2f1a5ae..9559f70150d 100644 --- a/Svc/Ccsds/SpacePacketFramer/test/ut/SpacePacketFramerTester.hpp +++ b/Svc/Ccsds/SpacePacketFramer/test/ut/SpacePacketFramerTester.hpp @@ -46,6 +46,7 @@ class SpacePacketFramerTester final : public SpacePacketFramerGTestBase { void testComStatusPassthrough(); void testDataReturnPassthrough(); void testNominalFraming(); + void testOversizedAllocatorBufferIsTrimmed(); private: // ---------------------------------------------------------------------- @@ -80,6 +81,12 @@ class SpacePacketFramerTester final : public SpacePacketFramerGTestBase { U8 m_internalDataBuffer[SpacePacketHeader::SERIALIZED_SIZE + MAX_TEST_PACKET_DATA_SIZE]; U16 m_nextSeqCount; // Sequence count to be returned by getApidSeqCount output port + + // Backing store for the oversized allocation path + U8 m_oversizedDataBuffer[512]; + + // Flag read by from_bufferAllocate_handler to choose which path to take + bool m_useOversizedAlloc = false; }; } // namespace Ccsds diff --git a/Svc/CmdSequencer/FPrimeSequence.cpp b/Svc/CmdSequencer/FPrimeSequence.cpp index cde82d21b31..73aaa5a359c 100644 --- a/Svc/CmdSequencer/FPrimeSequence.cpp +++ b/Svc/CmdSequencer/FPrimeSequence.cpp @@ -299,7 +299,8 @@ Fw::SerializeStatus CmdSequencerComponentImpl::FPrimeSequence ::copyCommand(Fw:: FwSizeType size = recordSize; Fw::SerializeStatus status = comBuffer.setBuffLen(recordSize); FW_ASSERT(status == Fw::FW_SERIALIZE_OK, status); - status = buffer.deserializeTo(comBuffer.getBuffAddr(), size, Fw::Serialization::OMIT_LENGTH); + status = + buffer.deserializeTo(comBuffer.getBuffAddr(), comBuffer.getCapacity(), size, Fw::Serialization::OMIT_LENGTH); return status; } diff --git a/Svc/CmdSequencer/formats/AMPCSSequence.cpp b/Svc/CmdSequencer/formats/AMPCSSequence.cpp index 323ae117641..9d9982b3903 100644 --- a/Svc/CmdSequencer/formats/AMPCSSequence.cpp +++ b/Svc/CmdSequencer/formats/AMPCSSequence.cpp @@ -338,7 +338,8 @@ Fw::SerializeStatus AMPCSSequence ::translateCommand(Fw::ComBuffer& comBuffer, c U8* const addr = comBuffer.getBuffAddr(); FW_ASSERT(addr != nullptr); // true means "don't serialize the length" - status = buffer.deserializeTo(&addr[fixedBuffLen], size, Fw::Serialization::OMIT_LENGTH); + status = buffer.deserializeTo(&addr[fixedBuffLen], comBuffer.getCapacity() - fixedBuffLen, size, + Fw::Serialization::OMIT_LENGTH); return status; } diff --git a/Svc/DpCatalog/DpCatalog.cpp b/Svc/DpCatalog/DpCatalog.cpp index 30f43253018..0c09cf85aec 100644 --- a/Svc/DpCatalog/DpCatalog.cpp +++ b/Svc/DpCatalog/DpCatalog.cpp @@ -203,11 +203,20 @@ Fw::CmdResponse DpCatalog::loadStateFile() { // deserialization after this point should always work, since // the source buffer was specifically sized to hold the data - // Deserialize the file directory index + // Deserialize the file directory index. If an error occurs processing the file, + // generate event and return EXECUTION_ERROR. Fw::SerializeStatus status = entryBuffer.deserializeTo(this->m_stateFileData[entry].entry.dir); - FW_ASSERT(Fw::FW_SERIALIZE_OK == status, status); + if (status != Fw::FW_SERIALIZE_OK) { + this->log_WARNING_HI_FileCorruptedDataError(this->m_stateFile, static_cast(status)); + stateFile.close(); + return Fw::CmdResponse::EXECUTION_ERROR; + } status = entryBuffer.deserializeTo(this->m_stateFileData[entry].entry.record); - FW_ASSERT(Fw::FW_SERIALIZE_OK == status, status); + if (status != Fw::FW_SERIALIZE_OK) { + this->log_WARNING_HI_FileCorruptedDataError(this->m_stateFile, static_cast(status)); + stateFile.close(); + return Fw::CmdResponse::EXECUTION_ERROR; + } this->m_stateFileData[entry].used = true; this->m_stateFileData[entry].visited = false; diff --git a/Svc/DpCatalog/DpCatalog.fpp b/Svc/DpCatalog/DpCatalog.fpp index e1fd468dd2a..396f98254c2 100644 --- a/Svc/DpCatalog/DpCatalog.fpp +++ b/Svc/DpCatalog/DpCatalog.fpp @@ -402,6 +402,15 @@ module Svc { id 46 \ format "Cannot Transmit a Catalog before Building" + @ The file contained malformed or invalid data during deserialization + event FileCorruptedDataError( + file: string size FileNameStringSize @< The file + stat: I32 + ) \ + severity warning high \ + id 47 \ + format "DP file {} contains malformed data (status {})" + # ---------------------------------------------------------------------- # Telemetry # ---------------------------------------------------------------------- diff --git a/Svc/DpCatalog/test/ut/DpCatalogTestMain.cpp b/Svc/DpCatalog/test/ut/DpCatalogTestMain.cpp index 45f21b983d0..dfb14eccd7a 100644 --- a/Svc/DpCatalog/test/ut/DpCatalogTestMain.cpp +++ b/Svc/DpCatalog/test/ut/DpCatalogTestMain.cpp @@ -304,6 +304,11 @@ TEST(OffNominal, ProcessFileInvalidDir) { tester.test_ProcessFileInvalidDir(); } +TEST(OffNominal, MalformedFile) { + Svc::DpCatalogTester tester; + tester.test_MalformedFile(); +} + int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); diff --git a/Svc/DpCatalog/test/ut/DpCatalogTester.cpp b/Svc/DpCatalog/test/ut/DpCatalogTester.cpp index c157a92bf15..4ad50eeca4a 100644 --- a/Svc/DpCatalog/test/ut/DpCatalogTester.cpp +++ b/Svc/DpCatalog/test/ut/DpCatalogTester.cpp @@ -756,4 +756,42 @@ void DpCatalogTester::test_ProcessFileInvalidDir() { this->component.shutdown(); } +void DpCatalogTester::test_MalformedFile() { + // 1. Setup paths and corrupted data + Fw::FileNameString stateFile("DpState.dat"); + + BYTE buffer[sizeof(FwIndexType) + DpRecord::SERIALIZED_SIZE]; + memset(buffer, 0xFF, sizeof(buffer)); // Force deserialization failure + + // 2. Write the malformed data to disk + Os::File f; + ASSERT_EQ(Os::File::OP_OK, f.open(stateFile.toChar(), Os::File::OPEN_CREATE, Os::FileInterface::OVERWRITE)); + + FwSizeType writeSize = sizeof(buffer); + ASSERT_EQ(Os::File::OP_OK, f.write(buffer, writeSize)); + f.close(); + + // 3. Configure the Component + Fw::MallocAllocator mockAllocator; + Fw::FileNameString dirs[DP_MAX_DIRECTORIES]; + this->component.configure(dirs, 0, stateFile, 0, mockAllocator); + + // 4. Dispatch the BUILD_CATALOG command + this->sendCmd_BUILD_CATALOG(0, 0); + this->component.doDispatch(); + + // 5. Command should generate event instead of ASSERT + ASSERT_CMD_RESPONSE_SIZE(1); + // ASSERT_CMD_RESPONSE(0, DpCatalogComponentBase::OPCODE_BUILD_CATALOG, 0, Fw::CmdResponse::EXECUTION_ERROR); + + // High-priority warning event should be caught by this test + ASSERT_EVENTS_SIZE(2); + ASSERT_EVENTS_FileCorruptedDataError_SIZE(1); + ASSERT_EVENTS_FileCorruptedDataError(0, stateFile.toChar(), static_cast(Fw::FW_DESERIALIZE_FORMAT_ERROR)); + + // 6. Cleanup + Os::FileSystem::removeFile(stateFile.toChar()); + this->component.shutdown(); +} + } // namespace Svc diff --git a/Svc/DpCatalog/test/ut/DpCatalogTester.hpp b/Svc/DpCatalog/test/ut/DpCatalogTester.hpp index 9a0c3cacd43..6d72daebcbf 100644 --- a/Svc/DpCatalog/test/ut/DpCatalogTester.hpp +++ b/Svc/DpCatalog/test/ut/DpCatalogTester.hpp @@ -149,6 +149,7 @@ class DpCatalogTester : public DpCatalogGTestBase { void test_PingIn(); void test_BadFileDone(); void test_ProcessFileInvalidDir(); + void test_MalformedFile(); }; } // namespace Svc diff --git a/Svc/FprimeFramer/FprimeFramer.cpp b/Svc/FprimeFramer/FprimeFramer.cpp index b802c3806cb..4a14fe2ce79 100644 --- a/Svc/FprimeFramer/FprimeFramer.cpp +++ b/Svc/FprimeFramer/FprimeFramer.cpp @@ -55,6 +55,8 @@ void FprimeFramer ::dataIn_handler(FwIndexType portNum, Fw::Buffer& data, const trailer.set_crcField(hashBuffer.asBigEndianU32()); status = frameSerializer.serializeFrom(trailer); FW_ASSERT(status == Fw::FW_SERIALIZE_OK, status); + // Trim to actual frame size in case allocator returned a larger buffer + frameBuffer.setSize(static_cast(frameSize)); // Send the full frame out - this port shall always be connected this->dataOut_out(0, frameBuffer, context); diff --git a/Svc/FprimeFramer/test/ut/FprimeFramerTestMain.cpp b/Svc/FprimeFramer/test/ut/FprimeFramerTestMain.cpp index e1acc1be269..be503412cff 100644 --- a/Svc/FprimeFramer/test/ut/FprimeFramerTestMain.cpp +++ b/Svc/FprimeFramer/test/ut/FprimeFramerTestMain.cpp @@ -21,6 +21,11 @@ TEST(Nominal, testNominalFraming) { tester.testNominalFraming(); } +TEST(OffNominal, OversizedAllocatorBufferIsTrimmed) { + Svc::FprimeFramerTester tester; + tester.testOversizedAllocatorBufferIsTrimmed(); +} + int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); diff --git a/Svc/FprimeFramer/test/ut/FprimeFramerTester.cpp b/Svc/FprimeFramer/test/ut/FprimeFramerTester.cpp index ecd91d35b17..b50e7cec0d6 100644 --- a/Svc/FprimeFramer/test/ut/FprimeFramerTester.cpp +++ b/Svc/FprimeFramer/test/ut/FprimeFramerTester.cpp @@ -78,6 +78,33 @@ void FprimeFramerTester ::testNominalFraming() { } } +void FprimeFramerTester ::testOversizedAllocatorBufferIsTrimmed() { + U8 bufferData[100]; + Fw::Buffer buffer(bufferData, sizeof(bufferData)); + ComCfg::FrameContext context; + + for (U32 i = 0; i < sizeof(bufferData); ++i) { + bufferData[i] = static_cast(i); + } + + // Signal the allocator handler to return a larger-than-needed buffer, + // simulating a BufferManager bin that is bigger than the exact frame size. + this->m_useOversizedAlloc = true; + this->invoke_to_dataIn(0, buffer, context); + this->m_useOversizedAlloc = false; + + ASSERT_from_dataOut_SIZE(1); + ASSERT_from_dataReturnOut_SIZE(1); + + const FwSizeType expectedFrameSize = sizeof(bufferData) + FprimeProtocol::FrameHeader::SERIALIZED_SIZE + + FprimeProtocol::FrameTrailer::SERIALIZED_SIZE; + + Fw::Buffer outputBuffer = this->fromPortHistory_dataOut->at(0).data; + // If setSize() is missing from FprimeFramer, getSize() returns the + // oversized allocation (2 * expectedFrameSize) and this assertion fails. + ASSERT_EQ(outputBuffer.getSize(), expectedFrameSize); +} + // ---------------------------------------------------------------------- // Test Harness: Handler implementations for output ports // ---------------------------------------------------------------------- diff --git a/Svc/FprimeFramer/test/ut/FprimeFramerTester.hpp b/Svc/FprimeFramer/test/ut/FprimeFramerTester.hpp index fd0ecdd9ea0..76b984b5152 100644 --- a/Svc/FprimeFramer/test/ut/FprimeFramerTester.hpp +++ b/Svc/FprimeFramer/test/ut/FprimeFramerTester.hpp @@ -49,6 +49,9 @@ class FprimeFramerTester final : public FprimeFramerGTestBase { //! Test framing of data void testNominalFraming(); + //! Test oversized buffer allocation (trimming) of data + void testOversizedAllocatorBufferIsTrimmed(); + private: // ---------------------------------------------------------------------- // Helper functions @@ -76,6 +79,9 @@ class FprimeFramerTester final : public FprimeFramerGTestBase { U8 m_buffer_slot[2048]; Fw::Buffer m_buffer; // buffer to be returned by mocked allocate call + + // Flag read by from_bufferAllocate_handler to choose which path to take + bool m_useOversizedAlloc = false; }; } // namespace Svc diff --git a/Svc/FpySequencer/FpySequencerRunState.cpp b/Svc/FpySequencer/FpySequencerRunState.cpp index 3bee80c44c7..4a5dde78e62 100644 --- a/Svc/FpySequencer/FpySequencerRunState.cpp +++ b/Svc/FpySequencer/FpySequencerRunState.cpp @@ -170,8 +170,8 @@ Fw::Success FpySequencer::deserializeDirective(const Fpy::Statement& stmt, Direc } // okay, it will fit. put it in - status = argBuf.deserializeTo(deserializedDirective.constCmd.get_argBuf(), cmdArgBufSize, - Fw::Serialization::OMIT_LENGTH); + status = argBuf.deserializeTo(deserializedDirective.constCmd.get_argBuf(), Fpy::MAX_DIRECTIVE_SIZE, + cmdArgBufSize, Fw::Serialization::OMIT_LENGTH); if (status != Fw::SerializeStatus::FW_SERIALIZE_OK) { this->log_WARNING_HI_DirectiveDeserializeError(stmt.get_opCode(), this->currentStatementIdx(), status, @@ -301,8 +301,8 @@ Fw::Success FpySequencer::deserializeDirective(const Fpy::Statement& stmt, Direc } // okay, it will fit. put it in - status = - argBuf.deserializeTo(deserializedDirective.pushVal.get_val(), bufSize, Fw::Serialization::OMIT_LENGTH); + status = argBuf.deserializeTo(deserializedDirective.pushVal.get_val(), Fpy::MAX_DIRECTIVE_SIZE, bufSize, + Fw::Serialization::OMIT_LENGTH); if (status != Fw::SerializeStatus::FW_SERIALIZE_OK) { this->log_WARNING_HI_DirectiveDeserializeError(stmt.get_opCode(), this->currentStatementIdx(), status, diff --git a/Svc/PrmDb/PrmDbImpl.cpp b/Svc/PrmDb/PrmDbImpl.cpp index 6dfe6b7aa34..d10a5b71970 100644 --- a/Svc/PrmDb/PrmDbImpl.cpp +++ b/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); diff --git a/Svc/PrmDb/test/ut/PrmDbTestMain.cpp b/Svc/PrmDb/test/ut/PrmDbTestMain.cpp index 440031277fa..5d3da7b31ce 100644 --- a/Svc/PrmDb/test/ut/PrmDbTestMain.cpp +++ b/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(); diff --git a/Svc/PrmDb/test/ut/PrmDbTester.cpp b/Svc/PrmDb/test/ut/PrmDbTester.cpp index f13ed6b257f..4ae39f859cf 100644 --- a/Svc/PrmDb/test/ut/PrmDbTester.cpp +++ b/Svc/PrmDb/test/ut/PrmDbTester.cpp @@ -1317,6 +1317,127 @@ void PrmDbTester::runPrmFileLoadIllegal() { ASSERT_EVENTS_PrmIdAdded_SIZE(1); } +void PrmDbTester::runShorterSaveDoesNotCorrupt() { + 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(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) { diff --git a/Svc/PrmDb/test/ut/PrmDbTester.hpp b/Svc/PrmDb/test/ut/PrmDbTester.hpp index 5077fdd9338..fcf5b2a74f2 100644 --- a/Svc/PrmDb/test/ut/PrmDbTester.hpp +++ b/Svc/PrmDb/test/ut/PrmDbTester.hpp @@ -32,6 +32,7 @@ class PrmDbTester : public PrmDbGTestBase { void runPrmFileLoadNominal(); void runPrmFileLoadWithErrors(); void runPrmFileLoadIllegal(); + void runShorterSaveDoesNotCorrupt(); void runRefPrmFile();