CNA-BUG-137: MediaPlayer::Play(Song*) reads and writes a destroyed Song when given a Song that the queue owns

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

Play(Song*) clears the queue before it copies and plays the given song, so passing the queue's own song, such as getQueueProperty().getActiveSongProperty(), is a heap use-after-free.

Identifier

CNA-BUG-137

Category

Bug

Subsystem

Audio & media

Status

Open

Verified against

CNA 009d40f5 (009d40f5dd085c4e674d3479675fac84b12b3e0a)

Severity

High (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

MediaPlayer::Play(Song*) given a pointer obtained from MediaQueue::getActiveSongProperty() or MediaQueue::operator[]

Expected behaviour

In XNA, MediaPlayer.Play(MediaPlayer.Queue.ActiveSong) is a valid call (restart the current song): the queue's indexer returns a new managed Song wrapper (decompiled MediaQueue), and FNA's queue holds garbage-collected objects. CNA's MediaQueue header hands out raw pointers into the queue without a lifetime caveat, and MediaPlayer::Play(Song*) is documented only as “Clears the queue, enqueues the given song, and starts playback”.

Actual behaviour at TARGET

MediaPlayer::Play(Song*) in MediaPlayer.cpp saves queue_[0], then calls queue_.Clear(). MediaQueue::Clear in MediaQueue.cpp clears a std::vector<std::unique_ptr<Song>>, destroying every queued Song. Play then calls LoadSong(song), which reads song->getHandle() and getNameProperty() to build the new copy, and PlaySong(song), which reads the handle again and, in a build with a mixer, writes setDurationProperty and setPlayCountProperty on it. When song is one of the queue's own Songs, all of these touch freed memory; the result can be a copy built from stale data, a crash, or heap corruption. Play(const SongCollection&, index) has the same shape if the collection holds pointers taken from the queue.

Source locations

• modules/media/src/Xna/MediaPlayer.cpp — MediaPlayer::Play(Song*), LoadSong, PlaySong
• modules/media/src/Xna/MediaQueue.cpp — MediaQueue::Clear destroys the owned Songs
• modules/media/include/Microsoft/Xna/Framework/Media/MediaQueue.hpp — getActiveSongProperty / operator[] return pointers into the queue; Add takes ownership
• modules/media/tests/Microsoft/Xna/Framework/Media/MediaPlayerTests.cpp — PlayEnqueuesADuplicateNotTheOriginalInstance

Evidence

Checked by reading at 009d40f5; the audit's own executed probe follows below. The ordering is unconditional: the destruction in Clear precedes the first read in LoadSong on every call, so the failure follows from the code, not from timing.

Independent re-verification: Reproduced by the audit at 009d40f5, not by CNA's own tests. A probe built from the TARGET sources of MediaPlayer, MediaQueue, Song and SongCollection with AddressSanitizer (no SOUND_ENABLED, so no mixer) called Play(&theme) and then Play(getQueueProperty().getActiveSongProperty()); ASan reported a heap-use-after-free read in MediaPlayer::LoadSong called from Play, on a Song freed by MediaQueue::Clear called from the same Play. The sharp-runtime System types came from a sibling checkout that TARGET does not pin. The writes in PlaySong (duration, play count) were not exercised because they exist only in SOUND_ENABLED builds. The C ABI cannot reach this path: cna_media_queue_get_at and cna_media_queue_get_active_song copy the queue entry out.

Focused reproduction

Illustrative; the executed probe is described under Evidence.

Song theme("Content/theme.ogg", "theme");
MediaPlayer::Play(&theme);                                   // the queue now owns a copy
MediaPlayer::Play(MediaPlayer::getQueueProperty().getActiveSongProperty());
// Clear() destroys that copy before LoadSong and PlaySong use it: heap-use-after-free under ASan

Current tests

MediaPlayerTests.cpp pins that Play enqueues a copy (PlayEnqueuesADuplicateNotTheOriginalInstance); MediaQueueTests.cpp covers the queue alone. No test passes a queue-owned song to Play.

Regression test

A MediaPlayerTest run under AddressSanitizer: Play(&song), then Play(getQueueProperty().getActiveSongProperty()); expect no sanitizer report and a queue holding one Song whose handle equals the original. The fix is to copy handle and name (or build the new queue) before clearing the old one.

Blast radius

Every audio profile, for callers that replay or restart a song taken from MediaPlayer::getQueueProperty(). The reads happen in every build; the writes only where SOUND_ENABLED is defined (SDL3 and ALSA audio). Songs owned by the caller, by ContentManager results or by MediaLibrary are not affected.

Workaround

Replay the Song object the game owns (the one originally passed to Play), never a pointer obtained from the queue.