Fix FPrimeRouter potential usage of invalid buffer (#4493)

* Fix #4491

* Formatting and enum size

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();
+                // Confirm we got a valid buffer before using it
-                status = copySerializer.serializeFrom(packetBuffer.getData(), packetBuffer.getSize(),
+                if (packetBufferCopy.isValid()) {
-                                                      Fw::Serialization::OMIT_LENGTH);
+                    auto copySerializer = packetBufferCopy.getSerializer();
-                FW_ASSERT(status == Fw::FW_SERIALIZE_OK, status);
+                    status = copySerializer.serializeFrom(packetBuffer.getData(), packetBuffer.getSize(),
-                // Send the copied buffer out. It will come back on fileBufferReturnIn once the receiver is done with it
+                                                          Fw::Serialization::OMIT_LENGTH);
-                this->fileOut_out(0, packetBufferCopy);
+                    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();
+                // Confirm we got a valid buffer before using it
-                status = copySerializer.serializeFrom(packetBuffer.getData(), packetBuffer.getSize(),
+                if (packetBufferCopy.isValid()) {
-                                                      Fw::Serialization::OMIT_LENGTH);
+                    auto copySerializer = packetBufferCopy.getSerializer();
-                FW_ASSERT(status == Fw::FW_SERIALIZE_OK, status);
+                    status = copySerializer.serializeFrom(packetBuffer.getData(), packetBuffer.getSize(),
-                // Send the copied buffer out. It will come back on fileBufferReturnIn once the receiver is done with it
+                                                          Fw::Serialization::OMIT_LENGTH);
-                this->unknownDataOut_out(0, packetBufferCopy, context);
+                    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;
         }
     }
 

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"
 
 
         ###############################################################################

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;

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);
+    if (this->m_forceAllocationError) {
-    this->m_buffer.setSize(size);
+        this->m_buffer.setData(nullptr);
-    ::memset(this->m_buffer.getData(), 0, size);
+        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;
 }
 

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