Fix deep/shallow copy issue in Joint Fabric#43208
Fix deep/shallow copy issue in Joint Fabric#43208vijs wants to merge 1 commit intoproject-chip:masterfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a critical shallow copy issue within the Joint Fabric datastore by introducing dedicated storage for data backing CharSpan and ByteSpan objects. This prevents use-after-free errors when command payloads are deallocated. The changes are consistently applied across various data structures like GroupKeySet, Admin, and GroupInformation, ensuring that their lifecycles are managed correctly. The addition of helper functions for deep copying and storage management centralizes this logic, improving maintainability. My review includes a couple of suggestions to further simplify the code and remove redundancy.
| storage.friendlyName.resize(friendlyName.size()); | ||
| if (!storage.friendlyName.empty()) | ||
| { | ||
| memcpy(storage.friendlyName.data(), friendlyName.data(), storage.friendlyName.size()); | ||
| destination.friendlyName = CharSpan(storage.friendlyName.data(), storage.friendlyName.size()); | ||
| } | ||
| else | ||
| { | ||
| destination.friendlyName = CharSpan(); | ||
| } | ||
|
|
||
| storage.icac.resize(icac.size()); | ||
| if (!storage.icac.empty()) | ||
| { | ||
| memcpy(storage.icac.data(), icac.data(), storage.icac.size()); | ||
| destination.icac = ByteSpan(storage.icac.data(), storage.icac.size()); | ||
| } | ||
| else | ||
| { | ||
| destination.icac = ByteSpan(); | ||
| } |
There was a problem hiding this comment.
For consistency with CopyGroupKeySetWithOwnedSpans and SetGroupInformationFriendlyNameWithOwnedStorage, consider using std::vector::assign instead of resize and memcpy. This would make the code more concise and idiomatic.
storage.friendlyName.assign(friendlyName.data(), friendlyName.data() + friendlyName.size());
destination.friendlyName = CharSpan(storage.friendlyName.data(), storage.friendlyName.size());
storage.icac.assign(icac.data(), icac.data() + icac.size());
destination.icac = ByteSpan(storage.icac.data(), storage.icac.size());| // Also drop storage for entries no longer present in the datastore list. | ||
| for (auto storageIt = mGroupKeySetStorage.begin(); storageIt != mGroupKeySetStorage.end();) | ||
| { | ||
| const bool inDatastore = | ||
| std::any_of(mGroupKeySetList.begin(), mGroupKeySetList.end(), | ||
| [&](const auto & entry) { return entry.groupKeySetID == storageIt->first; }); | ||
| if (!inDatastore) | ||
| { | ||
| storageIt = mGroupKeySetStorage.erase(storageIt); | ||
| } | ||
| else | ||
| { | ||
| ++storageIt; | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop appears to be redundant. The preceding loop (lines 687-701) iterates through mGroupKeySetList and removes entries that are not present on the remote node, while also calling RemoveGroupKeySetStorage to clean up the backing storage for each removed entry. Since other functions that modify mGroupKeySetList (e.g., RemoveGroupKeySetEntry) also correctly manage the backing storage, this additional cleanup loop seems unnecessary. Removing it would simplify the code.
|
PR #43208: Size comparison from c1553ce to dbb287f Full report (34 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
| { | ||
| storage.epochKey2.clear(); | ||
| destination.epochKey2.SetNull(); | ||
| } |
There was a problem hiding this comment.
The if else from lines 36 to 70 all perform the same operations just on different values. These could be made into separate function passing the difference keys as parameters. Something like:
bool epochKeyCopy(storage, source.key, destination.key) {
if(source.key.IsNul()){
...
return true;
} else {
...
return false;
}
The true and false would indicate if storage was made clear or not. Or add other checks.
This would also reduce the above 34 lines into 14 lines total making it easier to read and modify at later times.
| { | ||
| destination.epochStartTime2.SetNull(); | ||
| } | ||
|
|
There was a problem hiding this comment.
In similar comment to the one above about keys, lines 72-98 here can all be reduced to a single function for code reuse and reduction of lines of code.
2 participants
Summary
Fix deep/shallow copy issue in Joint Fabric
Related issues
Addresses Issue: #43174.
Testing
Build asan-instrumented Apps
Run Test (JFDS_2_2.py example)