search

nasa / CF

The Core Flight System (cFS) CFDP application.
Apache License 2.0
86 stars 46 forks source link

Numerous bugs in CF chunk processing edge cases #306

Closed skliper closed 2 years ago

skliper commented 2 years ago

Checklist (Please check before submitting)

Describe the bug As part of resolving #293, numerous bugs were discovered in cf_chunk.c both with adding to the list w/ CF_ChunkListAdd and computing gaps with CF_ChunkList_ComputeGaps.

Observed chunk add errors (covers described with non-inclusive end): 1) Adding a chunk that would completely replace chunks 2 and 3 (a CombineNext case). Starting list {offset, size}: {0,1}, {11, 2}, {23,3}, {36,4}, {50,5} so it covers 0-1 11-13 23-26 36-40 50-55 CF_ChunkListAdd with {20,21} so it should completely replace chunk 2:{23,3} and 3:{36,4} Results in {0,1}, {11,2}, {20,35}, {50,5} (note erroneous overlap of 2 and 3), expected: {0,1}, {11,2}, {20,21}, {50,5} covering 0-1 11-13 20-41 50-55

2) Adding a chunk that combines with chunk 1, 2, and 3 (prev, next, next). Starting list {offset, size}: {0,1}, {11, 2}, {23,3}, {36,4}, {50,5} so it covers 0-1 11-13 23-26 36-40 50-55 CF_ChunkListAdd with {12,25} so it should combine with chunk 1-3 Results in {0,1}, {11,29}, {36,4}, {50,5} (note erroneous overlap of 1 and 2), expected: {0,1}, {11,29}, {50,5} covering 0-1 11-40 50-55

3) Adding a chunk that is a subset of chunk 3 Starting list {offset, size}: {0,1}, {11, 2}, {23,3}, {36,4}, {50,5} so it covers 0-1 11-13 23-26 36-40 50-55 CF_ChunkListAdd with {37,2} so it should just drop since it's a subset of 3 Results in {11, 2}, {23,3}, {36,4}, {37,2}, {50,5} (note numerous issues), expected no change

Observed chunk gap errors (gaps and covers described with non-inclusive end): 1) Misses a leading gap. Starting list {offset, size}: {5,5}, {20,10}, {50,10} covers 5-10 20-30 50-60 so gaps 0-5, 10-20, 30-50 and anything after 60 CF_ChunkList_ComputeGaps with start 0, total 25 Results in {10,10}, so it missed the 0-5 gap

To Reproduce See scenarios above.

Expected behavior See scenarios above.

Code snips CF_Chunks_EraseRange has problems if start == end and the memmove size is wrong: https://github.com/nasa/CF/blob/a67eaf42c58df3cb154534eab8849189cf17979d/fsw/src/cf_chunk.c#L50-L52

CF_Chunks_CombinePrevious should combine whenever the offset is less than previous range (move ret=1 out of inner if): https://github.com/nasa/CF/blob/a67eaf42c58df3cb154534eab8849189cf17979d/fsw/src/cf_chunk.c#L152-L160

CF_Chunks_CombineNext is overly complex and broken... ended up refactoring completely to get it to work: https://github.com/nasa/CF/blob/a67eaf42c58df3cb154534eab8849189cf17979d/fsw/src/cf_chunk.c#L173-L222

CF_Chunks_ComputeGaps start logic is broken... again just refactored to straighten it out: https://github.com/nasa/CF/blob/a67eaf42c58df3cb154534eab8849189cf17979d/fsw/src/cf_chunk.c#L432-L451

System observed on: CI with #293 incorporated

Additional context None

Reporter Info Jacob Hageman - NASA/GSFC

skliper commented 2 years ago

Note the add cases are probably fairly unlikely in real ops, but an initial gap likely isn't all that rare.

The most nominal cases of send/receive with no drops work fine, and very simple drop cases are mostly OK.