Auto Increment Telemetry Sequence Count Overflow

[pchapates]

Describe the bug
The API (called by CFE_SB_TransmitMsg) CFE_SBR_IncrementSequenceCounter has no protection to ensure the sequence counter doesn't exceed the maximum value dictated by the size of the field in the CCSDS primary header (14-bits w/ max value 16384.

To Reproduce
Create a for-loop that calls CFE_SB_TransmitMsg with the IncrementSequenceCount flag set to true. Have the loop repeat this at least 16385 times. View the resultant messages and verify that the sequence count stops incrementing and is stuck at the maximum value.

Expected behavior
CFE_SBR_IncrementSequenceCounter (or the caller CFE_SB_TransmitMsg) should check that the sequence counter does not exceed the maximum value dictated by the size in the header (16384). The sequence counter should be reset to 0 when it reaches the maximum.

Code snips
In CFE_SB_TransmitMsg:

/* For Tlm packets, increment the seq count if requested */
CFE_MSG_GetType(MsgPtr, &MsgType);
if((MsgType == CFE_MSG_Type_Tlm) && IncrementSequenceCount)
{
    CFE_SBR_IncrementSequenceCounter(RouteId);
    CFE_MSG_SetSequenceCount(MsgPtr, CFE_SBR_GetSequenceCounter(RouteId));
}

In CFE_SBR_IncrementSequenceCounter:

if (CFE_SBR_IsValidRouteId(RouteId))
{
    CFE_SBR_RDATA.RoutingTbl[CFE_SBR_RouteIdToValue(RouteId)].SeqCnt++;
}

System observed on:

• Hardware: N/A
• OS: N/A
• Versions: cFE: 24f7b31

Additional context
N/A

Reporter Info
PJ Chapates
Gateway VSM Flight Software Production
JSC, ER6

[jphickey]

I just did a quick check, I think the issue lies here:

cFE/modules/msg/fsw/src/cfe_msg_ccsdspri.c

Line 309 in 73c338d

if (MsgPtr == NULL || ((SeqCnt & ~CFE_MSG_SEQCNT_MASK) != 0))

For something like sequence count the MSG shouldn't enforce the sequence is within its range, the caller is not supposed to know the range of the underlying encapsulation (it is supposed to be abstract).

My recommendation would be to remove this extra check and it should work as expected, masking off the lower 14 bits to put into the message.

[pchapates]

If I'm following correctly, couldn't that be potentially misleading? If you mask off the lower 14 bits to put into the message, and the SB API just continues to auto-increment, your receiver may get two (or many) packets in quick succession with the same sequence count (equal to the max value that fits in the 14-bits).

[jphickey]

No, because it is the least significant bits that are masked off. It will just become a "mod 16384" counter at that point. (Upper bits just reflect the number of times its rolled over back to 0, which isn't that important).

It does suggest, however, that the MSG module also needs an API to compare/diff two sequence numbers, since a "Set" followed by "Get" won't return the same value after the roll over point. This means that if a receiver actually wants to check/verify the sequence, they'll have to account for a rollover themselves, which is not ideal.

[pchapates]

Oh right. Binary operations are confusing for me late on a Friday afternoon...

I don't think I have enough insight into how these sequence counters are intended to be used in practice to know whether that's the right approach or not.

[skliper]

How about keeping the check, since it could cause a strange rollover when the input value overflows and to solve this issue w/ minimum changes add an inline CFE_MSG_GetNextSequenceCounter API, just pass in a sequence count and it returns the next (rolling over appropriately). Could be used for testing for gaps, incrementing correctly in an abstract sense, etc. I'd prefer to put off any additional new functionality (beyond just what's needed to fix this) to the next development cycle if possible.

[pchapates]

I'd be fine with that.