out of bounds access in DLE encoder #492

muellerr · 2021-09-30T11:29:09+02:00

muellerr commented

2021-09-30 11:29:09 +02:00

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

b0cbd40e64

possible bugfix for DLE encoder

0f90d50065 Merge remote-tracking branch 'origin/development' into mueller/dle-possible-bugfix

f76f462022

test added

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

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

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

afb472996c refactoring, code more understandable

mohr approved these changes 2021-10-01 13:12:53 +02:00

Dismissed

mohr left a comment

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

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

9f856761e2 Merge branch 'development' into mueller/dle-possible-bugfix

muellerr added 2 commits 2021-10-04 14:38:56 +02:00

2439613f21 preserve STX char

f2d0a0d9ee Merge branch 'mueller/dle-possible-bugfix' of https://egit.irs.uni-stuttgart.de/fsfw/fsfw into mueller/dle-possible-bugfix

gaisser added 1 commit 2021-10-04 14:40:50 +02:00

53400c8bfa Merge branch 'development' into mueller/dle-possible-bugfix

mohr approved these changes 2021-10-04 14:43:04 +02:00

mohr left a comment

looking good now

mohr merged commit a977302a53 into development

2021-10-04 14:43:54 +02:00

mohr referenced this issue from a commit

2021-10-04 14:43:54 +02:00

Merge pull request 'out of bounds access in DLE encoder' (#492) from mueller/dle-possible-bugfix into development

mohr deleted branch mueller/dle-possible-bugfix

2021-10-04 14:44:02 +02:00

Sign in to join this conversation.

No reviewers

No Label

No Milestone

No Assignees

3 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#492