Bug in TmPacketStoredPusC #478

New Issue

muellerr · 2021-09-15T18:41:19+02:00

muellerr commented

2021-09-15 18:41:19 +02:00

I found a bug which causes the FSFW to crash.

TmPacketStoredPusC::TmPacketStoredPusC(uint16_t apid, uint8_t service,
        uint8_t subservice, uint16_t packetSubcounter, SerializeIF *content,
        SerializeIF *header, uint16_t destinationId, uint8_t timeRefField) :
        TmPacketPusC(nullptr) {
    storeAddress.raw = StorageManagerIF::INVALID_ADDRESS;
    if (not TmPacketStoredBase::checkAndSetStore()) {
        return;
    }
    size_t sourceDataSize = 0;
    if (content != NULL) {
        sourceDataSize += content->getSerializedSize();
    }
    if (header != NULL) {
        sourceDataSize += header->getSerializedSize();
    }
    uint8_t *p_data = NULL;
    ReturnValue_t returnValue = store->getFreeElement(&storeAddress,
            (getPacketMinimumSize() + sourceDataSize), &p_data);
    if (returnValue != store->RETURN_OK) {
        TmPacketStoredBase::checkAndReportLostTm();
    }
    setData(p_data);
    initializeTmPacket(apid, service, subservice, packetSubcounter, destinationId, timeRefField);
    uint8_t *putDataHere = getSourceData();
    size_t size = 0;
    if (header != NULL) {
        header->serialize(&putDataHere, &size, sourceDataSize,
                SerializeIF::Endianness::BIG);
    }
    if (content != NULL) {
        content->serialize(&putDataHere, &size, sourceDataSize,
                SerializeIF::Endianness::BIG);
    }
    setPacketDataLength(
            sourceDataSize + sizeof(PUSTmDataFieldHeaderPusC) + CRC_SIZE - 1);
}

If getFreeElement fails, a nullptr is passed into initializeTmPacket.
The problem is, this is a constructor.. Im investigating whether returning prematurely solves the issue

I found a bug which causes the FSFW to crash. ```cpp TmPacketStoredPusC::TmPacketStoredPusC(uint16_t apid, uint8_t service, uint8_t subservice, uint16_t packetSubcounter, SerializeIF *content, SerializeIF *header, uint16_t destinationId, uint8_t timeRefField) : TmPacketPusC(nullptr) { storeAddress.raw = StorageManagerIF::INVALID_ADDRESS; if (not TmPacketStoredBase::checkAndSetStore()) { return; } size_t sourceDataSize = 0; if (content != NULL) { sourceDataSize += content->getSerializedSize(); } if (header != NULL) { sourceDataSize += header->getSerializedSize(); } uint8_t *p_data = NULL; ReturnValue_t returnValue = store->getFreeElement(&storeAddress, (getPacketMinimumSize() + sourceDataSize), &p_data); if (returnValue != store->RETURN_OK) { TmPacketStoredBase::checkAndReportLostTm(); } setData(p_data); initializeTmPacket(apid, service, subservice, packetSubcounter, destinationId, timeRefField); uint8_t *putDataHere = getSourceData(); size_t size = 0; if (header != NULL) { header->serialize(&putDataHere, &size, sourceDataSize, SerializeIF::Endianness::BIG); } if (content != NULL) { content->serialize(&putDataHere, &size, sourceDataSize, SerializeIF::Endianness::BIG); } setPacketDataLength( sourceDataSize + sizeof(PUSTmDataFieldHeaderPusC) + CRC_SIZE - 1); } ``` If `getFreeElement` fails, a `nullptr` is passed into `initializeTmPacket`. The problem is, this is a constructor.. Im investigating whether returning prematurely solves the issue

muellerr added the

bug

label 2021-09-15 18:41:54 +02:00

muellerr commented

2021-09-15 18:51:59 +02:00

Returning prematurely fixes the issue for the specific call here where an event was generated. The subsequent serialize call failed. However, the object is still invalid and other methods might cause issues.

Returning prematurely fixes the issue for the specific call here where an event was generated. The subsequent `serialize` call failed. However, the object is still invalid and other methods might cause issues.

gaisser commented

2021-09-16 10:28:30 +02:00

Hm this is one of those examples were a Constructor is able to fail and the object is in an invalid state. Calling initializeTmPacket if getFreeElement failed is a bug (and the serialize calls as well). We could change that to be an initialize function which is able to return instead of doing all this stuff in the constructor. This will make the API less clear unfortunately.

But one could argue that this should never happen as the TmStore needs to be large enough. This would be a risky argument because this might happen on very special occasions which are difficult to test.

I think failing here should not result in a crash. The TmPacket will be lost but that is reported.

Hm this is one of those examples were a Constructor is able to fail and the object is in an invalid state. Calling ``initializeTmPacket`` if `` getFreeElement`` failed is a bug (and the serialize calls as well). We could change that to be an initialize function which is able to return instead of doing all this stuff in the constructor. This will make the API less clear unfortunately. But one could argue that this should never happen as the TmStore needs to be large enough. This would be a risky argument because this might happen on very special occasions which are difficult to test. I think failing here should not result in a crash. The TmPacket will be lost but that is reported.

muellerr referenced a pull request that will close this issue

2021-09-27 11:10:19 +02:00

Increased TM stack robustness #483

gaisser commented

2021-09-27 22:09:36 +02:00

Ok I have checked the old FLP Implementation and the current fsfw:


ReturnValue_t TmPacketStoredBase::sendPacket(MessageQueueId_t destination,
        MessageQueueId_t sentFrom, bool doErrorReporting) {
    if (getAllTmData() == nullptr) {
        //SHOULDDO: More decent code.
        return HasReturnvaluesIF::RETURN_FAILED;
    }
	....
}

As long as the child implementation returns a nullptr for getAllTmData() this is safe to call. However, we should think about resolving the SHOULDDO.

Ok I have checked the old FLP Implementation and the current fsfw: ``` c++ ReturnValue_t TmPacketStoredBase::sendPacket(MessageQueueId_t destination, MessageQueueId_t sentFrom, bool doErrorReporting) { if (getAllTmData() == nullptr) { //SHOULDDO: More decent code. return HasReturnvaluesIF::RETURN_FAILED; } .... } ``` As long as the child implementation returns a nullptr for getAllTmData() this is safe to call. However, we should think about resolving the SHOULDDO.

gaisser commented

2021-09-27 22:18:41 +02:00

PusA and PusC share a lot of Code in TmPacketStoredPusX. This seems unreasonable to me.

muellerr referenced a pull request that will close this issue

2021-10-11 16:39:21 +02:00

Increased TM stack robustness #501

mohr closed this issue

2021-10-11 18:00:46 +02:00

mohr commented

2021-10-11 18:01:29 +02:00

@muellerr checked that combining A and C is not reasonable atm, so we're staying with the duality for now.

gaisser added this to the v2.0.1 milestone 2021-11-15 16:43:13 +01:00