Fix use-after-free crash in PushAV stream transport during recorder recreation#71470
Draft
raveendra-karu wants to merge 1 commit intoproject-chip:masterfrom
Draft
Conversation
…ecreation - Added validation check in PushAVClipRecorder destructor to prevent accessing freed mUploadFileBasePath - Added small delays after mRecorder.reset() calls to ensure proper cleanup before recreation - Fixed race condition in CanSendPacketsToRecorder(), CheckAndUpdateSession(), and ModifyPushTransport() functions - Addresses crash in TC_PAVST_2_13.py test execution
![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 introduces a check for an empty file path in the PushAVClipRecorder destructor and adds 10ms delays during recorder resets in PushAVTransport to facilitate cleanup. Review feedback correctly identifies that the added path check is ineffective at preventing use-after-free or data races, as accessing a member variable of a deleted or concurrently modified object is undefined behavior or a race condition regardless of the check.
Comment on lines
+105
to
+107
| // Check if mUploadFileBasePath is valid and not empty before accessing it | ||
| // This prevents use-after-free issues when the path might be concurrently accessed | ||
| if (!mUploadFileBasePath.empty()) |
Contributor
There was a problem hiding this comment.
The check !mUploadFileBasePath.empty() does not prevent use-after-free vulnerabilities. If the PushAVClipRecorder instance is being accessed after it has been deleted, calling empty() on a member variable is already undefined behavior. Furthermore, if the concern is concurrent access, a simple check without a mutex is a data race. Since mWorkerThread.join() is called at line 102, any concurrency from the worker thread is already resolved. If other threads are still accessing this object, the lifecycle management of the recorder needs to be corrected at the caller level rather than adding checks inside the destructor.
|
PR #71470: Size comparison from fdb31db to 61fc2d7 Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #71470 +/- ##
=======================================
Coverage 54.31% 54.31%
=======================================
Files 1576 1576
Lines 108247 108247
Branches 13401 13401
=======================================
Hits 58796 58796
Misses 49451 49451 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
1 participant
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
Related issues
Fixes #71405
Testing