Skip to content

ICD: Checking counter check moved from CheckInHandler to CheckinMessage.#71643

Draft
rcasallas-silabs wants to merge 1 commit intoproject-chip:masterfrom
rcasallas-silabs:icd/checkin_counter_check
Draft

ICD: Checking counter check moved from CheckInHandler to CheckinMessage.#71643
rcasallas-silabs wants to merge 1 commit intoproject-chip:masterfrom
rcasallas-silabs:icd/checkin_counter_check

Conversation

@rcasallas-silabs
Copy link
Copy Markdown
Contributor

Summary

ICD: Checking counter check moved from CheckInHandler to CheckinMessage.

Related issues

MATTER-2246.

Testing

UNTESTED.

Readability checklist

The checklist below will help the reviewer finish PR review in time and keep the
code readable:

  • PR title is
    descriptive
  • Apply the
    “When in Rome…”
    rule (coding style)
  • PR size is short
  • Try to avoid "squashing" and "force-update" in commit history
  • CI time didn't increase

See: Pull Request Guidelines

@github-actions github-actions bot added app tests icd Intermittently Connected Devices protocols labels Apr 17, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the ICD check-in message processing by moving duplicate message detection logic from CheckInHandler into CheckinMessage::ParseCheckinMessagePayload. The ProcessCheckInPayload interface has been updated to handle counter offsets directly. A significant issue was identified in DefaultICDClientStorage::ProcessCheckInPayload where the lastErr variable is initialized but never updated within the loop, which prevents the function from returning specific error codes such as CHIP_ERROR_DUPLICATE_MESSAGE_RECEIVED when a duplicate message is correctly identified during decryption.

Comment on lines +529 to +544
CHIP_ERROR lastErr = CHIP_ERROR_NOT_FOUND;

while (iterator->Next(clientInfo))
{
CHIP_ERROR err = chip::Protocols::SecureChannel::CheckinMessage::ParseCheckinMessagePayload(
clientInfo.aes_key_handle, clientInfo.hmac_key_handle, payload, counter, appData);
clientInfo.aes_key_handle, clientInfo.hmac_key_handle, clientInfo.start_icd_counter, clientInfo.offset, payload, offset,
appData);
if (CHIP_NO_ERROR == err)
{
clientInfo.offset = offset;
iterator->Release();
return CHIP_NO_ERROR;
}
}
iterator->Release();
return CHIP_ERROR_NOT_FOUND;
return lastErr;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The lastErr variable is initialized to CHIP_ERROR_NOT_FOUND but is never updated within the loop. This results in the function always returning CHIP_ERROR_NOT_FOUND if no successful match is found, masking more specific errors like CHIP_ERROR_DUPLICATE_MESSAGE_RECEIVED.

Since check-in message keys are unique per client, if ParseCheckinMessagePayload returns CHIP_ERROR_DUPLICATE_MESSAGE_RECEIVED, it confirms that the correct client was found (as decryption succeeded) but the message is a duplicate. The loop should terminate and return this specific error to allow the caller to log or handle the duplicate correctly.

    CHIP_ERROR lastErr = CHIP_ERROR_NOT_FOUND;

    while (iterator->Next(clientInfo))
    {
        CHIP_ERROR err = chip::Protocols::SecureChannel::CheckinMessage::ParseCheckinMessagePayload(
            clientInfo.aes_key_handle, clientInfo.hmac_key_handle, clientInfo.start_icd_counter, clientInfo.offset, payload, offset,
            appData);
        if (CHIP_NO_ERROR == err)
        {
            clientInfo.offset = offset;
            iterator->Release();
            return CHIP_NO_ERROR;
        }

        if (err == CHIP_ERROR_DUPLICATE_MESSAGE_RECEIVED)
        {
            lastErr = err;
            break;
        }
    }
    iterator->Release();
    return lastErr;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

app icd Intermittently Connected Devices protocols tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant