Most versions of OTAImageProcessorImpl::SetBlock are broken

[bzbarsky-apple]

Problem

The K32W, Linux, and EFR32 implementations of OTAImageProcessorImpl::SetBlock all have the same problem, due to being copy/pasted from each other. If the blocks coming in are different sizes (which is very much allowed in bdx), then any block that's larger than the smallest preceding block will fail.

Specifically, CopySpanToMutableSpan, which is used in the function does the following:

1. Ensures that the source span fits inside the target span.
2. Copies the data.
3. Sets the size of the target span to the size of the source span.

Step 3 is key. It means that mBlock will be the size of the smallest block seen so far.

The implementation on ESP32 does not have this problem, though it will do too much releasing/allocating because it does not keep track of the actual available space in mBlock, just of the space used by the last block copied into it.

Proposed Solution

Ideally, stop having multiple copies of this (clearly hard to get right) code. Have one copy, that we make correct and everyone uses.

Failing that, at least copy/paste the non-buggy version.

[selissia]

Testing the fix for this is currently blocked by #13429

[bzbarsky-apple]

Not fixed. Just fixed for Linux, still broken for Ameba, EFR32, qpg (this last fixed in #14388).

[selissia]

Not fixed. Just fixed for Linux, still broken for Ameba, EFR32, qpg (this last fixed in #14388).

Not sure how it got closed (did the CI close it because it was referenced in my PR?). If I did it, it certainly wasn't on purpose as at that time I was assigning it to QPG.
I'll follow up.

[bzbarsky-apple]

Not sure how it got closed (did the CI close it because it was referenced in my PR?

Your PR said "fixes #issue-number", and github itself automatically closes issues when a PR that says that is merged.

[selissia]

Not fixed. Just fixed for Linux, still broken for Ameba, EFR32, qpg (this last fixed in #14388).

@bzbarsky-apple: sorry I don't fully follow. The following is the implementation in EFR32 (Ameba's is similar). Is something still broken here? This is the fix I applied back in December.

// Store block data for HandleProcessBlock to access
CHIP_ERROR OTAImageProcessorImpl::SetBlock(ByteSpan & block)
{
    if ((block.data() == nullptr) || block.empty())
    {
        return CHIP_NO_ERROR;
    }

    // Allocate memory for block data if we don't have enough already
    if (mBlock.size() < block.size())
    {
        ReleaseBlock();

        mBlock = MutableByteSpan(static_cast<uint8_t *>(chip::Platform::MemoryAlloc(block.size())), block.size());
        if (mBlock.data() == nullptr)
        {
            return CHIP_ERROR_NO_MEMORY;
        }
    }

    // Store the actual block data
    CHIP_ERROR err = CopySpanToMutableSpan(block, mBlock);
    if (err != CHIP_NO_ERROR)
    {
        ChipLogError(SoftwareUpdate, "Cannot copy block data: %" CHIP_ERROR_FORMAT, err.Format());
        return err;
    }

    return CHIP_NO_ERROR;
}

[bzbarsky-apple]

I am not sure why I said "EFR32"... Ameba was fixed a few days ago (after my comment).

I just checked all the impls, and they look good now.