Skip to content

Retain WinMM buffers when teardown cannot reclaim them - #131

Draft
Blackspirits wants to merge 1 commit into
audit/ffmpeg-waveout-open-failclosed-09e7from
audit/ffmpeg-waveout-teardown-failclosed-4e26
Draft

Blackspirits wants to merge 1 commit into
audit/ffmpeg-waveout-open-failclosed-09e7from
audit/ffmpeg-waveout-teardown-failclosed-4e26

Conversation

@Blackspirits

@Blackspirits Blackspirits commented Sep 15, 2026

Copy link
Copy Markdown
Owner

Purpose

Dependent Windows/WinMM follow-up to #129 ("Fail closed when WinMM output setup is incomplete").

This tranche intentionally uses #129 as its base because it extends the same native-resource ownership boundary from setup into teardown. It does not alter the playback-position logic from #126 or FFmpeg decode/demux behavior.

Finding fixed

WinMM teardown freed native buffer memory even when the driver had not released it

WaveOutAudioSink.CloseCore() previously ignored every native teardown result:

  1. call waveOutReset();
  2. call waveOutUnprepareHeader() for every header;
  3. call waveOutClose();
  4. unconditionally free _headers and _data and close the event.

Microsoft's WinMM contract does not permit that assumption:

  • waveOutReset() returns an error on failure; only successful reset guarantees pending buffers are marked done/returned;
  • waveOutUnprepareHeader() returns WAVERR_STILLPLAYING if a data block is still in the queue, and Microsoft explicitly requires the driver to be finished with a buffer before it is unprepared/freed;
  • waveOutClose() returns WAVERR_STILLPLAYING while buffers remain queued.

Therefore a failed reset/unprepare/close could be followed by freeing memory still referenced by the audio driver — a native use-after-free during Dispose/error cleanup.

Fix

CloseCore() is now a retryable fail-closed transaction:

  • require successful waveOutReset() before proceeding;
  • require every waveOutUnprepareHeader() call to succeed before closing the device;
  • require successful waveOutClose() before clearing the device handle or freeing buffer memory;
  • if any of those operations fail, log the native error and retain the complete WinMM device/header/data/event boundary for a later Dispose retry;
  • only after the device is safely closed are _headers and _data freed;
  • CloseHandle() is also checked; if it fails, only the event handle is retained for retry because the waveform device and buffer memory are already safely released;
  • Open() refuses to initialize a replacement device if cleanup of a previous one could not complete safely.

Retry after a partial unprepare pass is valid: Microsoft documents that unpreparing a header that is already unprepared has no effect and returns success.

Scope / branch state

  • dependency/base PR: Fail closed when WinMM output setup is incomplete #129
  • base HEAD: 4e262966119437f3e90f87693ff279c65dc20dcb
  • HEAD: eb9ccb347022263004d8f0d79bd623c247bc87ae
  • 1 commit
  • 1 file
  • +40 / -6
  • current upstream main last checked: a3855e5fb200e8c5390704cf67df5b0485c355bf
  • audit draft only; no merge/promotion intended

AI assistance: ChatGPT was used for adversarial WinMM teardown/ownership review, Microsoft API contract verification, controlled-leak/retry design, and diff review.

Final CI

Authoritative run: SubtitleEdit#264 on eb9ccb347022263004d8f0d79bd623c247bc87ae

  • SeConv: 488 passed / 2 skipped / 0 failed
  • LibUiLogic: 905 passed / 0 skipped / 0 failed
  • LibSE: 2017 passed / 0 skipped / 0 failed
  • UI: 5233 passed / 9 skipped / 0 failed
  • build: 0 errors / 7 pre-existing warnings
  • retry step skipped
  • failure-artifact upload skipped

Final adversarial review was performed on this exact HEAD after CI; no blocker found.

@Blackspirits Blackspirits left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Final adversarial review on frozen HEAD eb9ccb347022263004d8f0d79bd623c247bc87ae after CI SubtitleEdit#264.

Rechecked WinMM teardown ownership across reset failure, partial header-unprepare failure, close failure, event-handle failure, repeated Dispose, and Open-after-failed-cleanup. Buffer/header memory is now released only after WinMM has successfully reset, unprepared every header, and closed the device. Any earlier native failure retains the complete device/header/data/event boundary for retry. Retrying after a partial unprepare pass is safe because already-unprepared headers can be passed to waveOutUnprepareHeader again. If only CloseHandle fails after the waveform device has been closed, only that event handle is retained.

No blocker or regression found in the changed set.

CI SubtitleEdit#264 passed on this exact HEAD: SeConv 488 passed / 2 skipped / 0 failed; LibUiLogic 905/0/0; LibSE 2017/0/0; UI 5233 passed / 9 skipped / 0 failed. Build: 0 errors / 7 pre-existing warnings. Retry step skipped. PR remains draft; no merge performed.

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.

1 participant