VPAAMP-387 missing null pointer guards for mMediaStreamContext[eMEDIATYPE_VIDEO] and mMediaStreamContext[eMEDIATYPE_AUDIO]#1487
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR targets VPAAMP-387 by adding null-pointer guards in StreamAbstractionAAMP_MPD::StreamSelection() to prevent potential crashes when accessing mMediaStreamContext[eMEDIATYPE_VIDEO] / mMediaStreamContext[eMEDIATYPE_AUDIO].
Changes:
- Added a null check for video/audio
mMediaStreamContextentries before the “audio-only period” handling block. - Added an error log when a null track context is detected.
…TYPE_VIDEO] and mMediaStreamContext[eMEDIATYPE_AUDIO] Reason for Change: static code analysis noted some missing null pointed guards - appears to be longstanding issue Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
5fb7677 to
eb88997
Compare
Null-check at the audio-only block only logged but did not prevent subsequent dereferences of mMediaStreamContext[eMEDIATYPE_AUDIO] and [eMEDIATYPE_VIDEO] (e.g. SetCurrentBandWidth, audio-only copy block). Add early return so all downstream uses are skipped when either context is null.
eb88997 to
2ab3d58
Compare
…ion test Adds StreamSelection_TrickPlay_NullAudioContext_DoesNotCrash to StreamSelectionTests. The test initializes at trick-play rate (4x) so that mMaxTracks==1 and mMediaStreamContext[eMEDIATYPE_AUDIO] remains null, then calls InvokeStreamSelection() under EXPECT_NO_FATAL_FAILURE to confirm the null guard on the audio-only copy block and the targeted null guard in the SetCurrentBandWidth loop both hold.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
test/utests/tests/StreamAbstractionAAMP_MPD/StreamSelectionTest.cpp:825
EXPECT_NO_FATAL_FAILUREdoes not protect against process crashes/segfaults, so it doesn’t actually validate the stated “DoesNotCrash” intent (a null dereference would still abort the whole test binary). Consider callingInvokeStreamSelection()directly (a crash will fail the test run) and/or adjust the assertion/comment to reflect what is actually being checked (no gtest fatal failures).
// StreamSelection() must complete without dereferencing the null audio
// context. Both guard sites (audio-only copy block and SetCurrentBandWidth
// loop) are reached in this path.
EXPECT_NO_FATAL_FAILURE(mStreamAbstractionAAMP_MPD->InvokeStreamSelection());
fragmentcollector_mpd.cpp:7326
AAMPLOG_ERR("... null track context in audio-only check")doesn’t indicate which context is null (audio vs video) or include useful state (e.g., play rate / mMaxTracks / mNumberOfTracks), which makes triage harder. Consider expanding the message to include which pointer(s) are null and relevant track/rate context.
if (mPlayRate == AAMP_NORMAL_PLAY_RATE)
{
AAMPLOG_ERR("StreamAbstractionAAMP_MPD: null track context in audio-only check");
}
The null dereferences of mMediaStreamContext[eMEDIATYPE_AUDIO] at the audio-only copy block and the SetCurrentBandWidth call are logically unreachable at trick-play rate: - The audio-only copy block fires when mNumberOfTracks==1 and mMediaStreamContext[eMEDIATYPE_VIDEO]->enabled==false. At trick-play rate the only code path that sets mNumberOfTracks=1 is the iframe-track branch, which also unconditionally sets enabled=true, making !enabled false. The two conditions are mutually exclusive. - The SetCurrentBandWidth call is inside the aTracks loop. SelectAudioTrack is only called at AAMP_NORMAL_PLAY_RATE, so aTracks is always empty at trick-play rate and the loop body is never entered. At normal play rate all contexts are allocated (mMaxTracks==AAMP_TRACK_COUNT), so neither dereference is unsafe there either. Replace the runtime guards and the regression test with comments at each site that document the invariant and the reasoning, so reviewers and future authors understand why the code is safe without dead guard code.
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
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.
Reason for Change: static code analysis noted some missing null pointer guards - appears to be longstanding issue
Test Guidance: will add new test to force the path that otherwise would have caused crash
Risk: Low