FileDownlink cancel packet buffer is set to wrong size

[nathancheek]

F´ Version	v4.2.2
Affected Component	Svc::FileDownlink

---

Problem Description

If a component receives a cancel packet from Fw::FilePacket, and tries to deserialize using filePacket.fromBuffer(fwBuffer), it will fail with error status FW_DESERIALIZE_SIZE_MISMATCH.

The reason this occurs is because FileDownlink fails to set the buffer size specifically when generating cancel packets.

Svc::FileDownlink serializes four kinds of Fw::FilePacket packets into Fw::Buffer buffers, in order to send them downstream. For Start, Data, and End packets, the sendFilePacket() helper function properly sets the reused buffer size to bufferSize, then sets it back to FILEDOWNLINK_INTERNAL_BUFFER_SIZE after the downstream port call returns. However, for Cancel packets, which don't use the sendFilePacket() helper function, no call to setSize() occurs. This means it is sent downstream as size FILEDOWNLINK_INTERNAL_BUFFER_SIZE, rather than the correct size (5).

Context / Environment

I first hit this problem in v3.6, but it seems (by inspection) that the same behavior exists in dev.

How to Reproduce

1. Connect a component to the Svc::FileDownlink bufferSendOut output port
2. Component should try to deserialize each incoming FilePacket using filePacket.fromBuffer(fwBuffer)
3. Send a file using SendFile command (large enough that there is time to send Cancel command before it finishes)
4. Send Cancel command
5. Start and Data packets should be deserialized properly, but Cancel packet deserialization fails with status FW_DESERIALIZE_SIZE_MISMATCH

Expected Behavior

Buffers sent via Svc::FileDownlink bufferSendOut output port should first be set to the correct size to match the serialized Fw::FilePacket.

Notes

I have been trying to understand why Cancel packets are handled differently than Start, Data, and End packets. The latter all utilize a single reused m_buffer, while Cancel packets utilize a more tightly-scoped buffer in the sendCancelPacket() function. m_buffer points to m_memoryStore[0] and buffer (cancel buffer) points to m_memoryStore[1].

m_buffer has its size reset and data re-pointed to m_memoryStore[0] whenever a new file is downlinked. Whereas buffer (cancel buffer) gets size+data reset every time a new cancel packet is generated.

My best guess as to why Cancel packets use a separate underlying buffer is that this may have been a useful distinction prior to the removal of the "timeout" functionality with its ambiguous buffer ownership bug. I don't currently see a reason why the buffers need to be separate, so I cautiously propose refactoring how Cancel packets are generated, to use the same code and underlying buffer as the other types of file packets.

[nathancheek]

The Devin PR looks like it resolves the bug reported in this issue.

However, I was hoping there might be a broader conversation regarding the FileDownlink component. For starters, should Cancel packets still be processed separately from Start, Data, and End packets? (they currently use two separate underlying buffers but seem to only have one downstream at a time, so why couldn't they use the same buffer?)

Also, bufferReturn_handler() doesn't know how to handle buffers returned in COOLDOWN mode, since that mode was seemingly added later. I added logic to handle COOLDOWN mode the same as IDLE mode in #5287, which was recently merged into v3.6.x via #5288. Should that be pulled into v4's FileDownlink? Alternatively, now that timeout functionality has been removed from the component, there shouldn't be any "stale" buffers being returned to the component. So maybe a cleaner update would be to remove the entire "stale" buffer check from bufferReturn_handler()?

I can pull this discussion out into a different issue/PR. And if people would rather respond to a PR than an issue, I can go ahead and propose these changes via PR. But these questions regarding the structure of the FileDownlink component all seem interrelated so maybe worth discussing together.