fix dangling spans when copying door lock user/credential info#72739
Open
kali834x wants to merge 1 commit into
Open
fix dangling spans when copying door lock user/credential info#72739kali834x wants to merge 1 commit into
kali834x wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request adds custom copy constructors and assignment operators to EmberAfPluginDoorLockCredentialInfo and EmberAfPluginDoorLockUserInfo to perform deep copies of internal buffers and re-bind spans, preventing dangling references. The review feedback highlights potential issues with self-assignment and passing null pointers to std::memcpy when the source spans are empty, which can lead to undefined behavior. It is recommended to add self-assignment checks and guard the std::memcpy calls.
Comment on lines
+797
to
+810
| 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; | ||
| } |
Contributor
There was a problem hiding this comment.
There are two potential issues here:
- Self-assignment: If an object is assigned to itself (e.g.,
info = info;),other.credentialData.data()will point tocredentialDataBuffer. Passing overlapping memory regions tostd::memcpyis undefined behavior. - Null pointer in
std::memcpy: Ifother.credentialDatais empty,other.credentialData.data()can benullptr. Callingstd::memcpywith 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;
}
Contributor
There was a problem hiding this comment.
please integrate Gemini's guard; its good defensive hardening
Comment on lines
+850
to
+872
| 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; | ||
| } |
Contributor
There was a problem hiding this comment.
There are two potential issues here:
- Self-assignment: If an object is assigned to itself,
other.userName.data()will point tonameBuffer. Passing overlapping memory regions tostd::memcpyis undefined behavior. - Null pointer in
std::memcpy: Ifother.userNameis empty,other.userName.data()can benullptr. Callingstd::memcpywith 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;
}
Contributor
There was a problem hiding this comment.
please integrate Gemini's guard; its good defensive hardening
|
PR #72739: Size comparison from e2077a4 to 5b0ac22 Full report (33 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #72739 +/- ##
=======================================
Coverage 56.79% 56.79%
=======================================
Files 1642 1642
Lines 112757 112757
Branches 13139 13139
=======================================
Hits 64040 64040
Misses 48717 48717 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Alami-Amine
reviewed
Jun 25, 2026
| #if DOOR_LOCK_USE_LOCAL_BUFFER | ||
| uint8_t credentialDataBuffer[DOOR_LOCK_CREDENTIAL_BUFFER_LENGTH]; | ||
| EmberAfPluginDoorLockCredentialInfo() { credentialData = chip::MutableByteSpan(credentialDataBuffer); } | ||
|
|
Contributor
There was a problem hiding this comment.
can you please add a unit test for this change?
Contributor
There was a problem hiding this comment.
hmm, a unit test here wouldn't run in CI anyways since DOOR_LOCK_USE_LOCAL_BUFFER is never 1 in our CI
Contributor
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
In
DOOR_LOCK_USE_LOCAL_BUFFERbuilds,EmberAfPluginDoorLockUserInfoandEmberAfPluginDoorLockCredentialInfobind theiruserName/credentialsandcredentialDataspans to their own internalnameBuffer/credentialsBuffer/credentialDataBufferin the default constructor, but neither struct declares copy control. The compiler-generated copy then copies the span members verbatim, so a copy's spans keep pointing into the source object's buffers.door-lock-server.cppcopies these structs by value:emberAfPluginDoorLockGetUser/emberAfPluginDoorLockGetCredentialfill a local struct, andfindUserIndexByCredentialdoesuserInfo = userbefore returning. Once the source goes out of scope the copy's spans dangle, so readinguserName/credentials/credentialDataafterwards is a use-after-free.The fix gives both structs a copy constructor and copy-assignment operator, guarded by the same
DOOR_LOCK_USE_LOCAL_BUFFER, that copy the buffer contents and re-point the spans at the destination's own buffers. This is the same dangling-span-on-copy problem that was fixed for the Push-AVTransportOptionsStoragein #72530. The defaultDOOR_LOCK_USE_LOCAL_BUFFER=0build is unchanged, since the spans there reference application-owned storage.Related issues
None.
Testing
Built a minimal reproducer mirroring the struct layout (fixed buffer plus a span bound to it in the default ctor) and the
userInfo = usercopy. Under AddressSanitizer the unpatched shape reports a use-after-scope read when the copied span is dereferenced after the source is destroyed (heap-use-after-free for heap-allocated infos); with the copy/assignment operators added it runs clean and the spans resolve to the copy's own buffers. The operators were also compiled in isolation with-Wall -WextraunderDOOR_LOCK_USE_LOCAL_BUFFER=1. The defaultDOOR_LOCK_USE_LOCAL_BUFFER=0build is unaffected because the new code sits inside that#if.