fix(Ccsds/AosDeframer): bound untrusted packet size before allocation by datqm · Pull Request #5145 · nasa/fprime

datqm · 2026-05-12T08:50:01Z


*Related Issue(s)*	(none - addresses FIXME comment in source)
*Has Unit Tests (y/n)*	y
*Documentation Included (y/n)*	n
*Generative AI was used in this contribution (y/n)*	y (code generation, testing)

Change Description

AosDeframer::appendToSpanningPacket reads packetSize from the wire
(via sizePacket() applied to the spanning-packet header) and passes
it directly to allocate_out(). The current source explicitly flags
this with a FIXME at line 316 of Svc/Ccsds/AosDeframer/AosDeframer.cpp:

// FIXME: packetSize is untrusted (read straight from the wire) and
// could have unintended side effects, especially with EPP. We should
// skip if packetSize exceeds a mission-defined limit instead of
// letting the allocator handle any requests, which has a side
// effect of abandoning spanning packets on failure

This PR addresses that FIXME by introducing a mission-tunable upper
bound on a single deframed packet and rejecting wire-read sizes that
exceed it before any allocation is attempted.

The change set:

Adds ComCfg::AosMaxPacketSize (default 65536) as a mission-tunable
constant in default/config/ComCfg.fpp. The default fits a maximally
sized CCSDS Space Packet (header + 65535 byte data field) and is
large enough for typical EPP traffic; missions that legitimately
need larger packets can override the constant in their own config.
Adds a new warning event
OversizedPacket(vcId, pvn, packetSize, maxSize) in
AosDeframerEvents.fppi so operators can see the rejection in
telemetry.
In appendToSpanningPacket, when sizePacket() returns a
packetSize that exceeds ComCfg::AosMaxPacketSize, the patch
logs OversizedPacket, abandons the spanning state, and seeks
past the rejected packet bytes - all before any allocate_out
call. The existing zero-size early return path is unchanged so the
"need more header bytes" case still works.

Rationale

Without the bound, a malformed packet that declares a very large size
drives the buffer allocator to attempt arbitrarily large requests. The
result is a resource-exhaustion vector: a single AOS frame on any
virtual channel can request a buffer larger than the buffer manager
can satisfy, and repeated frames can degrade allocation service across
all virtual channels even after the affected spanning packet has been
abandoned.

The fix mirrors the pattern already established in PR #5085
(Fix pkt_length integer overflow and replaced ASSERT w/ event in
Svc/Ccsds/SpacePacketDeframer): reject untrusted size values with a
warning event rather than forwarding them to the allocator.

Testing/Review Recommendations

Verification performed locally:

fprime-util format --check on Svc/Ccsds/AosDeframer: clean.
Svc/Ccsds/AosDeframer unit test suite: 33/33 passed. The
existing testSpanningPacketAllocFailureEvent case sends a
65539-byte EPP packet and now asserts on the new OversizedPacket
event because the bound check fires before the allocator is
consulted; the m_failNextAlloc-driven test downstream still
exercises the original allocation-failure path on a non-oversized
packet.
Full framework unit test suite via fprime-util check: 107/107
passed.

appendToSpanningPacket reads packetSize from the wire (via sizePacket applied to the spanning-packet header) and passes it directly to allocate_out. A malformed or hostile packet that declares a very large size therefore drives the buffer allocator to attempt arbitrarily large requests. This was previously called out in a FIXME comment in the source but no bound check was in place. The result was a resource-exhaustion vector: a single AOS frame on any virtual channel could request a buffer larger than the buffer manager can satisfy, and repeated frames could degrade allocation service across all virtual channels even after the affected spanning packet was abandoned. This commit: * Adds ComCfg::AosMaxPacketSize (default 65536) as a mission-tunable upper bound on a single deframed packet. The default fits a maximally-sized CCSDS Space Packet (header + 65535 byte data field) and is large enough for typical EPP traffic; missions that legitimately need larger packets can override it. * Adds a new warning event OversizedPacket(vcId, pvn, packetSize, maxSize) so operators can see the rejection in telemetry. * In appendToSpanningPacket, when sizePacket returns a packetSize that exceeds ComCfg::AosMaxPacketSize, logs OversizedPacket, abandons the spanning state, and seeks past the rejected packet bytes before any allocate_out call. The check is placed after the existing zero-size early return so the "need more header bytes" path is unchanged. Testing: fprime-util format --check on Svc/Ccsds/AosDeframer: clean AosDeframer unit test suite: 33/33 passed (existing testSpanningPacketAllocFailureEvent now asserts on the new OversizedPacket event for the same 65539-byte input, since the bound check fires up front; the alloc-failure path remains covered by the m_failNextAlloc-driven test downstream) Full framework unit test suite (fprime-util check): 107/107 passed Signed-off-by: datqm <datqm@vingroup.net>

