Fix use after free in producer_avformat's cache-miss path by unicorntearsproject · Pull Request #1330 · mltframework/mlt

unicorntearsproject · 2026-10-01T01:13:25Z

In producer_get_frame(), on a cache miss the state is created, mlt_service_cache_put() into the cache, then fetched again to take a reference. The cache holds only 4 avformat states by default, so with more producers decoding on several threads another thread's put can evict the new entry in between. Nobody holds a reference yet, so its destructor runs and the state is freed, but the code carried on with the pointer it had put, and stored a NULL cache item on the frame. This now uses the state the cache returns (with our reference) and creates it again if it has already gone.

My description in the issue (that an entry in use is evicted) was imprecise: held entries are protected by their reference count. The window is only between the put and the get.

Tested on master 89fbe37: repro (16 threads, each with its own avformat producer, 150 frames of video and audio, default cache size): 30 of 30 runs crashed in producer_set_up_video() before, 0 of 30 after. clang-format clean.

🤖 Generated with Claude Code

producer_get_frame() creates the producer's state on a cache miss, puts it in the cache, then fetches it again to take a reference. The cache holds only a few avformat states at once (4 by default), so with more producers decoding on several threads, another thread's put can evict the new entry in between. Nobody holds a reference yet, so its destructor runs and the state is freed, but the code carried on with the pointer it had put, and also stored a NULL cache item on the frame. Use the state the cache returns, with our reference, and create it again if it has already gone. Sixteen threads, each with its own avformat producer of the same clip reading 150 frames of video and audio (cache size 4): 30 of 30 runs crashed in producer_set_up_video() on master, 0 of 30 with this change. The crashing state was a freshly zeroed one belonging to another producer. Fixes mltframework#1318.

ddennedy · 2026-10-01T18:00:32Z

This is considered a critical fix and low risk change suitable for the immediate release. Thanks

Copilot

Copilot review overview

🟡 Changes recommended

Address the critical mutex-initialization issue and both moderate cache/re-registration issues.

Review effort: Lite
Findings: 1

Open (1)

Publish cache objects only after mutex initialization · New

What changed in this PR

Fixes a use-after-free race in producer_avformat cache-miss handling by reacquiring cache-owned state and retrying after eviction.

Changes:

Recreates state when eviction occurs before reacquisition.
Uses the cache-returned reference during frame setup.
Adds comments describing the race window.

File	Description and review findings
`src/modules/avformat/producer_avformat.c`	Updates cache-miss handling. Critical (4 votes): mutexes may be used before initialization when the state is published. Moderate (1 vote): retry loop can spin indefinitely with a zero-sized cache. Moderate (1 vote): recreated states are not re-registered with `property_changed`.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

ddennedy · 2026-10-03T17:09:30Z

Moving to the release after next since this requires a little more invasive change per the legitimate review comment.

unicorntearsproject · 2026-10-03T18:37:34Z

No worries, I'm running my own fork w/all the fixes & custom wheels built... my new video editor is now 100% multi-threaded! :D Tired of paying Lightworks for HD exports, so mine has 4K & 1080p exports for free..and interface that doesn't look like it's from 1980 and all the bells & whistles including full frei0r & lottie support. You can check it out here: https://rumble.com/v7g9apa-u-stu-video-editor-amazing-new-editor-for-streamers-and-content-creators.html if you're interested. Took me about 40 hours and the bots about 150... another 20/40 for the handful & a half of upstream patches. Linux flat/snap/deb/rpm & Windows installers are in testing. Thank you for all your hard work on MLT! J

…

