Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions src/app/clusters/door-lock-server/door-lock-server.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@
#include <platform/CHIPDeviceConfig.h>
#include <protocols/interaction_model/StatusCode.h>

#include <algorithm>
#include <cstring>

#ifndef DOOR_LOCK_SERVER_ENDPOINT
#define DOOR_LOCK_SERVER_ENDPOINT 1
#endif
Expand Down Expand Up @@ -785,6 +788,26 @@ struct EmberAfPluginDoorLockCredentialInfo
#if DOOR_LOCK_USE_LOCAL_BUFFER
uint8_t credentialDataBuffer[DOOR_LOCK_CREDENTIAL_BUFFER_LENGTH];
EmberAfPluginDoorLockCredentialInfo() { credentialData = chip::MutableByteSpan(credentialDataBuffer); }

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.

can you please add a unit test for this change?

@Alami-Amine Alami-Amine Jun 26, 2026

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.

hmm, a unit test here wouldn't run in CI anyways since DOOR_LOCK_USE_LOCAL_BUFFER is never 1 in our CI

// credentialData is a span into credentialDataBuffer, so the implicitly-defined copy would leave a
// copy's span pointing into the source's buffer (dangling once the source is gone). Deep-copy the
// bytes and re-bind the span to this object's own buffer.
EmberAfPluginDoorLockCredentialInfo(const EmberAfPluginDoorLockCredentialInfo & other) { *this = other; }

EmberAfPluginDoorLockCredentialInfo & operator=(const EmberAfPluginDoorLockCredentialInfo & other)
{
status = other.status;
credentialType = other.credentialType;
creationSource = other.creationSource;
createdBy = other.createdBy;
modificationSource = other.modificationSource;
lastModifiedBy = other.lastModifiedBy;

size_t dataLen = std::min(other.credentialData.size(), sizeof(credentialDataBuffer));
memcpy(credentialDataBuffer, other.credentialData.data(), dataLen);
credentialData = chip::MutableByteSpan(credentialDataBuffer, dataLen);
return *this;
}
Comment on lines +797 to +810

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

There are two potential issues here:

  1. Self-assignment: If an object is assigned to itself (e.g., info = info;), other.credentialData.data() will point to credentialDataBuffer. Passing overlapping memory regions to std::memcpy is undefined behavior.
  2. Null pointer in std::memcpy: If other.credentialData is empty, other.credentialData.data() can be nullptr. Calling std::memcpy with a null pointer as the source is undefined behavior, even if the size (dataLen) is 0.

Adding a self-assignment check and guarding the std::memcpy call with if (dataLen > 0) resolves both issues.

    EmberAfPluginDoorLockCredentialInfo & operator=(const EmberAfPluginDoorLockCredentialInfo & other)
    {
        if (this == &other)
        {
            return *this;
        }

        status             = other.status;
        credentialType     = other.credentialType;
        creationSource     = other.creationSource;
        createdBy          = other.createdBy;
        modificationSource = other.modificationSource;
        lastModifiedBy     = other.lastModifiedBy;

        size_t dataLen = std::min(other.credentialData.size(), sizeof(credentialDataBuffer));
        if (dataLen > 0)
        {
            memcpy(credentialDataBuffer, other.credentialData.data(), dataLen);
        }
        credentialData = chip::MutableByteSpan(credentialDataBuffer, dataLen);
        return *this;
    }

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.

please integrate Gemini's guard; its good defensive hardening

#endif
};

Expand Down Expand Up @@ -818,6 +841,35 @@ struct EmberAfPluginDoorLockUserInfo
userName = chip::MutableCharSpan(nameBuffer);
credentials = chip::Span<CredentialStruct>(credentialsBuffer);
}

// userName and credentials are spans into nameBuffer/credentialsBuffer, so the implicitly-defined
// copy would leave a copy's spans pointing into the source's buffers (dangling once the source is
// gone). Deep-copy the contents and re-bind the spans to this object's own buffers.
EmberAfPluginDoorLockUserInfo(const EmberAfPluginDoorLockUserInfo & other) { *this = other; }

EmberAfPluginDoorLockUserInfo & operator=(const EmberAfPluginDoorLockUserInfo & other)
{
userUniqueId = other.userUniqueId;
userStatus = other.userStatus;
userType = other.userType;
credentialRule = other.credentialRule;
creationSource = other.creationSource;
createdBy = other.createdBy;
modificationSource = other.modificationSource;
lastModifiedBy = other.lastModifiedBy;

size_t nameLen = std::min(other.userName.size(), sizeof(nameBuffer));
memcpy(nameBuffer, other.userName.data(), nameLen);
userName = chip::MutableCharSpan(nameBuffer, nameLen);

size_t credentialCount = std::min(other.credentials.size(), sizeof(credentialsBuffer) / sizeof(credentialsBuffer[0]));
for (size_t i = 0; i < credentialCount; i++)
{
credentialsBuffer[i] = other.credentials[i];
}
credentials = chip::Span<CredentialStruct>(credentialsBuffer, credentialCount);
return *this;
}
Comment on lines +850 to +872

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

There are two potential issues here:

  1. Self-assignment: If an object is assigned to itself, other.userName.data() will point to nameBuffer. Passing overlapping memory regions to std::memcpy is undefined behavior.
  2. Null pointer in std::memcpy: If other.userName is empty, other.userName.data() can be nullptr. Calling std::memcpy with a null pointer as the source is undefined behavior, even if the size (nameLen) is 0.

Adding a self-assignment check and guarding the std::memcpy call with if (nameLen > 0) resolves both issues.

    EmberAfPluginDoorLockUserInfo & operator=(const EmberAfPluginDoorLockUserInfo & other)
    {
        if (this == &other)
        {
            return *this;
        }

        userUniqueId       = other.userUniqueId;
        userStatus         = other.userStatus;
        userType           = other.userType;
        credentialRule     = other.credentialRule;
        creationSource     = other.creationSource;
        createdBy          = other.createdBy;
        modificationSource = other.modificationSource;
        lastModifiedBy     = other.lastModifiedBy;

        size_t nameLen = std::min(other.userName.size(), sizeof(nameBuffer));
        if (nameLen > 0)
        {
            memcpy(nameBuffer, other.userName.data(), nameLen);
        }
        userName = chip::MutableCharSpan(nameBuffer, nameLen);

        size_t credentialCount = std::min(other.credentials.size(), sizeof(credentialsBuffer) / sizeof(credentialsBuffer[0]));
        for (size_t i = 0; i < credentialCount; i++)
        {
            credentialsBuffer[i] = other.credentials[i];
        }
        credentials = chip::Span<CredentialStruct>(credentialsBuffer, credentialCount);
        return *this;
    }

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.

please integrate Gemini's guard; its good defensive hardening

#endif
};

Expand Down
Loading