From 251d29f4f49a1b58ad684d924eb9b7b5ae2287f0 Mon Sep 17 00:00:00 2001 From: bitWarrior Date: Fri, 9 Oct 2026 08:05:28 -0700 Subject: [PATCH] Handle descriptor-only file packets as off-nominal input A file packet consisting of only the FW_PACKET_FILE descriptor with no body passed the minimum-size guards in FileUplink and CfdpManager, which then built a zero-length buffer over a valid pointer. Since the LinearBufferBase refactor, its constructor required a null pointer whenever capacity is zero, so this input tripped an assertion instead of being reported as a decode error. - Fw/Types: LinearBufferBase accepts a valid pointer with zero capacity, matching ExternalSerializeBuffer::setExtBuffer and prior behavior. A null pointer with nonzero capacity still asserts. - Svc/FileUplink: reject packets that carry no bytes after the descriptor, logging InvalidPacketReceived as for shorter packets. - Svc/Ccsds/CfdpManager: same boundary check on dataIn. Tests cover zero-capacity buffers at the Fw/Types level and 1-, 2- and 3-byte packets for both components. Co-Authored-By: Claude Fable 5.1 --- Fw/Types/Serializable.cpp | 7 ++- .../test/ut/ExternalSerializeBufferTest.cpp | 49 +++++++++++++++++ Svc/Ccsds/CfdpManager/CfdpManager.cpp | 4 +- .../test/ut/CfdpManagerCommandTests.cpp | 41 ++++++++++++++ .../test/ut/CfdpManagerTestMain.cpp | 10 ++++ .../CfdpManager/test/ut/CfdpManagerTester.hpp | 6 +++ Svc/FileUplink/FileUplink.cpp | 5 +- Svc/FileUplink/test/ut/FileUplinkMain.cpp | 15 ++++++ Svc/FileUplink/test/ut/FileUplinkTester.cpp | 53 +++++++++++++++++++ Svc/FileUplink/test/ut/FileUplinkTester.hpp | 5 ++ 10 files changed, 189 insertions(+), 6 deletions(-) diff --git a/Fw/Types/Serializable.cpp b/Fw/Types/Serializable.cpp index 2ce8f30207d..8711f3d8d9e 100644 --- a/Fw/Types/Serializable.cpp +++ b/Fw/Types/Serializable.cpp @@ -39,8 +39,11 @@ SerialBufferBase::~SerialBufferBase() {} LinearBufferBase::LinearBufferBase(U8* buffAddr, Serializable::SizeType capacity) : m_buffAddr(buffAddr), m_capacity(capacity), m_serLoc(0), m_deserLoc(0) { - // Ensure buffAddr & capacity are consistent - FW_ASSERT((buffAddr == nullptr) == (capacity == 0), static_cast(capacity)); + // A null buffer must have zero capacity. A valid buffer may have zero capacity: + // callers routinely wrap the (possibly empty) tail of a larger buffer, such as a + // packet that carries a descriptor but no body. This matches the contract of + // ExternalSerializeBuffer::setExtBuffer. + FW_ASSERT((buffAddr != nullptr) || (capacity == 0), static_cast(capacity)); } LinearBufferBase::~LinearBufferBase() {} diff --git a/Fw/Types/test/ut/ExternalSerializeBufferTest.cpp b/Fw/Types/test/ut/ExternalSerializeBufferTest.cpp index 35ded9aa78b..b4f1d3597a6 100644 --- a/Fw/Types/test/ut/ExternalSerializeBufferTest.cpp +++ b/Fw/Types/test/ut/ExternalSerializeBufferTest.cpp @@ -1,6 +1,7 @@ #include #include +#include "Fw/Types/SerialBuffer.hpp" #include "Fw/Types/Serializable.hpp" namespace ExternalSerializeBufferTest { @@ -98,3 +99,51 @@ TEST(ExternalSerializeBufferWithMemberCopy, Assign) { } } // namespace ExternalSerializeBufferTest + +// ---------------------------------------------------------------------- +// LinearBufferBase constructor contract +// ---------------------------------------------------------------------- + +namespace LinearBufferBaseCtorTest { + +U8 backing[4]; + +// A valid pointer with zero capacity is a legal, empty buffer. Ingest paths build one +// when a packet carries a descriptor but no body, so it must not assert. +TEST(LinearBufferBase, ValidPointerZeroCapacity) { + Fw::ExternalSerializeBuffer esb(backing, 0); + ASSERT_EQ(esb.getCapacity(), 0); + ASSERT_EQ(esb.getSize(), 0); + const U8 value = 1; + ASSERT_EQ(esb.serializeFrom(value), Fw::FW_SERIALIZE_NO_ROOM_LEFT); + U8 out = 0; + ASSERT_EQ(esb.deserializeTo(out), Fw::FW_DESERIALIZE_BUFFER_EMPTY); +} + +TEST(SerialBuffer, ValidPointerZeroCapacity) { + Fw::SerialBuffer sb(backing, 0); + sb.fill(); + ASSERT_EQ(sb.getCapacity(), 0); + ASSERT_EQ(sb.getSize(), 0); + U8 out = 0; + ASSERT_EQ(sb.deserializeTo(out), Fw::FW_DESERIALIZE_BUFFER_EMPTY); +} + +// Null with zero capacity remains legal (used for initialization and reset) +TEST(LinearBufferBase, NullPointerZeroCapacity) { + Fw::ExternalSerializeBuffer esb(nullptr, 0); + ASSERT_EQ(esb.getCapacity(), 0); + ASSERT_EQ(esb.getSize(), 0); +} + +// Null with nonzero capacity is the only inconsistent pairing and must still assert +TEST(LinearBufferBaseDeathTest, NullPointerNonzeroCapacity) { + ASSERT_DEATH( + { + Fw::ExternalSerializeBuffer esb(nullptr, 1); + (void)esb; + }, + ".*"); +} + +} // namespace LinearBufferBaseCtorTest diff --git a/Svc/Ccsds/CfdpManager/CfdpManager.cpp b/Svc/Ccsds/CfdpManager/CfdpManager.cpp index 48367d302c7..fae94a4fb4a 100644 --- a/Svc/Ccsds/CfdpManager/CfdpManager.cpp +++ b/Svc/Ccsds/CfdpManager/CfdpManager.cpp @@ -159,8 +159,8 @@ void CfdpManager ::dataIn_handler(FwIndexType portNum, Fw::Buffer& fwBuffer) { // Strip FW_PACKET_FILE descriptor (first 2 bytes) from buffer // FprimeRouter sends the entire Space Packet data field, which includes the packet type descriptor - if (fwBuffer.getSize() < sizeof(FwPacketDescriptorType)) { - // Buffer too small - silently ignore + if (fwBuffer.getSize() <= sizeof(FwPacketDescriptorType)) { + // Buffer too small to hold the descriptor plus any PDU bytes - silently ignore this->dataInReturn_out(portNum, fwBuffer); return; } diff --git a/Svc/Ccsds/CfdpManager/test/ut/CfdpManagerCommandTests.cpp b/Svc/Ccsds/CfdpManager/test/ut/CfdpManagerCommandTests.cpp index ab19499c3be..4fc0c38c0d7 100644 --- a/Svc/Ccsds/CfdpManager/test/ut/CfdpManagerCommandTests.cpp +++ b/Svc/Ccsds/CfdpManager/test/ut/CfdpManagerCommandTests.cpp @@ -1180,6 +1180,47 @@ void CfdpManagerTester::testDataInWrongDescriptor() { ASSERT_EVENTS_SIZE(0); } +void CfdpManagerTester::testDataInDescriptorOnly() { + // dataIn_handler early-return: buffer holding exactly the 2-byte FW_PACKET_FILE + // descriptor and no PDU bytes. Before the fix this reached Engine::receivePdu with a + // zero-length buffer and tripped the LinearBufferBase constructor assertion. + + this->clearHistory(); + + const FwPacketDescriptorType descriptor = static_cast(Fw::ComPacketType::FW_PACKET_FILE); + U8 data[sizeof(FwPacketDescriptorType)]; + data[0] = static_cast((descriptor >> 8) & 0xFF); + data[1] = static_cast(descriptor & 0xFF); + Fw::Buffer buffer(data, sizeof(data)); + + this->invoke_to_dataIn(0, buffer); + this->component.doDispatch(); + + // Buffer returned to the sender, engine never invoked, no events emitted + ASSERT_from_dataInReturn_SIZE(1); + ASSERT_EVENTS_SIZE(0); +} + +void CfdpManagerTester::testDataInDescriptorPlusOneByte() { + // Control: descriptor plus a single PDU byte reaches the engine, which rejects the + // truncated header with an event rather than an assertion. + + this->clearHistory(); + + const FwPacketDescriptorType descriptor = static_cast(Fw::ComPacketType::FW_PACKET_FILE); + U8 data[sizeof(FwPacketDescriptorType) + 1]; + data[0] = static_cast((descriptor >> 8) & 0xFF); + data[1] = static_cast(descriptor & 0xFF); + data[2] = 0x00; + Fw::Buffer buffer(data, sizeof(data)); + + this->invoke_to_dataIn(0, buffer); + this->component.doDispatch(); + + ASSERT_from_dataInReturn_SIZE(1); + ASSERT_EVENTS_FailPduHeaderDeserialization_SIZE(1); +} + void CfdpManagerTester::testGetPduBufferMaxOutgoing() { // getPduBuffer: when the channel's outgoing PDU counter has reached the // per-cycle maximum, no buffer is allocated and SEND_PDU_NO_BUF_AVAIL_ERROR diff --git a/Svc/Ccsds/CfdpManager/test/ut/CfdpManagerTestMain.cpp b/Svc/Ccsds/CfdpManager/test/ut/CfdpManagerTestMain.cpp index 7154a23330f..53da778247b 100644 --- a/Svc/Ccsds/CfdpManager/test/ut/CfdpManagerTestMain.cpp +++ b/Svc/Ccsds/CfdpManager/test/ut/CfdpManagerTestMain.cpp @@ -687,6 +687,16 @@ TEST(Coverage, DataInWrongDescriptor) { tester.testDataInWrongDescriptor(); } +TEST(Coverage, DataInDescriptorOnly) { + Svc::Ccsds::Cfdp::CfdpManagerTester tester; + tester.testDataInDescriptorOnly(); +} + +TEST(Coverage, DataInDescriptorPlusOneByte) { + Svc::Ccsds::Cfdp::CfdpManagerTester tester; + tester.testDataInDescriptorPlusOneByte(); +} + TEST(Coverage, GetPduBufferMaxOutgoing) { Svc::Ccsds::Cfdp::CfdpManagerTester tester; tester.testGetPduBufferMaxOutgoing(); diff --git a/Svc/Ccsds/CfdpManager/test/ut/CfdpManagerTester.hpp b/Svc/Ccsds/CfdpManager/test/ut/CfdpManagerTester.hpp index 4bae2db000e..dc27f281803 100644 --- a/Svc/Ccsds/CfdpManager/test/ut/CfdpManagerTester.hpp +++ b/Svc/Ccsds/CfdpManager/test/ut/CfdpManagerTester.hpp @@ -668,6 +668,12 @@ class CfdpManagerTester final : public CfdpManagerGTestBase { //! dataIn_handler: packet descriptor is not FW_PACKET_FILE void testDataInWrongDescriptor(); + //! dataIn_handler: FW_PACKET_FILE descriptor with no PDU bytes after it + void testDataInDescriptorOnly(); + + //! dataIn_handler: FW_PACKET_FILE descriptor plus one PDU byte (truncated header) + void testDataInDescriptorPlusOneByte(); + //! getPduBuffer: max outgoing PDUs per cycle reached void testGetPduBufferMaxOutgoing(); diff --git a/Svc/FileUplink/FileUplink.cpp b/Svc/FileUplink/FileUplink.cpp index 0086321f0aa..5e420d84d4a 100644 --- a/Svc/FileUplink/FileUplink.cpp +++ b/Svc/FileUplink/FileUplink.cpp @@ -42,8 +42,9 @@ void FileUplink::configure(const char* directory) { // ---------------------------------------------------------------------- void FileUplink::bufferSendIn_handler(const FwIndexType portNum, Fw::Buffer& buffer) { - // If packet is too small to contain a packet type, log + deallocate and return - if (buffer.getSize() < sizeof(FwPacketDescriptorType)) { + // If packet is too small to contain a packet type plus at least one byte of file + // packet body, log + deallocate and return. A descriptor-only packet has nothing to decode. + if (buffer.getSize() <= sizeof(FwPacketDescriptorType)) { this->log_WARNING_HI_InvalidPacketReceived(Fw::ComPacketType::FW_PACKET_UNKNOWN); this->bufferSendOut_out(0, buffer); return; diff --git a/Svc/FileUplink/test/ut/FileUplinkMain.cpp b/Svc/FileUplink/test/ut/FileUplinkMain.cpp index 9e1492d5774..e442eb86898 100644 --- a/Svc/FileUplink/test/ut/FileUplinkMain.cpp +++ b/Svc/FileUplink/test/ut/FileUplinkMain.cpp @@ -64,6 +64,21 @@ TEST(FileUplink, CancelPacketInDataMode) { tester.cancelPacketInDataMode(); } +TEST(FileUplink, TooShortPacket) { + Svc::FileUplinkTester tester; + tester.tooShortPacket(); +} + +TEST(FileUplink, DescriptorOnlyPacket) { + Svc::FileUplinkTester tester; + tester.descriptorOnlyPacket(); +} + +TEST(FileUplink, TruncatedBodyPacket) { + Svc::FileUplinkTester tester; + tester.truncatedBodyPacket(); +} + int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); diff --git a/Svc/FileUplink/test/ut/FileUplinkTester.cpp b/Svc/FileUplink/test/ut/FileUplinkTester.cpp index 4ab9cca8259..ad6ad83716c 100644 --- a/Svc/FileUplink/test/ut/FileUplinkTester.cpp +++ b/Svc/FileUplink/test/ut/FileUplinkTester.cpp @@ -397,6 +397,40 @@ void FileUplinkTester ::cancelPacketInDataMode() { this->removeFile("test.bin"); } +void FileUplinkTester ::tooShortPacket() { + // One byte cannot hold a packet descriptor + U8 data[1] = {0xFF}; + this->sendRawPacket(data, sizeof(data)); + + ASSERT_EVENTS_SIZE(1); + ASSERT_EVENTS_InvalidPacketReceived(0, static_cast(Fw::ComPacketType::FW_PACKET_UNKNOWN)); + ASSERT_TLM_SIZE(0); +} + +void FileUplinkTester ::descriptorOnlyPacket() { + // Exactly the FW_PACKET_FILE descriptor with no body. Before the fix this reached + // FilePacket::fromBuffer with a zero-length buffer and tripped an assertion. + U8 data[sizeof(FwPacketDescriptorType)]; + this->writeFileDescriptor(data); + this->sendRawPacket(data, sizeof(data)); + + ASSERT_EVENTS_SIZE(1); + ASSERT_EVENTS_InvalidPacketReceived(0, static_cast(Fw::ComPacketType::FW_PACKET_UNKNOWN)); + ASSERT_TLM_SIZE(0); +} + +void FileUplinkTester ::truncatedBodyPacket() { + // Descriptor plus one body byte: too short for a file packet header, so a decode error + U8 data[sizeof(FwPacketDescriptorType) + 1]; + this->writeFileDescriptor(data); + data[sizeof(FwPacketDescriptorType)] = 0; + this->sendRawPacket(data, sizeof(data)); + + ASSERT_EVENTS_SIZE(1); + ASSERT_EVENTS_DecodeError_SIZE(1); + ASSERT_TLM_SIZE(0); +} + // ---------------------------------------------------------------------- // Handlers for from ports // ---------------------------------------------------------------------- @@ -447,6 +481,25 @@ void FileUplinkTester ::initComponents() { this->component.init(QUEUE_DEPTH, INSTANCE); } +void FileUplinkTester ::sendRawPacket(U8* const data, const size_t size) { + this->clearHistory(); + + Fw::Buffer buffer(data, static_cast(size)); + this->invoke_to_bufferSendIn(0, buffer); + this->component.doDispatch(); + + // The buffer is always handed back to the sender + ASSERT_from_bufferSendOut_SIZE(1); + ASSERT_from_bufferSendOut(0, buffer); +} + +void FileUplinkTester ::writeFileDescriptor(U8* const data) { + Fw::Buffer buffer(data, sizeof(FwPacketDescriptorType)); + const Fw::SerializeStatus status = + buffer.getSerializer().serializeFrom(static_cast(Fw::ComPacketType::FW_PACKET_FILE)); + ASSERT_EQ(Fw::FW_SERIALIZE_OK, status); +} + void FileUplinkTester ::sendFilePacket(const Fw::FilePacket& filePacket) { this->clearHistory(); diff --git a/Svc/FileUplink/test/ut/FileUplinkTester.hpp b/Svc/FileUplink/test/ut/FileUplinkTester.hpp index fa3e0ea5740..48c435e8da4 100644 --- a/Svc/FileUplink/test/ut/FileUplinkTester.hpp +++ b/Svc/FileUplink/test/ut/FileUplinkTester.hpp @@ -86,6 +86,9 @@ class FileUplinkTester : public FileUplinkGTestBase { //! Send a CANCEL packet in DATA mode //! void cancelPacketInDataMode(); + void tooShortPacket(); + void descriptorOnlyPacket(); + void truncatedBodyPacket(); private: // ---------------------------------------------------------------------- @@ -138,6 +141,8 @@ class FileUplinkTester : public FileUplinkGTestBase { //! Send a CancelPacket //! void sendCancelPacket(); + void sendRawPacket(U8* const data, const size_t size); + void writeFileDescriptor(U8* const data); //! Verify file data //!