IN-DEV: Synchronize access to CastingApp/CastingPlayer to avoid race conditions#43082
IN-DEV: Synchronize access to CastingApp/CastingPlayer to avoid race conditions#43082chrisdecenzo 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 aims to address potential race conditions and synchronization issues by adding state checks and synchronized blocks in CastingApp and MatterCastingPlayer. While the PR introduces valuable improvements for thread safety, two significant security issues were identified: a race condition in the singleton initialization of CastingApp on Android, and a potential Use-After-Free vulnerability in the JNI layer due to improper management of unique_ptr lifetimes when handling IdentificationDeclarationOptions. Additionally, the changes in CASESessionManager.cpp have introduced some redundant checks in private helper functions. Overall, these changes improve the stability and safety of the casting application, but the identified security vulnerabilities require further attention.
| matter::casting::core::IdentificationDeclarationOptions idOptions; | ||
| if (jIdentificationDeclarationOptions != nullptr) | ||
| { | ||
| ChipLogProgress(AppServer, | ||
| "MatterCastingPlayer-JNI::sendUDC() Optional jIdentificationDeclarationOptions not " | ||
| "provided by the client"); | ||
| ChipLogProgress( | ||
| AppServer, | ||
| "MatterCastingPlayer-JNI::sendUDCNative() Optional jIdentificationDeclarationOptions was provided by client"); | ||
| std::unique_ptr<matter::casting::core::IdentificationDeclarationOptions> idOptionsCpp( | ||
| support::convertIdentificationDeclarationOptionsFromJavaToCpp(jIdentificationDeclarationOptions)); | ||
| if (idOptionsCpp == nullptr) | ||
| { | ||
| ChipLogError(AppServer, | ||
| "MatterCastingPlayer-JNI::sendUDCNative() " | ||
| "convertIdentificationDeclarationOptionsFromJavaToCpp() error"); | ||
| return support::convertMatterErrorFromCppToJava(CHIP_ERROR_INVALID_ARGUMENT); | ||
| } | ||
| idOptions = *idOptionsCpp; | ||
| idOptions.LogDetail(); | ||
| } |
There was a problem hiding this comment.
A potential Use-After-Free (UAF) vulnerability exists here. A unique_ptr named idOptionsCpp is created within the local scope of the if block (lines 81-97). The object it points to is copied to the stack variable idOptions via idOptions = *idOptionsCpp; at line 95. In the Matter SDK, these options structs frequently use non-owning pointers (like chip::CharSpan) to refer to internal data. When idOptionsCpp goes out of scope at line 97, it destroys the original object and its owned data, leaving any spans in the idOptions copy dangling. Subsequent use of idOptions at line 126 results in a Use-After-Free.
This same pattern is repeated in verifyOrEstablishConnectionNative at lines 170-187. To remediate this, extend the lifetime of the unique_ptr to cover the call to the native Matter functions, or avoid the copy by using a pointer that points to either a default object or the object managed by the unique_ptr.
References
- When a stack-allocated object is passed by reference to a function that stores the reference, ensure the object's lifetime is static to prevent dangling references and undefined behavior.
| Log.e( | ||
| TAG, | ||
| "CastingApp.initialize() failed to updateCommissionableDataProviderData() on AndroidChipPlatform"); | ||
| synchronized (mStateLock) { |
There was a problem hiding this comment.
The getInstance() method (lines 55-60) is not thread-safe, which allows multiple instances of the CastingApp singleton to be created if accessed concurrently. While this PR adds synchronization to instance methods like initialize() using an instance-level lock (mStateLock), this synchronization is ineffective if multiple instances of CastingApp exist, as each instance will have its own lock. This can lead to multiple initializations of the global Matter stack and inconsistent application state.
To fix this, implement a thread-safe singleton pattern in getInstance(), such as using a synchronized block or the 'initialization-on-demand holder' idiom.
| if (mConfig.sessionSetupPool == nullptr) | ||
| { | ||
| ChipLogError(CASESessionManager, "FindExistingSessionSetup called before CASESessionManager is initialized"); | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
This null check for mConfig.sessionSetupPool is redundant. FindExistingSessionSetup is a private method, and all its callers (FindOrEstablishSessionHelper, UpdatePeerAddress, and ReleaseSession(const ScopedNodeId&) in this file) already perform the same null check before calling it.
To improve maintainability and reduce code duplication, you could remove this check and rely on the checks in the calling functions.
A similar pattern of redundant checks has been introduced for FindExistingSession and its caller GetPeerAddress.
References
- To improve maintainability and reduce code duplication, extract common logic from similar methods into private helper functions.
|
PR #43082: Size comparison from feb7cc3 to 36061fd Full report (12 builds for cc13x4_26x4, cc32xx, nrfconnect, qpg, realtek, stm32)
|
chrisdecenzo
changed the title
1 participant
Summary
Avoid synchronization issues when Server.init is slow.
When running multiple instances of the CastingApp on the phone (in different apps), synchronization issues have been observed
Testing
iOS and Android phone. More TBD