On Sat, Oct 3, 2026 at 1:09 PM Dan Dennedy ***@***.***> wrote: *ddennedy* left a comment (mltframework/mlt#1330) <#1330 (comment)> Moving to the release after next since this requires a little more invasive change per the legitimate review comment. — Reply to this email directly, view it on GitHub <#1330?email_source=notifications&email_token=ACZWVPHBAZXFUQCLAUBADJD5SEXGBA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKOJXGE2DQNBQGU2KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-5971484054>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACZWVPBAKWLA75XIWGM27HL5SEXGBAVCNFSNUABDKJSXA33TNF2G64TZHM2DAMJZGA2TEO2JONZXKZJ3GU3DKNJSG44TSOBZUF3AE> . Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS <https://github.com/notifications/mobile/ios/ACZWVPFKHJPC4JPPQRCWV3D5SEXGBA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKOJXGE2DQNBQGU2KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG> and Android <https://github.com/notifications/mobile/android/ACZWVPBOEZQU3DIX2T3AWKL5SEXGBA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKOJXGE2DQNBQGU2KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>. Download it today! You are receiving this because you authored the thread.Message ID: ***@***.***>

ddennedy

See review comment.

As raised in review: on a cache miss the state was put in the cache with its mutexes still zeroed. If it was evicted before we took our reference, or used by producer_set_up_video() before producer_open() ran, a mutex was locked before it was initialised (and producer_open() then initialised it while it was locked). - Move the mutex setup into init_mutexes() (it was written out twice) and call it before the state is put in the cache. - Try the cache miss path at most 10 times, so a cache that will not keep the state (a size of 0) cannot make it loop forever; return an empty frame and an error then, as the caller expects a frame.

unicorntearsproject · 2026-10-08T12:14:59Z

Thanks for the review. Pushed a follow-up:

the mutexes are now initialised (init_mutexes(), which replaces two copies of the same code) before the state is put in the cache, so an eviction or an early producer_set_up_video() can't lock an uninitialised mutex;
the cache-miss path tries at most 10 times, and with a cache size of 0 returns an empty frame and an error instead of looping or crashing.

Re-checked: the 16-thread test passes 30 of 30 (it crashed 30 of 30 before the fix), a normal render works, and cache sizes 0 and 1 both finish.

Not changed: the recreated state is not re-registered with property_changed. The listener already holds the first state's pointer, so that looks like a separate, older problem with evicted states; I'm happy to look at it in its own PR.

Copilot

🟡 Changes recommended

A zero-sized cache still causes an out-of-bounds write before the new fallback executes.

1 open finding

Zero cache size causes out-of-bounds write and memory corruption · New

1 resolved since last review

Publish cache objects only after mutex initialization

🧠 Review effort: Balanced

+    if (!self) {
+        // The cache would not keep the state (a cache size of 0, or no memory).
+        // The caller expects a frame even on failure.
+        *frame = mlt_frame_init(service);


unicorntearsproject mentioned this pull request Oct 1, 2026

Crash: avformat decoder cache (size 4) evicts state another thread is still decoding with #1318

Open

ddennedy added this to the v7.42.0 milestone Oct 1, 2026

ddennedy requested a lite review from Copilot October 1, 2026 18:00

Copilot started reviewing on behalf of ddennedy October 1, 2026 18:01 View session

Copilot AI reviewed Oct 1, 2026

View reviewed changes

Comment thread src/modules/avformat/producer_avformat.c

ddennedy modified the milestones: v7.42.0, v7.44.0 Oct 3, 2026

ddennedy requested changes Oct 4, 2026

View reviewed changes

ddennedy requested a balanced review from Copilot October 12, 2026 18:35

Copilot started reviewing on behalf of ddennedy October 12, 2026 18:36 View session

Copilot AI reviewed Oct 12, 2026

View reviewed changes

Comment thread src/modules/avformat/producer_avformat.c

Comment on lines +4477 to +4480

if (!self) {

// The cache would not keep the state (a cache size of 0, or no memory).

// The caller expects a frame even on failure.

*frame = mlt_frame_init(service);

Navigation Menu

Repository navigation

Fix use after free in producer_avformat's cache-miss path - #1330

Fix use after free in producer_avformat's cache-miss path#1330
unicorntearsproject wants to merge 2 commits into
mltframework:masterfrom
unicorntearsproject:fix-avformat-cache-miss-race

unicorntearsproject commented Oct 1, 2026

Uh oh!

ddennedy commented Oct 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

ddennedy commented Oct 3, 2026

Uh oh!

unicorntearsproject commented Oct 3, 2026 via email

Uh oh!

ddennedy left a comment

Uh oh!

unicorntearsproject commented Oct 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

unicorntearsproject commented Oct 1, 2026

Uh oh!

ddennedy commented Oct 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

ddennedy commented Oct 3, 2026

Uh oh!

unicorntearsproject commented Oct 3, 2026 via email

Uh oh!

ddennedy left a comment

Choose a reason for hiding this comment

Uh oh!

unicorntearsproject commented Oct 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants