From df97c582d7ab9bbb6734a82f5ca123405866c4e0 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Wed, 23 Mar 2022 16:48:17 +0100 Subject: [PATCH 1/8] possibly important fix for ring buffer write --- src/fsfw/container/SimpleRingBuffer.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/fsfw/container/SimpleRingBuffer.cpp b/src/fsfw/container/SimpleRingBuffer.cpp index bcf3cf20..0c95fe40 100644 --- a/src/fsfw/container/SimpleRingBuffer.cpp +++ b/src/fsfw/container/SimpleRingBuffer.cpp @@ -48,6 +48,9 @@ void SimpleRingBuffer::confirmBytesWritten(size_t amount) { } ReturnValue_t SimpleRingBuffer::writeData(const uint8_t* data, size_t amount) { + if(amount > maxSize()) { + return HasReturnvaluesIF::RETURN_FAILED; + } if (availableWriteSpace() >= amount or overwriteOld) { size_t amountTillWrap = writeTillWrap(); if (amountTillWrap >= amount) { From a891769a02949d82a144bc5ea8f0e6522b0ce14e Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Thu, 24 Mar 2022 11:32:27 +0100 Subject: [PATCH 2/8] ring buffer cfg error warning --- src/fsfw/container/SimpleRingBuffer.cpp | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/fsfw/container/SimpleRingBuffer.cpp b/src/fsfw/container/SimpleRingBuffer.cpp index 0c95fe40..7e9aac1a 100644 --- a/src/fsfw/container/SimpleRingBuffer.cpp +++ b/src/fsfw/container/SimpleRingBuffer.cpp @@ -1,4 +1,7 @@ #include "fsfw/container/SimpleRingBuffer.h" +#include "fsfw/FSFW.h" + +#include "fsfw/serviceinterface.h" #include @@ -48,7 +51,14 @@ void SimpleRingBuffer::confirmBytesWritten(size_t amount) { } ReturnValue_t SimpleRingBuffer::writeData(const uint8_t* data, size_t amount) { - if(amount > maxSize()) { + if(amount > getMaxSize()) { +#if FSFW_VERBOSE_LEVEL >= 1 +#if FSFW_CPP_OSTREAM_ENABLED == 1 + sif::error << "SimpleRingBuffer::writeData: Amount of data too large" << std::endl; +#else + sif::printError("SimpleRingBuffer::writeData: Amount of data too large\n"); +#endif +#endif return HasReturnvaluesIF::RETURN_FAILED; } if (availableWriteSpace() >= amount or overwriteOld) { From 8a44c498c5b24aba0da99557c616808f58d1e6a2 Mon Sep 17 00:00:00 2001 From: Cleanroom Laptop L590 Date: Thu, 24 Mar 2022 16:50:59 +0100 Subject: [PATCH 3/8] add two additional tests for ring buffer --- tests/src/fsfw_tests/unit/container/RingBufferTest.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/src/fsfw_tests/unit/container/RingBufferTest.cpp b/tests/src/fsfw_tests/unit/container/RingBufferTest.cpp index a83fa2ac..30ffd598 100644 --- a/tests/src/fsfw_tests/unit/container/RingBufferTest.cpp +++ b/tests/src/fsfw_tests/unit/container/RingBufferTest.cpp @@ -31,6 +31,8 @@ TEST_CASE("Ring Buffer Test", "[RingBufferTest]") { for (uint8_t i = 0; i < 9; i++) { CHECK(readBuffer[i] == i); } + REQUIRE(ringBuffer.writeData(testData, 1024) == retval::CATCH_FAILED); + REQUIRE(ringBuffer.writeData(nullptr, 5) == retval::CATCH_FAILED); } SECTION("Get Free Element Test") { From 879223f38fef0c6cd3705bb10efa674c701bb792 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Thu, 24 Mar 2022 20:57:42 +0100 Subject: [PATCH 4/8] added nullptr check --- src/fsfw/container/SimpleRingBuffer.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/fsfw/container/SimpleRingBuffer.cpp b/src/fsfw/container/SimpleRingBuffer.cpp index 7e9aac1a..437e72ea 100644 --- a/src/fsfw/container/SimpleRingBuffer.cpp +++ b/src/fsfw/container/SimpleRingBuffer.cpp @@ -51,6 +51,9 @@ void SimpleRingBuffer::confirmBytesWritten(size_t amount) { } ReturnValue_t SimpleRingBuffer::writeData(const uint8_t* data, size_t amount) { + if(data == nullptr) { + return HasReturnvaluesIF::RETURN_FAILED; + } if(amount > getMaxSize()) { #if FSFW_VERBOSE_LEVEL >= 1 #if FSFW_CPP_OSTREAM_ENABLED == 1 From 23f8e5cb410c193231b7ee96b00c4732817d5296 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Thu, 24 Mar 2022 21:01:21 +0100 Subject: [PATCH 5/8] some more tests fail --- tests/src/fsfw_tests/unit/container/RingBufferTest.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/src/fsfw_tests/unit/container/RingBufferTest.cpp b/tests/src/fsfw_tests/unit/container/RingBufferTest.cpp index 30ffd598..b365bed0 100644 --- a/tests/src/fsfw_tests/unit/container/RingBufferTest.cpp +++ b/tests/src/fsfw_tests/unit/container/RingBufferTest.cpp @@ -236,9 +236,8 @@ TEST_CASE("Ring Buffer Test3", "[RingBufferTest3]") { SECTION("Overflow") { REQUIRE(ringBuffer.availableWriteSpace() == 9); - // Writing more than the buffer is large, technically thats allowed - // But it is senseless and has undesired impact on read call - REQUIRE(ringBuffer.writeData(testData, 13) == retval::CATCH_OK); + // Writing more than the buffer is large. Will be rejected + REQUIRE(ringBuffer.writeData(testData, 13) == retval::CATCH_FAILED); REQUIRE(ringBuffer.getAvailableReadData() == 3); ringBuffer.clear(); uint8_t *ptr = nullptr; From ad57e6713e2460a997b8cc1a111da9902f10d9d6 Mon Sep 17 00:00:00 2001 From: Steffen Gaisser Date: Fri, 25 Mar 2022 15:20:06 +0100 Subject: [PATCH 6/8] Fixed Unittests --- .../fsfw_tests/unit/container/RingBufferTest.cpp | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/tests/src/fsfw_tests/unit/container/RingBufferTest.cpp b/tests/src/fsfw_tests/unit/container/RingBufferTest.cpp index b365bed0..f9e25451 100644 --- a/tests/src/fsfw_tests/unit/container/RingBufferTest.cpp +++ b/tests/src/fsfw_tests/unit/container/RingBufferTest.cpp @@ -146,12 +146,13 @@ TEST_CASE("Ring Buffer Test2", "[RingBufferTest2]") { SECTION("Overflow") { REQUIRE(ringBuffer.availableWriteSpace() == 9); - // Writing more than the buffer is large, technically thats allowed - // But it is senseless and has undesired impact on read call - REQUIRE(ringBuffer.writeData(testData, 13) == retval::CATCH_OK); - REQUIRE(ringBuffer.getAvailableReadData() == 3); + // We don't allow writing of Data that is larger than the ring buffer in total + REQUIRE(ringBuffer.getMaxSize() == 9); + REQUIRE(ringBuffer.writeData(testData, 13) == retval::CATCH_FAILED); + REQUIRE(ringBuffer.getAvailableReadData() == 0); ringBuffer.clear(); uint8_t *ptr = nullptr; + // With excess Bytes 13 Bytes can be written to this Buffer REQUIRE(ringBuffer.getFreeElement(&ptr, 13) == retval::CATCH_OK); REQUIRE(ptr != nullptr); memcpy(ptr, testData, 13); @@ -236,10 +237,13 @@ TEST_CASE("Ring Buffer Test3", "[RingBufferTest3]") { SECTION("Overflow") { REQUIRE(ringBuffer.availableWriteSpace() == 9); - // Writing more than the buffer is large. Will be rejected + // Writing more than the buffer is large. + // This write will be rejected and is seen as a configuration mistake REQUIRE(ringBuffer.writeData(testData, 13) == retval::CATCH_FAILED); - REQUIRE(ringBuffer.getAvailableReadData() == 3); + REQUIRE(ringBuffer.getAvailableReadData() == 0); ringBuffer.clear(); + // Using FreeElement allows the usage of excessBytes but + // should be used with caution uint8_t *ptr = nullptr; REQUIRE(ringBuffer.getFreeElement(&ptr, 13) == retval::CATCH_OK); REQUIRE(ptr != nullptr); From 70f575396db7beb9bb236e7b5ef007c9347c5bd7 Mon Sep 17 00:00:00 2001 From: Steffen Gaisser Date: Fri, 25 Mar 2022 15:27:22 +0100 Subject: [PATCH 7/8] Added changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c7d3b90b..a8e13578 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/). PR: https://egit.irs.uni-stuttgart.de/fsfw/fsfw/pulls/572 - HAL Devicehandlers: Periodic printout is run-time configurable now - `oneShotAction` flag in the `TestTask` class is not static anymore +- `SimpleRingBuffer::writeData` now checks if the amount is larger than the total size of the + Buffer and rejects such writeData calls with `HasReturnvaluesIF::RETURN_FAILED` + PR: https://egit.irs.uni-stuttgart.de/fsfw/fsfw/pulls/586 ## Removed From b7a316008a1642188a0e3e8e6b1789ae490d19e1 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Mon, 28 Mar 2022 15:07:46 +0200 Subject: [PATCH 8/8] increase allowed read bytes --- tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp b/tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp index 3ad26876..20677298 100644 --- a/tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp +++ b/tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp @@ -82,7 +82,7 @@ TEST_CASE("Command Executor", "[cmd-exec]") { readBytes = 0; sizesFifo.retrieve(&readBytes); // That's about the size of the reply - bool beTrue = (readBytes > 200) and (readBytes < 300); + bool beTrue = (readBytes > 200) and (readBytes < 400); REQUIRE(beTrue); uint8_t largerReadBuffer[1024] = {}; outputBuffer.readData(largerReadBuffer, readBytes);