Fix ComQueue Does Not Deallocate Buffers on Overflow #2853

DJKessler · 2024-08-30T18:28:42Z


*Related Issue(s)*	#2066
*Has Unit Tests (y/n)*	y
*Documentation Included (y/n)*	y

Change Description

Fix potential memory leak when a sent Fw::Buffer overflows ComQueue.

Added Fw.BufferSend deallocate port to ComQueue
Updated FPP to v2.2.0a2
Change buffQueueIn port from drop to hook
Added queue overflow hook method implementation
Added return status to ComQueue::enqueue()
Fw::Buffer is now deallocated on queue overflow
Enabled UT_AUTO_HELPERS in ComQueue UT build
Updated ComQueue UTs

Svc/ComQueue/ComQueue.cpp

@@ -139,7 +139,11 @@
    // Ensure that the port number of buffQueueIn is consistent with the expectation
    FW_ASSERT(portNum >= 0 && portNum < BUFFER_PORT_COUNT, portNum);
    FW_ASSERT(queueNum < TOTAL_PORT_COUNT);
-    this->enqueue(queueNum, QueueType::BUFFER_QUEUE, reinterpret_cast<const U8*>(&fwBuffer), sizeof(Fw::Buffer));
+    bool status =


Svc/ComQueue/ComQueue.cpp

 // ----------------------------------------------------------------------
 // Private helper methods
 // ----------------------------------------------------------------------

