From a8fe137283b8a3a8b46e02b554fd12e4f8645a6b Mon Sep 17 00:00:00 2001 From: M Starch Date: Wed, 3 Dec 2025 12:17:00 -0800 Subject: [PATCH] Fix FPrimeRouter potential usage of invalid buffer (#4493) * Fix #4491 * Formatting and enum size --- Svc/FprimeRouter/FprimeRouter.cpp | 37 +++++++++++++------ Svc/FprimeRouter/FprimeRouter.fpp | 9 +++++ .../test/ut/FprimeRouterTestMain.cpp | 10 +++++ .../test/ut/FprimeRouterTester.cpp | 27 ++++++++++++-- .../test/ut/FprimeRouterTester.hpp | 7 ++++ 5 files changed, 75 insertions(+), 15 deletions(-) diff --git a/Svc/FprimeRouter/FprimeRouter.cpp b/Svc/FprimeRouter/FprimeRouter.cpp index 83ff88c14b..09d8cbedf0 100644 --- a/Svc/FprimeRouter/FprimeRouter.cpp +++ b/Svc/FprimeRouter/FprimeRouter.cpp @@ -51,12 +51,18 @@ void FprimeRouter ::dataIn_handler(FwIndexType portNum, Fw::Buffer& packetBuffer // Copy buffer into a new allocated buffer. This lets us return the original buffer with dataReturnOut, // and FprimeRouter can handle the deallocation of the file buffer when it returns on fileBufferReturnIn Fw::Buffer packetBufferCopy = this->bufferAllocate_out(0, packetBuffer.getSize()); - auto copySerializer = packetBufferCopy.getSerializer(); - status = copySerializer.serializeFrom(packetBuffer.getData(), packetBuffer.getSize(), - Fw::Serialization::OMIT_LENGTH); - FW_ASSERT(status == Fw::FW_SERIALIZE_OK, status); - // Send the copied buffer out. It will come back on fileBufferReturnIn once the receiver is done with it - this->fileOut_out(0, packetBufferCopy); + // Confirm we got a valid buffer before using it + if (packetBufferCopy.isValid()) { + auto copySerializer = packetBufferCopy.getSerializer(); + status = copySerializer.serializeFrom(packetBuffer.getData(), packetBuffer.getSize(), + Fw::Serialization::OMIT_LENGTH); + FW_ASSERT(status == Fw::FW_SERIALIZE_OK, status); + // Send the copied buffer out. It will come back on fileBufferReturnIn once the receiver is done + // with it + this->fileOut_out(0, packetBufferCopy); + } else { + this->log_WARNING_HI_AllocationError(FprimeRouter_AllocationReason::FILE_UPLINK); + } } break; } @@ -67,13 +73,20 @@ void FprimeRouter ::dataIn_handler(FwIndexType portNum, Fw::Buffer& packetBuffer // Copy buffer into a new allocated buffer. This lets us return the original buffer with dataReturnOut, // and FprimeRouter can handle the deallocation of the unknown buffer when it returns on bufferReturnIn Fw::Buffer packetBufferCopy = this->bufferAllocate_out(0, packetBuffer.getSize()); - auto copySerializer = packetBufferCopy.getSerializer(); - status = copySerializer.serializeFrom(packetBuffer.getData(), packetBuffer.getSize(), - Fw::Serialization::OMIT_LENGTH); - FW_ASSERT(status == Fw::FW_SERIALIZE_OK, status); - // Send the copied buffer out. It will come back on fileBufferReturnIn once the receiver is done with it - this->unknownDataOut_out(0, packetBufferCopy, context); + // Confirm we got a valid buffer before using it + if (packetBufferCopy.isValid()) { + auto copySerializer = packetBufferCopy.getSerializer(); + status = copySerializer.serializeFrom(packetBuffer.getData(), packetBuffer.getSize(), + Fw::Serialization::OMIT_LENGTH); + FW_ASSERT(status == Fw::FW_SERIALIZE_OK, status); + // Send the copied buffer out. It will come back on fileBufferReturnIn once the receiver is done + // with it + this->unknownDataOut_out(0, packetBufferCopy, context); + } else { + this->log_WARNING_HI_AllocationError(FprimeRouter_AllocationReason::USER_BUFFER); + } } + break; } } diff --git a/Svc/FprimeRouter/FprimeRouter.fpp b/Svc/FprimeRouter/FprimeRouter.fpp index 39920bb6e4..815bff3107 100644 --- a/Svc/FprimeRouter/FprimeRouter.fpp +++ b/Svc/FprimeRouter/FprimeRouter.fpp @@ -7,6 +7,11 @@ module Svc { # ---------------------------------------------------------------------- import Router + enum AllocationReason : U8{ + FILE_UPLINK, @< Buffer allocation for file uplink + USER_BUFFER @< Buffer allocation for user handled buffer + } + @ Port for forwarding non-recognized packet types @ Ownership of the buffer is retained by the FprimeRouter, meaning receiving @ components should either process data synchronously, or copy the data if needed @@ -31,6 +36,10 @@ module Svc { ) \ severity warning high \ format "Deserializing packet type failed with status {}" + + @ An allocation error occurred + event AllocationError(reason: AllocationReason) severity warning high \ + format "Buffer allocation for {} failed" ############################################################################### diff --git a/Svc/FprimeRouter/test/ut/FprimeRouterTestMain.cpp b/Svc/FprimeRouter/test/ut/FprimeRouterTestMain.cpp index 08053af6a3..f9a546ee2a 100644 --- a/Svc/FprimeRouter/test/ut/FprimeRouterTestMain.cpp +++ b/Svc/FprimeRouter/test/ut/FprimeRouterTestMain.cpp @@ -28,6 +28,16 @@ TEST(FprimeRouter, TestRouteUnknownPacketUnconnected) { Svc::FprimeRouterTester tester(true); tester.testRouteUnknownPacketUnconnected(); } +TEST(FprimeRouter, TestAllocationFailureFile) { + COMMENT("Test failure to allocate for files"); + Svc::FprimeRouterTester tester; + tester.testAllocationFailureFile(); +} +TEST(FprimeRouter, TestAllocationFailureUnknown) { + COMMENT("Test failure to allocate for unknown packets"); + Svc::FprimeRouterTester tester; + tester.testAllocationFailureUnknown(); +} TEST(FprimeRouter, TestBufferReturn) { COMMENT("Deallocate a returning buffer"); Svc::FprimeRouterTester tester; diff --git a/Svc/FprimeRouter/test/ut/FprimeRouterTester.cpp b/Svc/FprimeRouter/test/ut/FprimeRouterTester.cpp index 037bad1ba4..0dffffd0d8 100644 --- a/Svc/FprimeRouter/test/ut/FprimeRouterTester.cpp +++ b/Svc/FprimeRouter/test/ut/FprimeRouterTester.cpp @@ -64,6 +64,22 @@ void FprimeRouterTester ::testRouteUnknownPacketUnconnected() { ASSERT_from_bufferAllocate_SIZE(0); // no buffer allocation when port is unconnected } +void FprimeRouterTester ::testAllocationFailureFile() { + this->m_forceAllocationError = true; + this->mockReceivePacketType(Fw::ComPacketType::FW_PACKET_FILE); + ASSERT_EVENTS_AllocationError_SIZE(1); // allocation error should be logged + ASSERT_EVENTS_AllocationError(0, FprimeRouter_AllocationReason::FILE_UPLINK); + ASSERT_from_dataReturnOut_SIZE(1); // data ownership should always be returned +} + +void FprimeRouterTester ::testAllocationFailureUnknown() { + this->m_forceAllocationError = true; + this->mockReceivePacketType(Fw::ComPacketType::FW_PACKET_UNKNOWN); + ASSERT_EVENTS_AllocationError_SIZE(1); // allocation error should be logged + ASSERT_EVENTS_AllocationError(0, FprimeRouter_AllocationReason::USER_BUFFER); + ASSERT_from_dataReturnOut_SIZE(1); // data ownership should always be returned +} + void FprimeRouterTester ::testBufferReturn() { U8 data[1]; Fw::Buffer buffer(data, sizeof(data)); @@ -116,9 +132,14 @@ void FprimeRouterTester::connectPortsExceptUnknownData() { // ---------------------------------------------------------------------- Fw::Buffer FprimeRouterTester::from_bufferAllocate_handler(FwIndexType portNum, FwSizeType size) { this->pushFromPortEntry_bufferAllocate(size); - this->m_buffer.setData(this->m_buffer_slot); - this->m_buffer.setSize(size); - ::memset(this->m_buffer.getData(), 0, size); + if (this->m_forceAllocationError) { + this->m_buffer.setData(nullptr); + this->m_buffer.setSize(0); + } else { + this->m_buffer.setData(this->m_buffer_slot); + this->m_buffer.setSize(size); + ::memset(this->m_buffer.getData(), 0, size); + } return this->m_buffer; } diff --git a/Svc/FprimeRouter/test/ut/FprimeRouterTester.hpp b/Svc/FprimeRouter/test/ut/FprimeRouterTester.hpp index 120c0d5fa2..69406cf445 100644 --- a/Svc/FprimeRouter/test/ut/FprimeRouterTester.hpp +++ b/Svc/FprimeRouter/test/ut/FprimeRouterTester.hpp @@ -56,6 +56,12 @@ class FprimeRouterTester : public FprimeRouterGTestBase { //! Route a packet of unknown type void testRouteUnknownPacketUnconnected(); + //! Route a packet of unknown type + void testAllocationFailureFile(); + + //! Deallocate a returning buffer + void testAllocationFailureUnknown(); + //! Deallocate a returning buffer void testBufferReturn(); @@ -95,6 +101,7 @@ class FprimeRouterTester : public FprimeRouterGTestBase { Fw::Buffer m_buffer; // buffer to be returned by mocked bufferAllocate call U8 m_buffer_slot[64]; + bool m_forceAllocationError = false; // Flag to force allocation error }; } // namespace Svc