CNA-BUG-143: VideoPlayer and Video keep raw pointers to each other, so destroying a Video before its player is stopped writes to freed memory

CNA snapshot 009d40f5  ·  Known Issues › Current bugs  ·  source links pinned to 009d40f5

VideoPlayer stores a borrowed Video* and Video a borrowed VideoPlayer* parent; CloseDecoder, run by Stop, Dispose and the destructor, writes through the stored Video*, and no header states the ordering rule.

Identifier

CNA-BUG-143

Category

Bug

Subsystem

Audio & media

Status

Open

Verified against

CNA 009d40f5 (009d40f5dd085c4e674d3479675fac84b12b3e0a)

Severity

Medium (a triage suggestion, not a project priority)

Evidence basis

Reproduced: executed for this entry (the Evidence section names exactly what was run)

Tests touching this area

Yes: see Current tests

Affected contract

VideoPlayer::Play(Video*), Stop(), Dispose(), ~VideoPlayer(), getVideoProperty(); Video::SetAudioTrackEXT and SetVideoTrackEXT

Expected behaviour

In XNA and FNA, Video and VideoPlayer are garbage-collected references, so neither order of release matters. A C++ port must either own or share the relationship, or state the ordering rule in the headers; VideoPlayer.hpp and Video.hpp state none.

Actual behaviour at TARGET

• VideoPlayer::OpenDecoder in VideoPlayer.cpp stores video_ = video and sets video->parent_ = this. CloseDecoder starts with if (video_) video_->parent_ = nullptr; and runs from Stop, Dispose and the destructor (through Dispose). Video declares no destructor, so a Video destroyed while a player still holds it leaves video_ dangling, and the next of those calls writes into freed memory; getVideoProperty() returns the dangling pointer meanwhile, and a SetVideoTrackEXT call on the player during playback reads through it (ReconfigureVideoOutputForCurrentTrack). A later Play(other) is not affected in that direction: it assigns video_ = video before OpenDecoder runs CloseDecoder. Ordinary member order is enough to trigger it: a class that declares its VideoPlayer before a Video member destroys the Video first.
• The other direction exists too. Video::SetAudioTrackEXT and SetVideoTrackEXT call through parent_ without checking that the player is alive. Video is implicitly copyable and copies keep parent_, so a copy made during playback calls into a destroyed player later. The same happens without any copy: because Play assigns video_ = video before OpenDecoder calls CloseDecoder, CloseDecoder clears parent_ on the new Video, so the Video played earlier keeps a parent_ that points at the player. Play(a), Play(b), destroy the player, then a.SetAudioTrackEXT(0) reads freed memory.

Source locations

• modules/media/src/Xna/Video/VideoPlayer.cpp — VideoPlayer::OpenDecoder, CloseDecoder, Dispose, ~VideoPlayer, Play, Stop
• modules/media/src/Xna/Video/Video.cpp — Video::SetAudioTrackEXT / SetVideoTrackEXT call through parent_
• modules/media/include/Microsoft/Xna/Framework/Media/Video/Video.hpp — parent_ member; no destructor or copy control
• modules/media/include/Microsoft/Xna/Framework/Media/Video/VideoPlayer.hpp — Play(Video*) documentation without a lifetime rule

Evidence

Checked by reading at 009d40f5; the audit's own executed probe follows below. The write in CloseDecoder is unconditional whenever video_ is set, so the failure follows from destruction order alone.

Independent re-verification: Reproduced by the audit at 009d40f5, not by CNA's own tests. The TARGET VideoPlayer.cpp and Video.cpp were compiled with AddressSanitizer against the real FFmpeg VideoDecoder.cpp and a TARGET fixture clip (Video built with no GraphicsDevice, no SOUND_ENABLED). Deleting a playing Video and then calling Stop() gave a heap-use-after-free WRITE in VideoPlayer::CloseDecoder. Copying a playing Video, destroying the player and calling SetAudioTrackEXT on the copy gave a heap-use-after-free READ in VideoPlayer::SetAudioTrackEXT, and so did Play(a), Play(b), destroying the player and calling a.SetAudioTrackEXT with no copy at all. The sharp-runtime types came from a sibling checkout that TARGET does not pin.

Focused reproduction

Illustrative; the audit's executed probe is described under Evidence.

struct Cutscene {
    VideoPlayer player;              // destroyed second
    std::optional<Video> video;     // destroyed first
};
{
    Cutscene c;
    c.video.emplace("Content/intro.mkv", &graphicsDevice);
    c.player.Play(&*c.video);
}   // ~Video, then ~VideoPlayer -> Dispose -> CloseDecoder writes into the destroyed Video

Current tests

VideoPlayerTests.cpp covers playback, looping, track selection and disposal with the Video outliving the player; no test destroys or copies a Video while its player holds it.

Regression test

Under AddressSanitizer: play a Video, destroy it, then Stop() and destroy the player; and copy a playing Video, destroy the player, then call SetAudioTrackEXT on the copy. Both must be clean, and so must Play(a), Play(b), destroying the player and then calling SetAudioTrackEXT on a, which needs no copy. A fix can detach in ~Video, make the link a weak handle, or define copy semantics that drop parent_.

Blast radius

Every build with the video backend where a Video can die before its player: member order, containers that reallocate, cut-scene objects released in UnloadContent. Builds without FFmpeg cannot start playback, so the link is never set there.

Workaround

Stop or dispose the player before the Video goes, declare the Video before the player, do not copy a Video that is playing, and destroy the player before any Video it has played.