-void ComQueue::enqueue(const FwIndexType queueNum, QueueType queueType, const U8* data, const FwSizeType size) {
+bool ComQueue::enqueue(const FwIndexType queueNum, QueueType queueType, const U8* data, const FwSizeType size) {


Svc/ComQueue/ComQueue.cpp

 // ----------------------------------------------------------------------
 // Private helper methods
 // ----------------------------------------------------------------------

-void ComQueue::enqueue(const FwIndexType queueNum, QueueType queueType, const U8* data, const FwSizeType size) {
+bool ComQueue::enqueue(const FwIndexType queueNum, QueueType queueType, const U8* data, const FwSizeType size) {


Svc/ComQueue/ComQueue.cpp

 // ----------------------------------------------------------------------
 // Private helper methods
 // ----------------------------------------------------------------------

-void ComQueue::enqueue(const FwIndexType queueNum, QueueType queueType, const U8* data, const FwSizeType size) {
+bool ComQueue::enqueue(const FwIndexType queueNum, QueueType queueType, const U8* data, const FwSizeType size) {


Svc/ComQueue/ComQueue.cpp

    // Enqueue the given message onto the matching queue. When no space is available then emit the queue overflow event,
    // set the appropriate throttle, and move on. Will assert if passed a message for a depth 0 queue.
    const FwSizeType expectedSize = (queueType == QueueType::COM_QUEUE) ? sizeof(Fw::ComBuffer) : sizeof(Fw::Buffer);
    const FwIndexType portNum = queueNum - ((queueType == QueueType::COM_QUEUE) ? 0 : COM_PORT_COUNT);
+    bool rvStatus = true;


Svc/ComQueue/ComQueue.cpp

 // ----------------------------------------------------------------------
 // Private helper methods
 // ----------------------------------------------------------------------

-void ComQueue::enqueue(const FwIndexType queueNum, QueueType queueType, const U8* data, const FwSizeType size) {
+bool ComQueue::enqueue(const FwIndexType queueNum, QueueType queueType, const U8* data, const FwSizeType size) {


Svc/ComQueue/ComQueue.cpp

@@ -139,7 +139,11 @@
    // Ensure that the port number of buffQueueIn is consistent with the expectation
    FW_ASSERT(portNum >= 0 && portNum < BUFFER_PORT_COUNT, portNum);
    FW_ASSERT(queueNum < TOTAL_PORT_COUNT);
-    this->enqueue(queueNum, QueueType::BUFFER_QUEUE, reinterpret_cast<const U8*>(&fwBuffer), sizeof(Fw::Buffer));
+    bool status =
+        this->enqueue(queueNum, QueueType::BUFFER_QUEUE, reinterpret_cast<const U8*>(&fwBuffer), sizeof(Fw::Buffer));


Svc/ComQueue/ComQueue.cpp

+// ----------------------------------------------------------------------
+
+void ComQueue::buffQueueIn_overflowHook(FwIndexType portNum, Fw::Buffer& fwBuffer) {
+    this->deallocate_out(portNum, fwBuffer);


LeStarch

There are several things that need to be fixed:

In the com queue function, we should explicitly discard the new enqueue status with (void) this->enqueue...
I don't see any tests proving that the drop function is called on overflow. Can we add those.
CI needs to pass

Svc/ComQueue/ComQueue.cpp

Svc/ComQueue/ComQueue.hpp

Svc/ComQueue/ComQueue.cpp

@@ -131,15 +131,19 @@
 void ComQueue::comQueueIn_handler(const NATIVE_INT_TYPE portNum, Fw::ComBuffer& data, U32 context) {
    // Ensure that the port number of comQueueIn is consistent with the expectation
    FW_ASSERT(portNum >= 0 && portNum < COM_PORT_COUNT, portNum);
-    this->enqueue(portNum, QueueType::COM_QUEUE, reinterpret_cast<const U8*>(&data), sizeof(Fw::ComBuffer));
+    (void)this->enqueue(portNum, QueueType::COM_QUEUE, reinterpret_cast<const U8*>(&data), sizeof(Fw::ComBuffer));


Svc/ComQueue/ComQueue.cpp

+// Hook implementations for typed async input ports
+// ----------------------------------------------------------------------
+
+void ComQueue::buffQueueIn_overflowHook(FwIndexType portNum, Fw::Buffer& fwBuffer) {


LeStarch · 2024-09-18T01:09:28Z

Skipping known ci failure. Nice work!

DJKessler added 8 commits August 30, 2024 10:40

Added Fw.BufferSend deallocate port to ComQueue

b048fb2

Updated FPP to v2.2.0a2

95661d0

Change buffQueueIn port from drop to hook

b88e179

Added queue overflow hook method implementation

e47c0df

Added return status to ComQueue::enqueue()

4389f2b

Fw::Buffer is now deallocated on queue overflow

53f7be1

Enabled UT_AUTO_HELPERS in ComQueue UT build

23e46b5

Updated ComQueue UTs

59e3229

github-advanced-security bot found potential problems Sep 3, 2024

View reviewed changes

thomas-bc assigned DJKessler Sep 4, 2024

LeStarch requested changes Sep 4, 2024

View reviewed changes

Svc/ComQueue/ComQueue.cpp Outdated Show resolved Hide resolved

Svc/ComQueue/ComQueue.hpp Outdated Show resolved Hide resolved

DJKessler added 4 commits September 9, 2024 14:17

Merge branch 'devel' into ComQueue-deallocate

cbe88c2

Explicitly discard enqueue() return status

240d848

Replaced overflowhook() call with deallocate()

658d2a4

Fixed comment style

c2e203c

github-advanced-security bot found potential problems Sep 9, 2024

View reviewed changes

DJKessler added 3 commits September 11, 2024 10:09

Renamed UT test case

a7cb539

Added internal queue overflow UT

03dbc05

Added assertion on overflowHook() argument

c90c999

github-advanced-security bot found potential problems Sep 11, 2024

View reviewed changes

DJKessler requested a review from LeStarch September 11, 2024 19:15

LeStarch approved these changes Sep 18, 2024

View reviewed changes

LeStarch merged commit 6a921a1 into nasa:devel Sep 18, 2024
47 of 48 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ComQueue Does Not Deallocate Buffers on Overflow #2853

Fix ComQueue Does Not Deallocate Buffers on Overflow #2853

DJKessler commented Aug 30, 2024

LeStarch left a comment

LeStarch commented Sep 18, 2024

Fix ComQueue Does Not Deallocate Buffers on Overflow #2853

Fix ComQueue Does Not Deallocate Buffers on Overflow #2853

Conversation

DJKessler commented Aug 30, 2024

Change Description

LeStarch left a comment

Choose a reason for hiding this comment

LeStarch commented Sep 18, 2024