Skip to content

Fixes potential race in teardown/restart flow observed under trickplay#1302

Merged
pstroffolino merged 12 commits into
dev_sprint_25_2from
VPLAY-11374_address_trickplay_crash
Apr 25, 2026
Merged

Fixes potential race in teardown/restart flow observed under trickplay#1302
pstroffolino merged 12 commits into
dev_sprint_25_2from
VPLAY-11374_address_trickplay_crash

Conversation

@p-bond
Copy link
Copy Markdown
Contributor

@p-bond p-bond commented Apr 13, 2026

Fix race condition in ProfileEventAAMP::TuneBegin and move mLldLowBuffObject null assignment before cJSON_Delete in both TuneBegin and GetTelemetryParam to prevent use-after-free (addresses double free in ticket)

Add discontinuityParamMutex lock_guard in TuneBegin to protect telemetryParam and mLldLowBuffObject from concurrent access

Move mLldLowBuffObject = NULL to before cJSON_Delete(telemetryParam) in both methods, ensuring the pointer is cleared before the underlying cJSON node it points to is freed

@p-bond p-bond marked this pull request as ready for review April 20, 2026 14:48
@p-bond p-bond requested a review from a team as a code owner April 20, 2026 14:48
Comment thread AampProfiler.cpp Outdated
Comment thread AampProfiler.cpp Outdated
Comment thread AampProfiler.cpp
Comment thread AampProfiler.cpp Outdated
Comment thread AampProfiler.cpp
@nebutler963
Copy link
Copy Markdown
Contributor

Looks Good

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the AAMP profiler’s telemetry reset path to avoid races/use-after-free during teardown/restart (notably under trickplay) by synchronizing access to the telemetry cJSON tree and clearing a cached child pointer before freeing the parent.

Changes:

  • Add discontinuityParamMutex protection around telemetryParam reset in ProfileEventAAMP::TuneBegin().
  • Clear mLldLowBuffObject before deleting telemetryParam in both TuneBegin() and GetTelemetryParam() to avoid use-after-free/double-free patterns.

Comment thread AampProfiler.cpp Outdated
Comment thread AampProfiler.cpp Outdated
Comment thread AampProfiler.cpp Outdated
Comment thread AampProfiler.cpp Outdated
Comment thread AampProfiler.cpp
@p-bond
Copy link
Copy Markdown
Contributor Author

p-bond commented Apr 24, 2026

The code changes have gone through Copilot as a minimal code change.
The suggested review comments introduce a lot more potential for issues.

pstroffolino and others added 4 commits April 24, 2026 15:13
Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
…/GetTelemetryParam

- Guard against cJSON_CreateObject() returning NULL (OOM): keep existing
  telemetryParam intact and log an error rather than leaving it null and
  crashing callers like SetLatencyParam that dereference it without a guard.
- Move cJSON_Delete() and cJSON_PrintUnformatted() outside the
  discontinuityParamMutex critical section: swap the pointer under the lock,
  then free/log the old tree after release to reduce lock contention.

Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
@pstroffolino pstroffolino merged commit 67c0606 into dev_sprint_25_2 Apr 25, 2026
10 checks passed
@pstroffolino pstroffolino deleted the VPLAY-11374_address_trickplay_crash branch April 25, 2026 00:04
Abhi-jith-S pushed a commit that referenced this pull request May 12, 2026
…1305) on linear channel on performing multiple times trickplay/Seek" (#1302)

Reason for Change: Fixes potential race in teardown/restart flow observed under trickplay
* Move cJSON_CreateObject outside mutex
* Reason for Change: NULL -> nullptr
* AampProfiler: OOM guard + narrow mutex critical sections in TuneBegin/GetTelemetryParam
- Guard against cJSON_CreateObject() returning NULL (OOM): keep existing
  telemetryParam intact and log an error rather than leaving it null and
  crashing callers like SetLatencyParam that dereference it without a guard.
- Move cJSON_Delete() and cJSON_PrintUnformatted() outside the
  discontinuityParamMutex critical section: swap the pointer under the lock,
  then free/log the old tree after release to reduce lock contention.

---------

Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
varatharajan568 pushed a commit that referenced this pull request May 20, 2026
…1305) on linear channel on performing multiple times trickplay/Seek" (#1302)

Reason for Change: Fixes potential race in teardown/restart flow observed under trickplay
* Move cJSON_CreateObject outside mutex
* Reason for Change: NULL -> nullptr
* AampProfiler: OOM guard + narrow mutex critical sections in TuneBegin/GetTelemetryParam
- Guard against cJSON_CreateObject() returning NULL (OOM): keep existing
  telemetryParam intact and log an error rather than leaving it null and
  crashing callers like SetLatencyParam that dereference it without a guard.
- Move cJSON_Delete() and cJSON_PrintUnformatted() outside the
  discontinuityParamMutex critical section: swap the pointer under the lock,
  then free/log the old tree after release to reduce lock contention.

---------

Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants