fsfw/fsfw
fsfw
/
fsfw
10
3
Fork 6
Code Issues 31 Pull Requests 4 Releases 10 Wiki Activity

Space Packet Size Check #644

Closed
meierj wants to merge 4 commits from meier/upstream-pus-distributor-bugfix into development
pull from: meier/upstream-pus-distributor-bugfix
merge into: fsfw:development
Conversation 5 Commits 4 Files Changed 9 +39 -12
meierj commented 2022-06-22 18:32:37 +02:00
Owner
Copy Link
  • PUS distributor run into segmentation fault when PUS packet is too short
  • Now an acceptance failure report will be send in case the packet contains not enough data to represent a valid PUS packet
  • However an invalid PUS packet does not reveal the information about the TC Service ID and the sequence control number. Thus this fields will be set to zero in the acceptance failure report.
* PUS distributor run into segmentation fault when PUS packet is too short * Now an acceptance failure report will be send in case the packet contains not enough data to represent a valid PUS packet * However an invalid PUS packet does not reveal the information about the TC Service ID and the sequence control number. Thus this fields will be set to zero in the acceptance failure report.
meierj added 1 commit 2022-06-22 18:32:39 +02:00
fsfw/fsfw/pipeline/head There was a failure building this commit Details
fsfw/fsfw/pipeline/pr-development There was a failure building this commit Details
2e76250209 faulty handling of size check in pus distributor
meierj changed title from Space Packet Size Check to WIP: Space Packet Size Check 2022-06-22 18:33:01 +02:00
meierj added 1 commit 2022-06-22 19:11:44 +02:00
fsfw/fsfw/pipeline/head Build started... Details
fsfw/fsfw/pipeline/pr-development This commit looks good Details
298f764298 return value of setStoreSddress
meierj requested review from muellerr 2022-06-22 19:14:39 +02:00
meierj changed title from WIP: Space Packet Size Check to Space Packet Size Check 2022-06-22 19:14:48 +02:00
gaisser commented 2022-06-23 19:14:36 +02:00
Owner
Copy Link

I think sending a PUS Service 1 Answer is not the correct way here. This is not an application layer problem. A package being too short (from a valid frame?) is an issue of the layers beneath. Additionally, Service 1 Answers without an SSC and Packet ID can not be attributed to a TC.

I think sending a PUS Service 1 Answer is not the correct way here. This is not an application layer problem. A package being too short (from a valid frame?) is an issue of the layers beneath. Additionally, Service 1 Answers without an SSC and Packet ID can not be attributed to a TC.
meierj closed this pull request 2022-07-14 09:21:48 +02:00
meierj reopened this pull request 2022-07-14 09:23:12 +02:00
meierj commented 2022-07-14 09:46:32 +02:00
Author
Owner
Copy Link

I think sending a PUS Service 1 Answer is not the correct way here. This is not an application layer problem. A package being too short (from a valid frame?) is an issue of the layers beneath. Additionally, Service 1 Answers without an SSC and Packet ID can not be attributed to a TC.

I agree that sending a PUS service 1 answer might be the wrong way to handle an invalid PUS packet but in my opinion the length check should still be part of the application layer because the CCSDS distributor does not know the packet structure of upper layers (e.g. the size of the PUS header). Maybe it would be better to just print a warning and discard the data. This would be enough to prevent the OBSW from running into a segmentation fault when an invalid PUS packet arrives at the PUS distributor.

> I think sending a PUS Service 1 Answer is not the correct way here. This is not an application layer problem. A package being too short (from a valid frame?) is an issue of the layers beneath. Additionally, Service 1 Answers without an SSC and Packet ID can not be attributed to a TC. I agree that sending a PUS service 1 answer might be the wrong way to handle an invalid PUS packet but in my opinion the length check should still be part of the application layer because the CCSDS distributor does not know the packet structure of upper layers (e.g. the size of the PUS header). Maybe it would be better to just print a warning and discard the data. This would be enough to prevent the OBSW from running into a segmentation fault when an invalid PUS packet arrives at the PUS distributor.
👍 1
gaisser commented 2022-07-14 18:11:24 +02:00
Owner
Copy Link

