diff --git a/CHANGELOG.md b/CHANGELOG.md index 1374e4423..1464bae80 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,14 +22,17 @@ 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 - add CFDP subsystem ID https://egit.irs.uni-stuttgart.de/fsfw/fsfw/pulls/742 - New object ID for persistent TM store PUS 15 +- `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`. @@ -38,14 +41,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); 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 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") {} }