From 2e76250209f3dfd95159bbb829323e1aafbc1054 Mon Sep 17 00:00:00 2001 From: Jakob Meier Date: Wed, 22 Jun 2022 18:19:06 +0200 Subject: [PATCH 1/2] faulty handling of size check in pus distributor --- src/fsfw/tcdistribution/CFDPDistributor.cpp | 6 +++++- src/fsfw/tcdistribution/PUSDistributor.cpp | 15 ++++++++++++++- src/fsfw/tcdistribution/PUSDistributor.h | 5 +++++ src/fsfw/tcdistribution/TcDistributor.h | 1 + src/fsfw/tmtcpacket/cfdp/CFDPPacketStored.cpp | 8 ++++---- src/fsfw/tmtcpacket/cfdp/CFDPPacketStored.h | 2 +- src/fsfw/tmtcpacket/pus/tc/TcPacketStoredBase.cpp | 6 +++--- src/fsfw/tmtcpacket/pus/tc/TcPacketStoredIF.h | 5 ++++- 8 files changed, 37 insertions(+), 11 deletions(-) diff --git a/src/fsfw/tcdistribution/CFDPDistributor.cpp b/src/fsfw/tcdistribution/CFDPDistributor.cpp index d8be1543e..3b679d831 100644 --- a/src/fsfw/tcdistribution/CFDPDistributor.cpp +++ b/src/fsfw/tcdistribution/CFDPDistributor.cpp @@ -33,7 +33,11 @@ CFDPDistributor::TcMqMapIter CFDPDistributor::selectDestination() { if (this->currentPacket == nullptr) { return queueMapIt; } - this->currentPacket->setStoreAddress(this->currentMessage.getStorageId()); + ReturnValue_t result = this->currentPacket->setStoreAddress(this->currentMessage.getStorageId()); + if (result != HasReturnvaluesIF::RETURN_OK) { + tcStatus = PACKET_TOO_SHORT; + return this->queueMap.end(); + } if (currentPacket->getWholeData() != nullptr) { tcStatus = checker.checkPacket(currentPacket); if (tcStatus != HasReturnvaluesIF::RETURN_OK) { diff --git a/src/fsfw/tcdistribution/PUSDistributor.cpp b/src/fsfw/tcdistribution/PUSDistributor.cpp index dad002a1a..6ce36701e 100644 --- a/src/fsfw/tcdistribution/PUSDistributor.cpp +++ b/src/fsfw/tcdistribution/PUSDistributor.cpp @@ -27,7 +27,12 @@ PUSDistributor::TcMqMapIter PUSDistributor::selectDestination() { if (this->currentPacket == nullptr) { return queueMapIt; } - this->currentPacket->setStoreAddress(this->currentMessage.getStorageId(), currentPacket); + ReturnValue_t result = + this->currentPacket->setStoreAddress(this->currentMessage.getStorageId(), currentPacket); + if (result != HasReturnvaluesIF::RETURN_OK) { + tcStatus = PACKET_TOO_SHORT; + return this->queueMap.end(); + } if (currentPacket->getWholeData() != nullptr) { tcStatus = checker.checkPacket(currentPacket); if (tcStatus != HasReturnvaluesIF::RETURN_OK) { @@ -106,6 +111,14 @@ ReturnValue_t PUSDistributor::registerService(AcceptsTelecommandsIF* service) { MessageQueueId_t PUSDistributor::getRequestQueue() { return tcQueue->getId(); } ReturnValue_t PUSDistributor::callbackAfterSending(ReturnValue_t queueStatus) { + if (tcStatus == PACKET_LOST or tcStatus == PACKET_TOO_SHORT) { + // The ack flags, packet id and sequence control are unknown for a packet which is too short or + // has been lost + this->verifyChannel.sendFailureReport(tc_verification::ACCEPTANCE_FAILURE, UNKNOWN_ACK_FLAGS, + UNKNOWN_PACKET_ID, UNKNOWN_SEQUENCE_CONTROL, tcStatus); + currentPacket->deletePacket(); + return RETURN_FAILED; + } if (queueStatus != RETURN_OK) { tcStatus = queueStatus; } diff --git a/src/fsfw/tcdistribution/PUSDistributor.h b/src/fsfw/tcdistribution/PUSDistributor.h index c069c81b8..7fc6d3ba9 100644 --- a/src/fsfw/tcdistribution/PUSDistributor.h +++ b/src/fsfw/tcdistribution/PUSDistributor.h @@ -72,6 +72,11 @@ class PUSDistributor : public TcDistributor, public PUSDistributorIF, public Acc * success/failure messages. */ ReturnValue_t callbackAfterSending(ReturnValue_t queueStatus) override; + + private: + static const uint8_t UNKNOWN_ACK_FLAGS = 0; + static const uint8_t UNKNOWN_PACKET_ID = 0; + static const uint8_t UNKNOWN_SEQUENCE_CONTROL = 0; }; #endif /* FSFW_TCDISTRIBUTION_PUSDISTRIBUTOR_H_ */ diff --git a/src/fsfw/tcdistribution/TcDistributor.h b/src/fsfw/tcdistribution/TcDistributor.h index 1b783ff49..4f1cd761c 100644 --- a/src/fsfw/tcdistribution/TcDistributor.h +++ b/src/fsfw/tcdistribution/TcDistributor.h @@ -36,6 +36,7 @@ class TcDistributor : public SystemObject, public ExecutableObjectIF, public Has static constexpr ReturnValue_t PACKET_LOST = MAKE_RETURN_CODE(1); static constexpr ReturnValue_t DESTINATION_NOT_FOUND = MAKE_RETURN_CODE(2); static constexpr ReturnValue_t SERVICE_ID_ALREADY_EXISTS = MAKE_RETURN_CODE(3); + static constexpr ReturnValue_t PACKET_TOO_SHORT = MAKE_RETURN_CODE(4); /** * Within the default constructor, the SystemObject id is set and the * message queue is initialized. diff --git a/src/fsfw/tmtcpacket/cfdp/CFDPPacketStored.cpp b/src/fsfw/tmtcpacket/cfdp/CFDPPacketStored.cpp index 9a410b40b..ad03fc8cf 100644 --- a/src/fsfw/tmtcpacket/cfdp/CFDPPacketStored.cpp +++ b/src/fsfw/tmtcpacket/cfdp/CFDPPacketStored.cpp @@ -37,7 +37,7 @@ ReturnValue_t CFDPPacketStored::deletePacket() { // CFDPPacket* CFDPPacketStored::getPacketBase() { // return this; // } -void CFDPPacketStored::setStoreAddress(store_address_t setAddress) { +ReturnValue_t CFDPPacketStored::setStoreAddress(store_address_t setAddress) { this->storeAddress = setAddress; const uint8_t* tempData = nullptr; size_t tempSize; @@ -46,11 +46,11 @@ void CFDPPacketStored::setStoreAddress(store_address_t setAddress) { status = this->store->getData(this->storeAddress, &tempData, &tempSize); } if (status == StorageManagerIF::RETURN_OK) { - this->setData(const_cast(tempData), tempSize); + return this->setData(const_cast(tempData), tempSize); } else { - // To circumvent size checks - this->setData(nullptr, -1); this->storeAddress.raw = StorageManagerIF::INVALID_ADDRESS; + // To circumvent size checks + return this->setData(nullptr, -1); } } diff --git a/src/fsfw/tmtcpacket/cfdp/CFDPPacketStored.h b/src/fsfw/tmtcpacket/cfdp/CFDPPacketStored.h index f9c73bdd9..eba8980c3 100644 --- a/src/fsfw/tmtcpacket/cfdp/CFDPPacketStored.h +++ b/src/fsfw/tmtcpacket/cfdp/CFDPPacketStored.h @@ -29,7 +29,7 @@ class CFDPPacketStored : public CFDPPacket, public TcPacketStoredBase { */ ReturnValue_t getData(const uint8_t** dataPtr, size_t* dataSize); - void setStoreAddress(store_address_t setAddress); + ReturnValue_t setStoreAddress(store_address_t setAddress); store_address_t getStoreAddress(); diff --git a/src/fsfw/tmtcpacket/pus/tc/TcPacketStoredBase.cpp b/src/fsfw/tmtcpacket/pus/tc/TcPacketStoredBase.cpp index 229185262..e2f29e779 100644 --- a/src/fsfw/tmtcpacket/pus/tc/TcPacketStoredBase.cpp +++ b/src/fsfw/tmtcpacket/pus/tc/TcPacketStoredBase.cpp @@ -42,7 +42,7 @@ bool TcPacketStoredBase::checkAndSetStore() { return true; } -void TcPacketStoredBase::setStoreAddress(store_address_t setAddress, +ReturnValue_t TcPacketStoredBase::setStoreAddress(store_address_t setAddress, RedirectableDataPointerIF* packet) { this->storeAddress = setAddress; const uint8_t* tempData = nullptr; @@ -53,10 +53,10 @@ void TcPacketStoredBase::setStoreAddress(store_address_t setAddress, } if (status == StorageManagerIF::RETURN_OK) { - packet->setData(const_cast(tempData), tempSize); + return packet->setData(const_cast(tempData), tempSize); } else { - packet->setData(nullptr, -1); this->storeAddress.raw = StorageManagerIF::INVALID_ADDRESS; + return packet->setData(nullptr, -1); } } diff --git a/src/fsfw/tmtcpacket/pus/tc/TcPacketStoredIF.h b/src/fsfw/tmtcpacket/pus/tc/TcPacketStoredIF.h index 7ac8c3319..826791253 100644 --- a/src/fsfw/tmtcpacket/pus/tc/TcPacketStoredIF.h +++ b/src/fsfw/tmtcpacket/pus/tc/TcPacketStoredIF.h @@ -16,8 +16,11 @@ class TcPacketStoredIF { * With this call, the stored packet can be set to another packet in a store. This is useful * if the packet is a class member and used for more than one packet. * @param setAddress The new packet id to link to. + * + * @return RETURN_OK if successful otherwise error return code. */ - virtual void setStoreAddress(store_address_t setAddress, RedirectableDataPointerIF* packet) = 0; + virtual ReturnValue_t setStoreAddress(store_address_t setAddress, + RedirectableDataPointerIF* packet) = 0; virtual store_address_t getStoreAddress() = 0; -- 2.34.1 From 298f764298b1b27e916847845dd5f46753cb1cdf Mon Sep 17 00:00:00 2001 From: Jakob Meier Date: Wed, 22 Jun 2022 19:11:51 +0200 Subject: [PATCH 2/2] return value of setStoreSddress --- src/fsfw/tmtcpacket/pus/tc/TcPacketStoredBase.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/fsfw/tmtcpacket/pus/tc/TcPacketStoredBase.h b/src/fsfw/tmtcpacket/pus/tc/TcPacketStoredBase.h index ece0e4827..adee9598a 100644 --- a/src/fsfw/tmtcpacket/pus/tc/TcPacketStoredBase.h +++ b/src/fsfw/tmtcpacket/pus/tc/TcPacketStoredBase.h @@ -38,7 +38,8 @@ class TcPacketStoredBase : public TcPacketStoredIF { */ ReturnValue_t getData(const uint8_t** dataPtr, size_t* dataSize) override; - void setStoreAddress(store_address_t setAddress, RedirectableDataPointerIF* packet) override; + ReturnValue_t setStoreAddress(store_address_t setAddress, + RedirectableDataPointerIF* packet) override; store_address_t getStoreAddress() override; /** -- 2.34.1