Skip to content

Commit a8fe137

Browse files
LeStarchLeStarch
authored
Fix FPrimeRouter potential usage of invalid buffer (#4493)
* Fix #4491 * Formatting and enum size
1 parent 3a293cd commit a8fe137

File tree

5 files changed

+75
-15
lines changed

5 files changed

+75
-15
lines changed

‎Svc/FprimeRouter/FprimeRouter.cpp‎

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -51,12 +51,18 @@ void FprimeRouter ::dataIn_handler(FwIndexType portNum, Fw::Buffer& packetBuffer
5151
// Copy buffer into a new allocated buffer. This lets us return the original buffer with dataReturnOut,
5252
// and FprimeRouter can handle the deallocation of the file buffer when it returns on fileBufferReturnIn
5353
Fw::Buffer packetBufferCopy = this->bufferAllocate_out(0, packetBuffer.getSize());
54-
auto copySerializer = packetBufferCopy.getSerializer();
55-
status = copySerializer.serializeFrom(packetBuffer.getData(), packetBuffer.getSize(),
56-
Fw::Serialization::OMIT_LENGTH);
57-
FW_ASSERT(status == Fw::FW_SERIALIZE_OK, status);
58-
// Send the copied buffer out. It will come back on fileBufferReturnIn once the receiver is done with it
59-
this->fileOut_out(0, packetBufferCopy);
54+
// Confirm we got a valid buffer before using it
55+
if (packetBufferCopy.isValid()) {
56+
auto copySerializer = packetBufferCopy.getSerializer();
57+
status = copySerializer.serializeFrom(packetBuffer.getData(), packetBuffer.getSize(),
58+
Fw::Serialization::OMIT_LENGTH);
59+
FW_ASSERT(status == Fw::FW_SERIALIZE_OK, status);
60+
// Send the copied buffer out. It will come back on fileBufferReturnIn once the receiver is done
61+
// with it
62+
this->fileOut_out(0, packetBufferCopy);
63+
} else {
64+
this->log_WARNING_HI_AllocationError(FprimeRouter_AllocationReason::FILE_UPLINK);
65+
}
6066
}
6167
break;
6268
}
@@ -67,13 +73,20 @@ void FprimeRouter ::dataIn_handler(FwIndexType portNum, Fw::Buffer& packetBuffer
6773
// Copy buffer into a new allocated buffer. This lets us return the original buffer with dataReturnOut,
6874
// and FprimeRouter can handle the deallocation of the unknown buffer when it returns on bufferReturnIn
6975
Fw::Buffer packetBufferCopy = this->bufferAllocate_out(0, packetBuffer.getSize());
70-
auto copySerializer = packetBufferCopy.getSerializer();
71-
status = copySerializer.serializeFrom(packetBuffer.getData(), packetBuffer.getSize(),
72-
Fw::Serialization::OMIT_LENGTH);
73-
FW_ASSERT(status == Fw::FW_SERIALIZE_OK, status);
74-
// Send the copied buffer out. It will come back on fileBufferReturnIn once the receiver is done with it
75-
this->unknownDataOut_out(0, packetBufferCopy, context);
76+
// Confirm we got a valid buffer before using it
77+
if (packetBufferCopy.isValid()) {
78+
auto copySerializer = packetBufferCopy.getSerializer();
79+
status = copySerializer.serializeFrom(packetBuffer.getData(), packetBuffer.getSize(),
80+
Fw::Serialization::OMIT_LENGTH);
81+
FW_ASSERT(status == Fw::FW_SERIALIZE_OK, status);
82+
// Send the copied buffer out. It will come back on fileBufferReturnIn once the receiver is done
83+
// with it
84+
this->unknownDataOut_out(0, packetBufferCopy, context);
85+
} else {
86+
this->log_WARNING_HI_AllocationError(FprimeRouter_AllocationReason::USER_BUFFER);
87+
}
7688
}
89+
break;
7790
}
7891
}
7992

‎Svc/FprimeRouter/FprimeRouter.fpp‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,11 @@ module Svc {
77
# ----------------------------------------------------------------------
88
import Router
99

10+
enum AllocationReason : U8{
11+
FILE_UPLINK, @< Buffer allocation for file uplink
12+
USER_BUFFER @< Buffer allocation for user handled buffer
13+
}
14+
1015
@ Port for forwarding non-recognized packet types
1116
@ Ownership of the buffer is retained by the FprimeRouter, meaning receiving
1217
@ components should either process data synchronously, or copy the data if needed
@@ -31,6 +36,10 @@ module Svc {
3136
) \
3237
severity warning high \
3338
format "Deserializing packet type failed with status {}"
39+
40+
@ An allocation error occurred
41+
event AllocationError(reason: AllocationReason) severity warning high \
42+
format "Buffer allocation for {} failed"
3443

3544

3645
###############################################################################

‎Svc/FprimeRouter/test/ut/FprimeRouterTestMain.cpp‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,16 @@ TEST(FprimeRouter, TestRouteUnknownPacketUnconnected) {
2828
Svc::FprimeRouterTester tester(true);
2929
tester.testRouteUnknownPacketUnconnected();
3030
}
31+
TEST(FprimeRouter, TestAllocationFailureFile) {
32+
COMMENT("Test failure to allocate for files");
33+
Svc::FprimeRouterTester tester;
34+
tester.testAllocationFailureFile();
35+
}
36+
TEST(FprimeRouter, TestAllocationFailureUnknown) {
37+
COMMENT("Test failure to allocate for unknown packets");
38+
Svc::FprimeRouterTester tester;
39+
tester.testAllocationFailureUnknown();
40+
}
3141
TEST(FprimeRouter, TestBufferReturn) {
3242
COMMENT("Deallocate a returning buffer");
3343
Svc::FprimeRouterTester tester;

‎Svc/FprimeRouter/test/ut/FprimeRouterTester.cpp‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,22 @@ void FprimeRouterTester ::testRouteUnknownPacketUnconnected() {
6464
ASSERT_from_bufferAllocate_SIZE(0); // no buffer allocation when port is unconnected
6565
}
6666

67+
void FprimeRouterTester ::testAllocationFailureFile() {
68+
this->m_forceAllocationError = true;
69+
this->mockReceivePacketType(Fw::ComPacketType::FW_PACKET_FILE);
70+
ASSERT_EVENTS_AllocationError_SIZE(1); // allocation error should be logged
71+
ASSERT_EVENTS_AllocationError(0, FprimeRouter_AllocationReason::FILE_UPLINK);
72+
ASSERT_from_dataReturnOut_SIZE(1); // data ownership should always be returned
73+
}
74+
75+
void FprimeRouterTester ::testAllocationFailureUnknown() {
76+
this->m_forceAllocationError = true;
77+
this->mockReceivePacketType(Fw::ComPacketType::FW_PACKET_UNKNOWN);
78+
ASSERT_EVENTS_AllocationError_SIZE(1); // allocation error should be logged
79+
ASSERT_EVENTS_AllocationError(0, FprimeRouter_AllocationReason::USER_BUFFER);
80+
ASSERT_from_dataReturnOut_SIZE(1); // data ownership should always be returned
81+
}
82+
6783
void FprimeRouterTester ::testBufferReturn() {
6884
U8 data[1];
6985
Fw::Buffer buffer(data, sizeof(data));
@@ -116,9 +132,14 @@ void FprimeRouterTester::connectPortsExceptUnknownData() {
116132
// ----------------------------------------------------------------------
117133
Fw::Buffer FprimeRouterTester::from_bufferAllocate_handler(FwIndexType portNum, FwSizeType size) {
118134
this->pushFromPortEntry_bufferAllocate(size);
119-
this->m_buffer.setData(this->m_buffer_slot);
120-
this->m_buffer.setSize(size);
121-
::memset(this->m_buffer.getData(), 0, size);
135+
if (this->m_forceAllocationError) {
136+
this->m_buffer.setData(nullptr);
137+
this->m_buffer.setSize(0);
138+
} else {
139+
this->m_buffer.setData(this->m_buffer_slot);
140+
this->m_buffer.setSize(size);
141+
::memset(this->m_buffer.getData(), 0, size);
142+
}
122143
return this->m_buffer;
123144
}
124145

‎Svc/FprimeRouter/test/ut/FprimeRouterTester.hpp‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,12 @@ class FprimeRouterTester : public FprimeRouterGTestBase {
5656
//! Route a packet of unknown type
5757
void testRouteUnknownPacketUnconnected();
5858

59+
//! Route a packet of unknown type
60+
void testAllocationFailureFile();
61+
62+
//! Deallocate a returning buffer
63+
void testAllocationFailureUnknown();
64+
5965
//! Deallocate a returning buffer
6066
void testBufferReturn();
6167

@@ -95,6 +101,7 @@ class FprimeRouterTester : public FprimeRouterGTestBase {
95101

96102
Fw::Buffer m_buffer; // buffer to be returned by mocked bufferAllocate call
97103
U8 m_buffer_slot[64];
104+
bool m_forceAllocationError = false; // Flag to force allocation error
98105
};
99106

100107
} // namespace Svc

0 commit comments

Comments
 (0)