Willmac16 · 2026-05-12T22:08:29Z

I would request that the limit be (additionally) override-able on a per instance basis via a configure() call like the rest of the parameters should be (I might've forgotten one on my original PR).

Willmac16

Good change overall, a couple of comments on code dupe and configurability.

Willmac16 · 2026-05-12T22:11:56Z

+            const FwSizeType remainingBody = packetSize - vc.spanningPacket.bytesReceived;
+            this->abandonSpanningPacket(vc);
+
+            // Seek past the rejected packet (header bytes already consumed + remaining body)
+            const FwSizeType remainingLength = seekForward + remainingBody;
+            if (remainingLength > size) {
+                return 0;
+            } else {
+                return remainingLength;
+            }


This code path should be shared w/ the allocate failure imo should be the same exact logic, just a diff check fail and event

Willmac16 · 2026-05-12T22:12:42Z

+        // field would force allocate_out() to request arbitrarily large
+        // buffers, which exhausts the buffer manager and degrades downlink
+        // throughput across all virtual channels.
+        if (packetSize > ComCfg::AosMaxPacketSize) {


Make this a member var that defaults to the global ComCfg const but can be overridden via a configure call

I like this suggestion, and for the sake of not having two ways of specifying the same thing, I would just make it a configure() option and remove the ComCfg config value.

Address two review threads on the prior packet-size bound commit: 1. Configurability (Willmac16, thomas-bc): the global ComCfg::AosMaxPacketSize constant duplicates what a per-instance configure() option should provide. Remove the global and pass the bound through configure() instead, defaulting to a new AosDeframer.DefaultMaxPacketSize FPP constant (still 65536). Missions that need a different cap now set it explicitly when configuring the component, with one canonical place to override. 2. Code duplication (Willmac16): the oversize-reject path and the allocator-failure-reject path had identical bodies — save the remaining-body offset, abandon the spanning packet, compute the seek-forward, and clamp it to the available data block. Factor this into a private helper abandonAndSeekPast() so the two call sites differ only in the warning event they emit. New configure() signature: void configure(U32 fixedFrameSize, bool frameErrorControlField, U16 spacecraftId = ComCfg::SpacecraftId, U8 vcId = 0, U8 pvnMask = SPP_MASK | EPP_MASK, FwSizeType maxPacketSize = AosDeframer_DefaultMaxPacketSize); The OversizedPacket event format string and docstring no longer name the (now-removed) ComCfg::AosMaxPacketSize constant. Tests: - Existing testSpanningPacketAllocFailureEvent and testAllocFailureNextPacketExtracted continue to exercise the now- shared reject path through the alloc-failure branch. - New testConfiguredMaxPacketSizeOverride configures with a tight 50-byte cap and verifies a 100-byte SPP packet is rejected via OversizedPacket with the configured cap reported in the event, proving the configure() override is wired through end-to-end. - 34/34 AosDeframer UTs pass (was 33; +1 new test). Signed-off-by: datqm <datqm@vingroup.net>

datqm · 2026-05-13T00:11:43Z

Thanks for the thoughtful review — I fully agree with both points. Addressed in 81bbd42:

Configurability: Removed the global ComCfg::AosMaxPacketSize and added a per-instance override via configure(..., maxPacketSize), defaulting to a new AosDeframer.DefaultMaxPacketSize FPP constant (still 65536). One canonical place to set it.

Code dedup: Factored the common "log → abandon → seek-past" sequence into a private abandonAndSeekPast() helper. The two reject call sites now differ only in the warning event they emit.

Added testConfiguredMaxPacketSizeOverride to verify the override path end-to-end (configures a tight 50-byte cap, sends a 100-byte SPP packet, asserts OversizedPacket reports the configured value not the default).

Willmac16

A couple more notes on the min & max bounds of default SPP packet size

Willmac16 · 2026-05-13T00:55:27Z

+    // maxPacketSize must be large enough to accommodate at least an empty packet header.
+    // We cannot enforce a fixed minimum here (SPP is 6 bytes, EPP is 1) so just require non-zero.
+    FW_ASSERT(maxPacketSize > 0, static_cast<FwAssertArgType>(maxPacketSize));


W/ the PVN mask we do actually have enough info to know if 1 (EPP) vs 7 (SPP) should be the minimum packet size.

Willmac16 · 2026-05-13T01:00:10Z

+        @ Default upper bound on the size of a single packet (Space Packet or
+        @ Encapsulation Packet) accepted by the deframer. The packet-length
+        @ field of a CCSDS Space Packet is U16 (max 65535 + 1 byte = 65536),
+        @ and Encapsulation Packets can in principle be larger. Capping at
+        @ 65536 by default keeps a malformed or hostile packet-length read
+        @ from the wire from being passed directly to the buffer allocator.
+        @ Override per-instance via configure().
+        constant DefaultMaxPacketSize = 65536


If your goal is to fit a whole, max-size SPP then 65542 is the right number since the header is not included in that 16 bit length count

I would also rename the const to SPP Max Length (and prob move it to Ccsds/Types.fpp) if that’s what we’re deriving it from; that way looking at the default arg in the configure signature in the hpp its obvious whats driving the number.

Willmac16 · 2026-05-13T01:08:22Z

+        // packetSize is derived from the on-the-wire packet header. Cap it at
+        // the configured upper bound (m_maxPacketSize, set via configure())
+        // before passing to the allocator. Without this check a malformed or
+        // hostile size field would force allocate_out() to request arbitrarily
+        // large buffers, which exhausts the buffer manager and degrades
+        // downlink throughput across all virtual channels.


Consider slimming down this comment block. IMO:

Prevent large or hostile packets from starving all virtual channels of buffer manager resources

or similar is a sufficient description

Willmac16 · 2026-05-13T01:10:25Z

    vc.spanningPacket.context.set_pvn(ComCfg::Pvn::INVALID_UNINITIALIZED);
 }

+FwSizeType AosDeframer::abandonAndSeekPast(AosDeframerVc& vc,


nit: this function does abandon; however, it only computes the seek, rather than performing it.

I’d prefer abandonAndComputeSeek or similar to make it clear we aren’t executing the seek just yet.

Willmac16 · 2026-05-13T02:01:46Z

+    if (pvnMask & PvnBitfield::EPP_MASK) {
+        minPacketSize = Svc::Ccsds::EncapsulationPacketMinLength;
+    } else if (pvnMask & PvnBitfield::SPP_MASK) {
+        minPacketSize = Svc::Ccsds::SpacePacketMinLength;
+    }


both can be enabled; however this will only check for EPP's min if both are enabled

Updated in b3a0ba9.

datqm · 2026-05-13T02:11:02Z

@Willmac16 Updated in 7c9a64c:

pvnMask-aware minimum: configure() derives min maxPacketSize from pvnMask (1 byte for EPP, 7 bytes for SPP-only).
Default cap promoted: DefaultMaxPacketSize → Svc::Ccsds::SpacePacketMaxLength in Svc/Ccsds/Types/Types.fpp, value 65536 → 65542 per CCSDS 133.0-B-2 §4.1.2.2. Added companion SpacePacketMinLength = 7 and EncapsulationPacketMinLength = 1.
Comment block trimmed to your suggested one-liner.
Renamed abandonAndSeekPast → abandonAndComputeSeek

… shared reject helper, pvnMask-aware min check Plus: SPP constants moved to Ccsds/Types.fpp, slim oversize comment, rename helper to abandonAndComputeSeek: 1. Pvn-aware minimum-packet-size validation in configure() (Willmac16): the prior `maxPacketSize > 0` assert was too loose. The pvnMask passed into configure() already encodes which packet protocols this deframer is being told to accept, so we can enforce a meaningful minimum: 1 byte if EPP is enabled (a single Encapsulation Idle Packet per CCSDS 133.1-B-3 §4.1.2.1), or 7 bytes if only SPP is enabled (6-byte primary header + 1-byte minimum data field per CCSDS 133.0-B-2 §4.1.2.2). 2. Promote the default maxPacketSize constant to a CCSDS-wide location and align its value with the standard (Willmac16, with CCSDS 133.0-B-2 §4.1.2.2 cited inline): * Renamed AosDeframer.DefaultMaxPacketSize (65536) to Svc.Ccsds.SpacePacketMaxLength (65542), reflecting that the number is driven by the CCSDS Space Packet specification (6-byte header + 65535+1 = 65536 byte data field = 65542 octets total), not an AosDeframer-internal choice. * Moved the constant from Svc/Ccsds/AosDeframer/AosDeframer.fpp to Svc/Ccsds/Types/Types.fpp alongside the SpacePacketHeader struct, so reading the default arg in the configure() signature makes it obvious where the number comes from. * Added Svc.Ccsds.SpacePacketMinLength (7) and Svc.Ccsds.EncapsulationPacketMinLength (1) as companion constants used by the new minimum check in configure(). 3. Trim the oversize-reject comment block (Willmac16): the prior six-line explanation of why the cap exists has been replaced with a single line ("Prevent large or hostile packets from starving all virtual channels of buffer manager resources"), matching reviewer-suggested wording and the project's general comment density. 4. Rename abandonAndSeekPast -> abandonAndComputeSeek (Willmac16): the helper abandons the spanning packet but only returns the seek distance; the caller chain is responsible for actually advancing the read pointer. The new name makes that contract explicit at the call site. Tests: - testSpanningPacketAllocFailureEvent was originally exercising the oversize-reject path with a 2-byte EPP length field producing a 65539-byte declared packet. With the default cap raised from 65536 to 65542, that input now falls under the cap. Updated the test to use a 4-byte EPP length field (lengthOfLength = Four = 0b11) with a declared size of 65544 bytes (8-byte EPP header + 0x10000-byte body), which exceeds the new 65542-byte cap and triggers OversizedPacket as intended. - 34/34 AosDeframer UTs pass. - 107/107 framework UTs pass. Signed-off-by: datqm <datqm@vingroup.net>

Willmac16

Looks good at this point!

In general I'd recommend against force pushing multiple copies of the same commit--it makes it harder to track what's changing w/o explicitly having gihtub/git cli diff the 2 commits vs the branch history.

If you do additional changes on this I would consider pruning some of the comments down in length, since they seem to accumulate more and more lines each iteration along with commentary that almost seems like an LLM berating itself for getting something wrong the first time.

datqm · 2026-05-13T07:22:43Z

Looks good at this point!

In general I'd recommend against force pushing multiple copies of the same commit--it makes it harder to track what's changing w/o explicitly having gihtub/git cli diff the 2 commits vs the branch history.

If you do additional changes on this I would consider pruning some of the comments down in length, since they seem to accumulate more and more lines each iteration along with commentary that almost seems like an LLM berating itself for getting something wrong the first time.

Thank you for the guidance! I’ve recently started my research in the satellite field and I'm really excited about it. I’m still learning the ropes, so I appreciate the tips. I hope to contribute more effectively to this field in the future !

thomas-bc

Code looks fine, but I would like to push back on the LLM-generated comments. Please find a way to make it less verbose. Good code should not need that much commenting. This is hindering maintainability.

thomas-bc · 2026-05-13T16:14:40Z

    //! \param spacecraftId The spacecraft ID to accept (10 bits, per Section 4.1.2.2)
    //! \param vcId The virtual channel ID to accept (6 bits, per Section 4.1.2.3)
    //! \param pvnMask Bitmask of Packet Version Numbers to extract (SPP=0x01, EPP=0x80)
+    //! \param maxPacketSize Upper bound on a single deframed packet (Space Packet


way too verbose for a parameter description

thomas-bc · 2026-05-13T16:15:22Z

+    //! responsible for emitting the appropriate warning event before calling
+    //! and for performing the returned seek by propagating it up through the
+    //! caller chain. This function abandons the spanning packet but does not
+    //! itself advance any read pointer.


too verbose, first and last sentence are essentially saying the same thing, and there's no point calling out where the helper is being used imo

thomas-bc · 2026-05-13T16:18:20Z

+    // EPP can be as small as a single idle byte (CCSDS 133.1-B-3 §4.1.2.1),
+    // SPP is 7 octets minimum (CCSDS 133.0-B-2 §4.1.2.2: 6-byte header +
+    // 1-byte minimum data field). When both are enabled the SPP minimum is
+    // the binding constraint.


also too verbose - no need to describe in that much depth what's going on in the code when it's trivial like this

datqm · 2026-05-14T04:02:19Z

Code looks fine, but I would like to push back on the LLM-generated comments. Please find a way to make it less verbose. Good code should not need that much commenting. This is hindering maintainability.
I've updated it in 6693ee4

thomas-bc requested a review from Willmac16 May 12, 2026 21:53

Willmac16 suggested changes May 12, 2026

View reviewed changes

Willmac16 suggested changes May 13, 2026

View reviewed changes

datqm force-pushed the fix/aosdeframer-bound-packet-size branch 2 times, most recently from d9596a5 to 7c9a64c Compare May 13, 2026 02:02

Willmac16 suggested changes May 13, 2026

View reviewed changes

datqm force-pushed the fix/aosdeframer-bound-packet-size branch from 7c9a64c to b3a0ba9 Compare May 13, 2026 02:24

Willmac16 approved these changes May 13, 2026

View reviewed changes

thomas-bc requested changes May 13, 2026

View reviewed changes

docs(Ccsds/AosDeframer): trim verbose comments.

6693ee4

Conversation

datqm commented May 12, 2026

Change Description

Rationale

Testing/Review Recommendations

Uh oh!

Willmac16 commented May 12, 2026

Uh oh!

Willmac16 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

datqm commented May 13, 2026

Uh oh!

Willmac16 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

datqm commented May 13, 2026

Uh oh!

Willmac16 left a comment

Choose a reason for hiding this comment

Uh oh!

datqm commented May 13, 2026

Uh oh!

thomas-bc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomas-bc May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

datqm commented May 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thomas-bc May 13, 2026 •

edited

Loading