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

out of bounds access in DLE encoder #492

Merged
mohr merged 8 commits from mueller/dle-possible-bugfix into development 2021-10-04 14:43:54 +02:00
Conversation 4 Commits 8 Files Changed 2 +29 -22
muellerr commented 2021-09-30 11:29:09 +02:00
Owner
Copy Link

Found when writing unit tests for Rust.
Rust would not have an array out of bounds access (even for reading) and panicked.

The test for C++ was missing (was added as well) but might not have caught the array out of bounds error.

In the while loop, the encoder might step out of the while loop if encoded index is equal to the source stream length. The problem is that the index will be invalid to use because it is incremented but is still used at line 208

Found when writing unit tests for Rust. Rust would not have an array out of bounds access (even for reading) and panicked. The test for C++ was missing (was added as well) but might not have caught the array out of bounds error. In the while loop, the encoder might step out of the while loop if encoded index is equal to the source stream length. The problem is that the index will be invalid to use because it is incremented but is still used at line 208
muellerr added 3 commits 2021-09-30 11:29:10 +02:00
muellerr added the
bug
 label 2021-09-30 11:29:17 +02:00
muellerr requested review from gaisser 2021-09-30 11:30:52 +02:00
mohr commented 2021-09-30 15:58:51 +02:00
Owner
Copy Link

Good catch.

I am wondering if it would be better (and easier to understand for future readers) if we changed the while loop in decodeStreamEscaped() similar to the one in decodeStreamNonEscaped(), where the while loop only protects out of bound reads (as in (encodedIndex < sourceStreamLen) and (decodedIndex < maxDestStreamlen)) and move the exit conditions completely into the loop, that is checking for STX and ETX.

Results would be:

  • Simpler while loop
  • While loop conditions stays < sourceStreamLen which I find more intuitive
  • All special charaters are handled explicitely in the loop (maybe then a switch might be approptriate)

What do you think about this?

Good catch. I am wondering if it would be better (and easier to understand for future readers) if we changed the while loop in `decodeStreamEscaped()` similar to the one in `decodeStreamNonEscaped()`, where the while loop only protects out of bound reads (as in `(encodedIndex < sourceStreamLen) and (decodedIndex < maxDestStreamlen)`) and move the exit conditions completely into the loop, that is checking for STX and ETX. Results would be: * Simpler while loop * While loop conditions stays `< sourceStreamLen` which I find more intuitive * All special charaters are handled explicitely in the loop (maybe then a `switch` might be approptriate) What do you think about this?
muellerr commented 2021-09-30 16:50:39 +02:00
Author
Owner
Copy Link

I like this idea, it makes the code more understandable. I added it to the PR for now, the unit tests verified that everything is still working

I like this idea, it makes the code more understandable. I added it to the PR for now, the unit tests verified that everything is still working
muellerr added 1 commit 2021-09-30 16:51:39 +02:00
mohr approved these changes 2021-10-01 13:12:53 +02:00
Dismissed
mohr left a comment
Owner
Copy Link

Looks good, minor change below

Looks good, minor change below
src/fsfw/globalfunctions/DleEncoder.cpp Outdated
@ -200,2 +199,3 @@
}
else {
case(STX_CHAR): {
*readLen = ++encodedIndex;
mohr commented 2021-10-01 13:11:05 +02:00
Owner
Copy Link

I think this should be *readLen = encodedIndex;, we should preserve the STX as it might start a new, valid frame.

I think this should be `*readLen = encodedIndex;`, we should preserve the `STX` as it might start a new, valid frame.
mohr marked this conversation as resolved
mohr removed review request for gaisser 2021-10-01 13:13:24 +02:00
mohr self-assigned this 2021-10-01 13:13:37 +02:00
mohr dismissed mohr’s review 2021-10-01 13:14:03 +02:00
Reason:

clicked wrong button

mohr changed title from possible bugfix for DLE encoder to out of bounds access in DLE encoder 2021-10-01 13:15:23 +02:00
mohr added this to the v2.0.0 milestone 2021-10-01 13:30:07 +02:00
muellerr added 1 commit 2021-10-02 12:24:40 +02:00
muellerr added 2 commits 2021-10-04 14:38:56 +02:00
gaisser added 1 commit 2021-10-04 14:40:50 +02:00
mohr approved these changes 2021-10-04 14:43:04 +02:00
mohr left a comment
Owner
Copy Link

looking good now

looking good now
mohr merged commit a977302a53 into development 2021-10-04 14:43:54 +02:00
mohr deleted branch mueller/dle-possible-bugfix 2021-10-04 14:44:02 +02:00
Sign in to join this conversation.
Reviewers
No Reviewers
mohr
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
bug
Milestone
No items
No Milestone v2.0.0
Assignees
Clear assignees
ahinkel dwoodward gaisser hanke jenkins_buildfix lrajer meierj mohr muellerr taheran tbaumgartl
No Assignees mohr
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fsfw/fsfw#492
No description provided.
Delete Branch "mueller/dle-possible-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?