Clean up FFmpegDemuxer for new shutdown semantics [chromium/src : main]

0 views

Skip to first unread message

Frank Liberato (Gerrit)

unread,

Mar 11, 2026, 5:00:43 PMMar 11

to Eugene Zemtsov, chromium...@chromium.org, feature-me...@chromium.org

Attention needed from Eugene Zemtsov

Frank Liberato voted and added 1 comment

Votes added by Frank Liberato

Code-Review

-1

1 comment

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Frank Liberato . resolved

my first entry to the corpus.

-fl

Open in Gerrit

Related details

Attention is currently required from:

Eugene Zemtsov

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

blocking_requirement

unsatisfied_requirement

open

diffy

Eugene Zemtsov (Gerrit)

unread,

Mar 11, 2026, 5:34:07 PMMar 11

to Eugene Zemtsov, chromium...@chromium.org, feature-me...@chromium.org

Attention needed from Frank Liberato

Eugene Zemtsov added 1 comment

File media/filters/ffmpeg_demuxer.cc

Line 873, Patchset 1 (Latest): glue_.reset();

Eugene Zemtsov . unresolved

## Review by momory_safety

- **Vulnerability:** Use-After-Free (UAF) / Concurrency Race Condition
- **Location:** `FFmpegDemuxer::~FFmpegDemuxer()` in `media/filters/ffmpeg_demuxer.cc`
- **Analysis:** 
    The `FFmpegDemuxer` manages asynchronous operations (such as `av_read_frame`, `avformat_find_stream_info`, and `av_seek_frame`) by posting tasks to a private, sequenced `blocking_task_runner_`. These tasks operate on the `AVFormatContext` (owned by `glue_`) and the `BlockingUrlProtocol` (owned by `url_protocol_`).

    The previous implementation correctly used `blocking_task_runner_->DeleteSoon(...)` to ensure that `glue_` and `url_protocol_` were only destroyed after all pending tasks on that sequenced runner had completed.

    The new code replaces this with synchronous `reset()` calls in the destructor. However, `FFmpegDemuxer::Stop()` does not synchronize with or join the `blocking_task_runner_`. It merely signals an abort to the protocol and invalidates weak pointers. If a background task is currently executing a blocking FFmpeg call (like `av_read_frame`) when the `FFmpegDemuxer` is destroyed (typically on the main thread during tab closure or navigation), the following occurs:
    1. The destructor calls `glue_.reset()`, which invokes the `FFmpegGlue` destructor.
    2. `FFmpegGlue` calls `avformat_close_input(&format_context_)`.
    3. Simultaneously, the background thread is still inside `av_read_frame` using that same `format_context_`.

    This results in a Use-After-Free or a double-free within the FFmpeg library, as `AVFormatContext` and its internal `AVIOContext` are not thread-safe for concurrent access and destruction. The "guarantee" mentioned in the commit message does not account for the internal threading model of `FFmpegDemuxer`, which is not joined during the pipeline shutdown sequence.

- **Exploitability:** High. This race condition can be reliably triggered by closing a tab or navigating away while a media file is actively demuxing or buffering, leading to a browser process crash (DoS) or potentially memory corruption.
- **Remediation:** 
    Revert the synchronous `reset()` calls and restore the use of `DeleteSoon` on the `blocking_task_runner_` to ensure proper sequencing of object destruction relative to background tasks.

```cpp
FFmpegDemuxer::~FFmpegDemuxer() {
  DCHECK(!init_cb_);
  DCHECK(!pending_seek_cb_);
  DCHECK(!weak_factory_.HasWeakPtrs());

  streams_.clear();

  // Restore asynchronous deletion to ensure background tasks finish using these objects.
  if (url_protocol_) {
    blocking_task_runner_->DeleteSoon(FROM_HERE, url_protocol_.release());
  }
  if (glue_) {
    blocking_task_runner_->DeleteSoon(FROM_HERE, glue_.release());
  }
}
```