PR 5086 - Improved frame detector arithmetic against integer underflow and overflow

bitWarrior · bitWarrior · commit 3a7a19ef0afd · 2026-05-02T11:17:42.000-07:00
diff --git a/Svc/FrameAccumulator/CMakeLists.txt b/Svc/FrameAccumulator/CMakeLists.txt
@@ -39,6 +39,7 @@ register_fprime_ut(
   "Svc_FrameAccumulator_FprimeFrameDetector_test"
   SOURCES 
     "${CMAKE_CURRENT_LIST_DIR}/test/ut/detectors/FprimeFrameDetectorTestMain.cpp"
+    "${CMAKE_CURRENT_LIST_DIR}/test/ut/detectors/FprimeFrameDetectorTest.cpp"
   HEADERS
     "${CMAKE_CURRENT_LIST_DIR}/FrameDetector/FprimeFrameDetector.hpp"
   DEPENDS
@@ -50,6 +51,7 @@ register_fprime_ut(
   "Svc_FrameAccumulator_CcsdsTcFrameDetector_test"
   SOURCES 
     "${CMAKE_CURRENT_LIST_DIR}/test/ut/detectors/CcsdsTcFrameDetectorTestMain.cpp"
+    "${CMAKE_CURRENT_LIST_DIR}/test/ut/detectors/CcsdsTcFrameDetectorUnderflowTest.cpp"
   HEADERS
     "${CMAKE_CURRENT_LIST_DIR}/FrameDetector/CcsdsTcFrameDetector.hpp"
   DEPENDS
diff --git a/Svc/FrameAccumulator/FrameDetector/CcsdsTcFrameDetector.cpp b/Svc/FrameAccumulator/FrameDetector/CcsdsTcFrameDetector.cpp
@@ -44,17 +44,22 @@ FrameDetector::Status CcsdsTcFrameDetector::detect(const Types::CircularBuffer&
     // TC protocol defines the Frame Length as number of bytes minus 1, so we add 1 back to get length in bytes
     const FwSizeType expected_frame_length =
         static_cast<FwSizeType>((header.get_vcIdAndLength() & Ccsds::TCSubfields::FrameLengthMask) + 1);
-    const U16 data_to_crc_length = static_cast<U16>(expected_frame_length - Ccsds::TCTrailer::SERIALIZED_SIZE);
 
     // If the full frame is not yet available, report that more data is needed
     if (data.get_allocated_size() < expected_frame_length) {
         size_out = expected_frame_length;
         return Status::MORE_DATA_NEEDED;
     }
-    // If the deserialized length is too small to even hold the header and trailer, we don't have a valid frame
+    // Validate minimum frame size BEFORE computing data_to_crc_length.
+    // If expected_frame_length < TRAILER_SIZE the subtraction below would underflow to a
+    // near-maximum value, causing the CRC loop to read far beyond the valid frame data.
     if (expected_frame_length < Ccsds::TCHeader::SERIALIZED_SIZE + Ccsds::TCTrailer::SERIALIZED_SIZE) {
         return Status::NO_FRAME_DETECTED;
     }
+    // Safe: the guard above guarantees expected_frame_length >= TRAILER_SIZE, so this
+    // subtraction cannot underflow. Type is FwSizeType (not U16) to avoid silent truncation
+    // if expected_frame_length ever exceeds 65535.
+    const FwSizeType data_to_crc_length = expected_frame_length - Ccsds::TCTrailer::SERIALIZED_SIZE;
 
     // ---------------- Frame Trailer ----------------
     // Compute CRC on the received data
diff --git a/Svc/FrameAccumulator/FrameDetector/FprimeFrameDetector.cpp b/Svc/FrameAccumulator/FrameDetector/FprimeFrameDetector.cpp
@@ -49,19 +49,29 @@ FrameDetector::Status FprimeFrameDetector::detect(const Types::CircularBuffer& d
     if (header.get_startWord() != default_value.get_startWord()) {
         return Status::NO_FRAME_DETECTED;
     }
-    // Validate size before proceeding
-    const FwSizeType max_payload_size = std::numeric_limits<FwSizeType>::max() -
-                                        FprimeProtocol::FrameHeader::SERIALIZED_SIZE -
-                                        FprimeProtocol::FrameTrailer::SERIALIZED_SIZE;
-    // If the header length is larger than size can store, then frame is invalid
-    if (max_payload_size < header.get_lengthField()) {
-        // Size overflow - frame is invalid
+    // Validate size before proceeding.
+    // Use a static_assert to guarantee the two fixed overhead constants don't themselves overflow
+    // when added together. This is a compile-time check so it carries zero runtime cost, but it
+    // protects the runtime guard below if SERIALIZED_SIZE values are ever changed.
+    static_assert(FprimeProtocol::FrameHeader::SERIALIZED_SIZE <=
+                      std::numeric_limits<FwSizeType>::max() - FprimeProtocol::FrameTrailer::SERIALIZED_SIZE,
+                  "FrameHeader::SERIALIZED_SIZE + FrameTrailer::SERIALIZED_SIZE overflows FwSizeType");
+    constexpr FwSizeType header_trailer_overhead =
+        FprimeProtocol::FrameHeader::SERIALIZED_SIZE + FprimeProtocol::FrameTrailer::SERIALIZED_SIZE;
+
+    // Guard: reject frames whose declared length would overflow FwSizeType when added to the
+    // fixed overhead. Using subtraction on unsigned types (as in the prior implementation) is
+    // fragile — if the constants change sign or width the subtraction itself can wrap silently.
+    // An explicit addition-based check is clearer and easier to audit.
+    if (header.get_lengthField() > std::numeric_limits<FwSizeType>::max() - header_trailer_overhead) {
+        // lengthField + overhead would overflow — frame is invalid
         return Status::NO_FRAME_DETECTED;
     }
 
-    // We expect the frame size to be size of header + body (of size specified in header) + trailer
-    const FwSizeType expected_frame_size = FprimeProtocol::FrameHeader::SERIALIZED_SIZE + header.get_lengthField() +
-                                           FprimeProtocol::FrameTrailer::SERIALIZED_SIZE;
+    // We expect the frame size to be size of header + body (of size specified in header) + trailer.
+    // Overflow is impossible here: the guard above ensures
+    //   lengthField <= MAX - header_trailer_overhead
+    const FwSizeType expected_frame_size = header.get_lengthField() + header_trailer_overhead;
     // If the frame will never fit, then report NO_FRAME_DETECTED to drop the erroneous frame
     if (data.get_capacity() < expected_frame_size) {
         return Status::NO_FRAME_DETECTED;
@@ -90,7 +100,16 @@ FrameDetector::Status FprimeFrameDetector::detect(const Types::CircularBuffer& d
 
     Utils::Hash hash;
     Utils::HashBuffer hashBuffer;
-    // Compute CRC over the transmitted data (header + body)
+    // Compute CRC over the transmitted data (header + body).
+    // Safety invariant: the guard above ensures
+    //   lengthField <= MAX - header_trailer_overhead
+    //                <= MAX - HEADER_SIZE - TRAILER_SIZE
+    //                <  MAX - HEADER_SIZE
+    // so this addition cannot overflow. The assert makes that contract explicit at the
+    // point of use so it remains correct if this code is ever moved or refactored.
+    FW_ASSERT(header.get_lengthField() <=
+                  std::numeric_limits<FwSizeType>::max() - FprimeProtocol::FrameHeader::SERIALIZED_SIZE,
+              static_cast<FwAssertArgType>(header.get_lengthField()));
     FwSizeType hash_field_size = header.get_lengthField() + FprimeProtocol::FrameHeader::SERIALIZED_SIZE;
     hash.init();
     for (FwSizeType i = 0; i < hash_field_size; i++) {
diff --git a/Svc/FrameAccumulator/test/ut/detectors/CcsdsTcFrameDetectorUnderflowTest.cpp b/Svc/FrameAccumulator/test/ut/detectors/CcsdsTcFrameDetectorUnderflowTest.cpp
@@ -0,0 +1,234 @@
+// ======================================================================
+// \title  CcsdsTcFrameDetectorUnderflowTest.cpp
+// \brief  Unit tests for the data_to_crc_length underflow fix in
+//         CcsdsTcFrameDetector
+//
+// These tests exercise CcsdsTcFrameDetector::detect() directly and focus
+// on the ordering fix: data_to_crc_length must only be computed AFTER the
+// minimum-size validity check, never before.
+//
+// The defect (before the fix):
+//   data_to_crc_length was computed as:
+//       U16(expected_frame_length - TRAILER_SIZE)
+//   before the check that ensures expected_frame_length >= TRAILER_SIZE.
+//   A crafted frame with length field 0 (giving expected_frame_length = 1)
+//   produced data_to_crc_length = 0xFFFF (unsigned underflow), which
+//   would cause the CRC loop to read 65535 bytes beyond the frame data.
+//
+// The fix:
+//   - Both the MORE_DATA_NEEDED check and the minimum-size check now
+//     appear before data_to_crc_length is computed.
+//   - data_to_crc_length is widened from U16 to FwSizeType to prevent
+//     silent truncation for frames larger than 65535 bytes.
+//
+// Note on flagsAndScId: CcsdsTcFrameDetector rejects frames whose
+// flagsAndScId token does not match its internal m_expectedFlagsAndScIdToken.
+// The default-constructed detector's expected token is not publicly
+// queryable, so the underflow tests below use flagsAndScId = 0, which is
+// unlikely to match and will therefore be rejected at the flags check
+// rather than the size check. This is intentional: the tests verify the
+// observable safety contract (no crash, NO_FRAME_DETECTED returned) for
+// crafted extreme-length inputs. A future test with access to the
+// configured token value could additionally verify that the size guard
+// itself fires as the rejection point.
+// ======================================================================
+
+#include <cassert>
+#include <cstring>
+#include <limits>
+#include "Svc/Ccsds/Types/TCHeaderSerializableAc.hpp"
+#include "Svc/Ccsds/Types/TCTrailerSerializableAc.hpp"
+#include "Svc/FrameAccumulator/FrameDetector/CcsdsTcFrameDetector.hpp"
+#include "Utils/Types/CircularBuffer.hpp"
+#include "gtest/gtest.h"
+
+namespace Svc {
+namespace FrameDetectors {
+
+// ======================================================================
+// Module-level constants (namespace scope avoids C++14 ODR-use linker
+// errors that arise from static constexpr class members passed by ref
+// to EXPECT_EQ)
+// ======================================================================
+namespace {
+
+constexpr FwSizeType kTcHeaderSize = Ccsds::TCHeader::SERIALIZED_SIZE;
+constexpr FwSizeType kTcTrailerSize = Ccsds::TCTrailer::SERIALIZED_SIZE;
+constexpr FwSizeType kTcOverhead = kTcHeaderSize + kTcTrailerSize;
+
+}  // namespace
+
+// ======================================================================
+// Test fixture
+// ======================================================================
+
+class CcsdsTcFrameDetectorUnderflowTest : public ::testing::Test {
+  protected:
+    // Use the default constructor — CcsdsTcFrameDetector takes no arguments
+    CcsdsTcFrameDetector detector;
+
+    // ----------------------------------------------------------------
+    // Helper: serialize a TCHeader into `out_bytes` with the given
+    // flagsAndScId and vcIdAndLength field values.
+    // Returns true on success.
+    // ----------------------------------------------------------------
+    static bool buildHeader(U8* out_bytes, U16 flags_and_sc_id, U16 vc_id_and_length) {
+        Ccsds::TCHeader header;
+        header.set_flagsAndScId(flags_and_sc_id);
+        header.set_vcIdAndLength(vc_id_and_length);
+        Fw::ExternalSerializeBuffer ser(out_bytes, kTcHeaderSize);
+        return header.serializeTo(ser) == Fw::FW_SERIALIZE_OK;
+    }
+
+    // ----------------------------------------------------------------
+    // Helper: load bytes into a CircularBuffer.
+    // Uses assert() (always available) rather than FW_ASSERT which
+    // requires a specific F Prime include chain.
+    // ----------------------------------------------------------------
+    static void loadRing(Types::CircularBuffer& ring, const U8* src, FwSizeType count) {
+        Fw::SerializeStatus s = ring.serialize(src, count);
+        assert(s == Fw::FW_SERIALIZE_OK);
+        static_cast<void>(s);  // suppress unused-variable warning in release builds
+    }
+};
+
+// ======================================================================
+// Fix tests — data_to_crc_length underflow / ordering safety
+//
+// These tests submit frames with extreme declared lengths and verify that
+// detect() returns NO_FRAME_DETECTED without crashing or exhibiting
+// undefined behaviour. The rejection may occur at the flagsAndScId check
+// (if flagsAndScId = 0 doesn't match the detector's expected token) or at
+// the minimum-size check — either way the safety contract holds.
+// ======================================================================
+
+// ----------------------------------------------------------------------
+// Length field == 0 encodes expected_frame_length == 1, which is less
+// than TRAILER_SIZE. Before the fix, data_to_crc_length = U16(1 - 2) =
+// 0xFFFF was computed before any size validation. The fix ensures the
+// size guard fires first, making the underflow impossible to reach.
+// ----------------------------------------------------------------------
+TEST_F(CcsdsTcFrameDetectorUnderflowTest, LengthFieldZeroRejectedSafely) {
+    // vcIdAndLength with all length bits == 0 → expected_frame_length = 1
+    U8 storage[kTcOverhead] = {};
+    Types::CircularBuffer ring(storage, sizeof(storage));
+
+    U8 header_bytes[kTcHeaderSize] = {};
+    ASSERT_TRUE(buildHeader(header_bytes, 0, 0));
+    loadRing(ring, header_bytes, kTcHeaderSize);
+
+    U8 padding[kTcTrailerSize] = {};
+    loadRing(ring, padding, kTcTrailerSize);
+
+    FwSizeType size_out = 0;
+    FrameDetector::Status result = detector.detect(ring, size_out);
+
+    // Must return NO_FRAME_DETECTED with no crash, regardless of which
+    // guard (flags or size) fires first.
+    EXPECT_EQ(result, FrameDetector::Status::NO_FRAME_DETECTED);
+}
+
+// ----------------------------------------------------------------------
+// expected_frame_length == kTcOverhead - 1 is still too small. Verifies
+// the boundary just below the minimum valid size is rejected cleanly.
+// ----------------------------------------------------------------------
+TEST_F(CcsdsTcFrameDetectorUnderflowTest, LengthFieldOneBelowOverheadRejectedSafely) {
+    // CCSDS TC length field encodes (frame_bytes - 1), so to get
+    // expected_frame_length = kTcOverhead - 1 we write kTcOverhead - 2.
+    const U16 vc_id_and_length = static_cast<U16>(kTcOverhead - 2);
+
+    U8 storage[kTcOverhead] = {};
+    Types::CircularBuffer ring(storage, sizeof(storage));
+
+    U8 header_bytes[kTcHeaderSize] = {};
+    ASSERT_TRUE(buildHeader(header_bytes, 0, vc_id_and_length));
+    loadRing(ring, header_bytes, kTcHeaderSize);
+
+    U8 padding[kTcTrailerSize] = {};
+    loadRing(ring, padding, kTcTrailerSize);
+
+    FwSizeType size_out = 0;
+    FrameDetector::Status result = detector.detect(ring, size_out);
+
+    EXPECT_EQ(result, FrameDetector::Status::NO_FRAME_DETECTED);
+}
+
+// ----------------------------------------------------------------------
+// expected_frame_length == kTcOverhead is the minimum valid size.
+// This test verifies the exact boundary: a frame of exactly overhead
+// size is not rejected by the minimum-size guard. The result is still
+// NO_FRAME_DETECTED (flags mismatch or CRC mismatch), but the minimum-
+// size guard correctly does not fire.
+// ----------------------------------------------------------------------
+TEST_F(CcsdsTcFrameDetectorUnderflowTest, LengthFieldAtMinimumValidSizeNotRejectedByGuard) {
+    // CCSDS TC length field encodes (frame_bytes - 1)
+    const U16 vc_id_and_length = static_cast<U16>(kTcOverhead - 1);
+
+    U8 storage[kTcOverhead] = {};
+    Types::CircularBuffer ring(storage, sizeof(storage));
+
+    U8 header_bytes[kTcHeaderSize] = {};
+    ASSERT_TRUE(buildHeader(header_bytes, 0, vc_id_and_length));
+    loadRing(ring, header_bytes, kTcHeaderSize);
+
+    U8 trailer_bytes[kTcTrailerSize] = {};
+    loadRing(ring, trailer_bytes, kTcTrailerSize);
+
+    FwSizeType size_out = 0;
+    FrameDetector::Status result = detector.detect(ring, size_out);
+
+    // Rejected due to flags mismatch or CRC mismatch — not the size guard.
+    EXPECT_EQ(result, FrameDetector::Status::NO_FRAME_DETECTED);
+}
+
+// ======================================================================
+// Regression tests — pre-existing behaviour must be preserved
+// ======================================================================
+
+// ----------------------------------------------------------------------
+// Insufficient data (below header + trailer minimum) returns
+// MORE_DATA_NEEDED before any header parsing occurs. This path is
+// independent of the flagsAndScId token value.
+// ----------------------------------------------------------------------
+TEST_F(CcsdsTcFrameDetectorUnderflowTest, InsufficientDataReturnsMoreDataNeeded) {
+    const FwSizeType too_small = kTcOverhead - 1;
+
+    U8 storage[kTcOverhead] = {};
+    Types::CircularBuffer ring(storage, sizeof(storage));
+
+    U8 data[too_small] = {};
+    loadRing(ring, data, too_small);
+
+    FwSizeType size_out = 0;
+    FrameDetector::Status result = detector.detect(ring, size_out);
+
+    EXPECT_EQ(result, FrameDetector::Status::MORE_DATA_NEEDED);
+    EXPECT_EQ(size_out, kTcOverhead);
+}
+
+// ----------------------------------------------------------------------
+// A mismatched flagsAndScId token must return NO_FRAME_DETECTED before
+// any length or CRC logic is reached. Uses 0xFFFF as a value that is
+// guaranteed not to match any valid default token.
+// ----------------------------------------------------------------------
+TEST_F(CcsdsTcFrameDetectorUnderflowTest, MismatchedFlagsAndScIdRejected) {
+    const U16 vc_id_and_length = static_cast<U16>(kTcOverhead - 1);
+
+    U8 storage[kTcOverhead] = {};
+    Types::CircularBuffer ring(storage, sizeof(storage));
+
+    U8 header_bytes[kTcHeaderSize] = {};
+    ASSERT_TRUE(buildHeader(header_bytes, 0xFFFF, vc_id_and_length));
+    loadRing(ring, header_bytes, kTcHeaderSize);
+
+    U8 padding[kTcTrailerSize] = {};
+    loadRing(ring, padding, kTcTrailerSize);
+
+    FwSizeType size_out = 0;
+    FrameDetector::Status result = detector.detect(ring, size_out);
+
+    EXPECT_EQ(result, FrameDetector::Status::NO_FRAME_DETECTED);
+}
+
+}  // namespace FrameDetectors
+}  // namespace Svc
diff --git a/Svc/FrameAccumulator/test/ut/detectors/FprimeFrameDetectorTest.cpp b/Svc/FrameAccumulator/test/ut/detectors/FprimeFrameDetectorTest.cpp
