Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix ComQueue Does Not Deallocate Buffers on Overflow #2853

Merged
merged 15 commits into from
Sep 18, 2024
Merged

Fix ComQueue Does Not Deallocate Buffers on Overflow #2853

merged 15 commits into from
Sep 18, 2024

Conversation

DJKessler
Copy link
Collaborator

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

github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 3, 2024
View reviewed changes
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 =

Check notice

Code scanning / CodeQL

Use of basic integral type Note

status uses the basic integral type bool rather than a typedef with size and signedness.
Svc/ComQueue/ComQueue.cpp Fixed Show fixed Hide fixed
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) {

Check notice

Code scanning / CodeQL

Use of basic integral type Note

enqueue uses the basic integral type bool rather than a typedef with size and signedness.
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) {

Check notice

Code scanning / CodeQL

Use of basic integral type Note

queueNum uses the basic integral type int rather than a typedef with size and signedness.
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) {

Check notice

Code scanning / CodeQL

Use of basic integral type Note

size uses the basic integral type unsigned long rather than a typedef with size and signedness.
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;

Check notice

Code scanning / CodeQL

Use of basic integral type Note

rvStatus uses the basic integral type bool rather than a typedef with size and signedness.
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 3, 2024
View reviewed changes
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) {

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 3, 2024
View reviewed changes
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));

Check warning

Code scanning / CodeQL

Unchecked function argument Warning

This use of parameter fwBuffer has not been checked.
Svc/ComQueue/ComQueue.cpp Fixed Show fixed Hide fixed
Svc/ComQueue/ComQueue.cpp
// ----------------------------------------------------------------------

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

Check warning

Code scanning / CodeQL

Unchecked function argument Warning

This use of parameter fwBuffer has not been checked.
LeStarch
LeStarch requested changes Sep 4, 2024
View reviewed changes
Copy link
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are several things that need to be fixed:

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

Svc/ComQueue/ComQueue.cpp Outdated Show resolved Hide resolved
Svc/ComQueue/ComQueue.hpp Outdated Show resolved Hide resolved
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 9, 2024
View reviewed changes
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));

Check warning

Code scanning / CodeQL

Unchecked function argument Warning

This use of parameter data has not been checked.
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 11, 2024
View reviewed changes
Svc/ComQueue/ComQueue.cpp
// Hook implementations for typed async input ports
// ----------------------------------------------------------------------

void ComQueue::buffQueueIn_overflowHook(FwIndexType portNum, Fw::Buffer& fwBuffer) {

Check notice

Code scanning / CodeQL

Use of basic integral type Note

portNum uses the basic integral type int rather than a typedef with size and signedness.
@LeStarch
Copy link
Collaborator

Skipping known ci failure. Nice work!

@LeStarch LeStarch merged commit 6a921a1 into nasa:devel Sep 18, 2024
47 of 48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LeStarch LeStarch LeStarch approved these changes

Assignees

@DJKessler DJKessler

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DJKessler @LeStarch