I think sending a PUS Service 1 Answer is not the correct way here. This is not an application layer problem. A package being too short (from a valid frame?) is an issue of the layers beneath. Additionally, Service 1 Answers without an SSC and Packet ID can not be attributed to a TC.

I agree that sending a PUS service 1 answer might be the wrong way to handle an invalid PUS packet but in my opinion the length check should still be part of the application layer because the CCSDS distributor does not know the packet structure of upper layers (e.g. the size of the PUS header). Maybe it would be better to just print a warning and discard the data. This would be enough to prevent the OBSW from running into a segmentation fault when an invalid PUS packet arrives at the PUS distributor.

I agree, this should never run into a segmentation fault. We could trigger an event in addition to the print of a warning if we expect this to happen only in rare circumstances.

> > I think sending a PUS Service 1 Answer is not the correct way here. This is not an application layer problem. A package being too short (from a valid frame?) is an issue of the layers beneath. Additionally, Service 1 Answers without an SSC and Packet ID can not be attributed to a TC. > > I agree that sending a PUS service 1 answer might be the wrong way to handle an invalid PUS packet but in my opinion the length check should still be part of the application layer because the CCSDS distributor does not know the packet structure of upper layers (e.g. the size of the PUS header). Maybe it would be better to just print a warning and discard the data. This would be enough to prevent the OBSW from running into a segmentation fault when an invalid PUS packet arrives at the PUS distributor. I agree, this should never run into a segmentation fault. We could trigger an event in addition to the print of a warning if we expect this to happen only in rare circumstances.
muellerr added 1 commit 2022-07-15 10:16:41 +02:00
fsfw/fsfw/pipeline/pr-development There was a failure building this commit Details
ec8e5cc123 Merge branch 'development' into meier/upstream-pus-distributor-bugfix
muellerr added 1 commit 2022-07-18 15:12:33 +02:00
fsfw/fsfw/pipeline/pr-development There was a failure building this commit Details
fsfw/fsfw/pipeline/head This commit looks good Details
b96f5a2552 Merge branch 'development' into meier/upstream-pus-distributor-bugfix
muellerr commented 2022-07-21 13:26:44 +02:00
Owner
Copy Link

I am working on a complete overhaul of the TMTC packet stack which comes with a complete test suite. I will look into ensuring this case is covered as well.

I am working on a complete overhaul of the TMTC packet stack which comes with a complete test suite. I will look into ensuring this case is covered as well.
gaisser added this to the v6.0.0 milestone 2022-07-25 15:24:08 +02:00
mohr commented 2022-09-13 14:25:23 +02:00
Owner
Copy Link

resolved by #655

resolved by #655
mohr closed this pull request 2022-09-13 14:25:24 +02:00
mohr removed this from the v6.0.0 milestone 2023-02-09 16:06:41 +01:00
Some checks failed
fsfw/fsfw/pipeline/pr-development There was a failure building this commit
Details
fsfw/fsfw/pipeline/head This commit looks good
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
muellerr
Labels
Clear labels   
API Change

Changes the API, so Users need to adapted

  
Breaking API Change

Changes the API in a non backwards compatible way

  
bug

Something is not working

  
build

Issues related to the buildsystem, including CI

  
cosmetics

Small change most users won't notice

  
Documentation

Improvement of code documentation

  
duplicate

This issue or pull request already exists

  
feature

New feature

  
help wanted

Need some help

  
hotfix

Quick or important fix that is merged directly into master, causing a new revision

  
invalid

#worksforme

  
question

More information is needed

  
Refactor

Contains a refactoring of some code

  
Tests

Issues related to testing code

  
wontfix

This won't be fixed

No Label
API Change
Breaking API Change
bug
build
cosmetics
Documentation
duplicate
feature
help wanted
hotfix
invalid
question
Refactor
Tests
wontfix
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: fsfw/fsfw#644
No description provided.
Delete Branch "meier/upstream-pus-distributor-bugfix"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?