From 60f6ef5f1f9c8fdf7071085fc190bfb90dd960e1 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Fri, 1 Sep 2023 12:18:55 +0200 Subject: [PATCH 1/3] ActionHelper update --- CHANGELOG.md | 10 +++------- src/fsfw/action/ActionHelper.cpp | 21 ++++++++++++++------- src/fsfw/action/CommandActionHelper.h | 4 ++-- 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 66e8c0d25..4b2d9bb2d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,8 +27,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - add CFDP subsystem ID https://egit.irs.uni-stuttgart.de/fsfw/fsfw/pulls/742 +- `EventManager`: Add function to print all listeners. ## Changed + - Bump ETL version to 20.35.14 https://egit.irs.uni-stuttgart.de/fsfw/fsfw/pulls/748 - Renamed `PCDU_2` subsystem ID to `POWER_SWITCH_IF`. @@ -37,14 +39,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/). https://egit.irs.uni-stuttgart.de/fsfw/fsfw/pulls/743 - Assert that `FixedArrayList` is larger than 0 at compile time. https://egit.irs.uni-stuttgart.de/fsfw/fsfw/pulls/740 - -## Added - -- `EventManager`: Add function to print all listeners. - -## Changed - - `EventManager`: Queue depth is configurable now +- `ActionHelper`: Allow execution of actions without additional data # [v6.0.0] 2023-02-10 diff --git a/src/fsfw/action/ActionHelper.cpp b/src/fsfw/action/ActionHelper.cpp index fd6c8afba..81a1a727c 100644 --- a/src/fsfw/action/ActionHelper.cpp +++ b/src/fsfw/action/ActionHelper.cpp @@ -59,17 +59,24 @@ void ActionHelper::setQueueToUse(MessageQueueIF* queue) { queueToUse = queue; } void ActionHelper::prepareExecution(MessageQueueId_t commandedBy, ActionId_t actionId, store_address_t dataAddress) { + bool hasAdditionalData = false; const uint8_t* dataPtr = nullptr; size_t size = 0; - ReturnValue_t result = ipcStore->getData(dataAddress, &dataPtr, &size); - if (result != returnvalue::OK) { - CommandMessage reply; - ActionMessage::setStepReply(&reply, actionId, 0, result); - queueToUse->sendMessage(commandedBy, &reply); - return; + ReturnValue_t result; + if (dataAddress != store_address_t::invalid()) { + hasAdditionalData = true; + ReturnValue_t result = ipcStore->getData(dataAddress, &dataPtr, &size); + if (result != returnvalue::OK) { + CommandMessage reply; + ActionMessage::setStepReply(&reply, actionId, 0, result); + queueToUse->sendMessage(commandedBy, &reply); + return; + } } result = owner->executeAction(actionId, commandedBy, dataPtr, size); - ipcStore->deleteData(dataAddress); + if (hasAdditionalData) { + ipcStore->deleteData(dataAddress); + } if (result == HasActionsIF::EXECUTION_FINISHED) { CommandMessage reply; ActionMessage::setCompletionReply(&reply, actionId, true, result); diff --git a/src/fsfw/action/CommandActionHelper.h b/src/fsfw/action/CommandActionHelper.h index 55176e09b..ac8ed1ba8 100644 --- a/src/fsfw/action/CommandActionHelper.h +++ b/src/fsfw/action/CommandActionHelper.h @@ -16,8 +16,8 @@ class CommandActionHelper { public: explicit CommandActionHelper(CommandsActionsIF* owner); virtual ~CommandActionHelper(); - ReturnValue_t commandAction(object_id_t commandTo, ActionId_t actionId, const uint8_t* data, - uint32_t size); + ReturnValue_t commandAction(object_id_t commandTo, ActionId_t actionId, + const uint8_t* data = nullptr, uint32_t size = 0); ReturnValue_t commandAction(object_id_t commandTo, ActionId_t actionId, SerializeIF* data); ReturnValue_t initialize(); ReturnValue_t handleReply(CommandMessage* reply); From 1569416a8f990719b7183c1f7ad661de544e1142 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Fri, 1 Sep 2023 12:33:25 +0200 Subject: [PATCH 2/3] fix action helper tests --- unittests/action/TestActionHelper.cpp | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/unittests/action/TestActionHelper.cpp b/unittests/action/TestActionHelper.cpp index d99700d7a..de021bb83 100644 --- a/unittests/action/TestActionHelper.cpp +++ b/unittests/action/TestActionHelper.cpp @@ -27,12 +27,7 @@ TEST_CASE("Action Helper", "[action]") { CHECK(not testDhMock.executeActionCalled); REQUIRE(actionHelper.handleActionMessage(&actionMessage) == returnvalue::OK); CHECK(testDhMock.executeActionCalled); - // No message is sent if everything is alright. CHECK(not testMqMock.wasMessageSent()); - store_address_t invalidAddress; - ActionMessage::setCommand(&actionMessage, testActionId, invalidAddress); - actionHelper.handleActionMessage(&actionMessage); - CHECK(testMqMock.wasMessageSent()); const uint8_t* ptr = nullptr; size_t size = 0; REQUIRE(ipcStore->getData(paramAddress, &ptr, &size) == @@ -44,6 +39,10 @@ TEST_CASE("Action Helper", "[action]") { for (uint8_t i = 0; i < 3; i++) { REQUIRE(ptr[i] == (i + 1)); } + // Action message without application data is also valid + store_address_t invalidAddress; + ActionMessage::setCommand(&actionMessage, testActionId, invalidAddress); + actionHelper.handleActionMessage(&actionMessage); testDhMock.clearBuffer(); } @@ -95,17 +94,5 @@ TEST_CASE("Action Helper", "[action]") { REQUIRE(ActionMessage::getActionId(&testMessage) == testActionId); } - SECTION("Missing IPC Data") { - ActionMessage::setCommand(&actionMessage, testActionId, store_address_t::invalid()); - CHECK(not testDhMock.executeActionCalled); - REQUIRE(actionHelper.handleActionMessage(&actionMessage) == returnvalue::OK); - CommandMessage testMessage; - REQUIRE(testMqMock.getNextSentMessage(testMessage) == returnvalue::OK); - REQUIRE(testMessage.getCommand() == static_cast(ActionMessage::STEP_FAILED)); - REQUIRE(ActionMessage::getReturnCode(&testMessage) == - static_cast(StorageManagerIF::ILLEGAL_STORAGE_ID)); - REQUIRE(ActionMessage::getStep(&testMessage) == 0); - } - SECTION("Data Reply") {} } From e5910c77e94a78a61a15d4b2a2f80d6bf29c398b Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Wed, 6 Sep 2023 20:51:53 +0200 Subject: [PATCH 3/3] bugfix for local pool --- CHANGELOG.md | 1 + src/fsfw/storagemanager/LocalPool.cpp | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 66e8c0d25..e47581943 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). https://egit.irs.uni-stuttgart.de/fsfw/fsfw/pulls/745 - Small tweak for version getter https://egit.irs.uni-stuttgart.de/fsfw/fsfw/pulls/744 +- Important bugfix for `LocalPool::delete` function, where an overflow could possibly lead to UB. ## Added diff --git a/src/fsfw/storagemanager/LocalPool.cpp b/src/fsfw/storagemanager/LocalPool.cpp index b62c19b60..9a4b53a6d 100644 --- a/src/fsfw/storagemanager/LocalPool.cpp +++ b/src/fsfw/storagemanager/LocalPool.cpp @@ -89,7 +89,7 @@ ReturnValue_t LocalPool::deleteData(store_address_t storeId) { ReturnValue_t status = returnvalue::OK; size_type pageSize = getSubpoolElementSize(storeId.poolIndex); if ((pageSize != 0) and (storeId.packetIndex < numberOfElements[storeId.poolIndex])) { - uint16_t packetPosition = getRawPosition(storeId); + size_type packetPosition = getRawPosition(storeId); uint8_t* ptr = &store[storeId.poolIndex][packetPosition]; std::memset(ptr, 0, pageSize); // Set free list