[PATCH 0/1] MR8270: mf: Prevent double submission of session commands.
This is not the right fix, and I need some help. Let me explain. `session_submit_command` checks that: 1. `session->commands` is empty, and 2. `SESSION_FLAG_PENDING_COMMAND` is NOT set before submitting the `session_op`. The assumption is, if there are queued commands, or if there is a command currently running, that pending command will submit subsequent commands when it completes. However, in this call chain: ``` #0 0x00006fffe1d27ad7 in create_async_result (object=object(a)entry=0x0, callback=callback(a)entry=0x7e62489f1c08, state=state(a)entry=0x7e61cb5322b0, out=out(a)entry=0x7e6242bac7a0) at ../dlls/rtworkq/queue.c:1229 #1 0x00006fffe1d222d5 in RtwqCreateAsyncResult (object=object(a)entry=0x0, callback=callback(a)entry=0x7e62489f1c08, state=state(a)entry=0x7e61cb5322b0, out=out(a)entry=0x7e6242bac7a0) at ../dlls/rtworkq/queue.c:1245 #2 0x00006fffd45e2096 in MFPutWorkItem (queue=queue(a)entry=1, callback=callback(a)entry=0x7e62489f1c08, state=state(a)entry=0x7e61cb5322b0) at ../dlls/mfplat/queue.c:46 #3 0x00006fffe41244ee in session_command_complete (session=0x7e62489f1be0) at ../dlls/mf/session.c:975 #4 0x00006fffe412d00f in session_reset (session=session(a)entry=0x7e62489f1be0) at ../dlls/mf/session.c:1051 #5 0x00006fffe41299fc in session_handle_source_shutdown (session=session(a)entry=0x7e62489f1be0) at ../dlls/mf/session.c:2957 #6 0x00006fffe4126e90 in session_events_callback_Invoke (iface=0x7e62489f1c20, result=<optimized out>) at ../dlls/mf/session.c:4445 ``` nowhere was the `SESSION_FLAG_PENDING_COMMAND` flag set. In addition to that, `session_events_callback_Invoke` is not a command. So this could happend: ``` Thread 1 | Thread 2 call session_events_callback_Invoke | | call session_submit_command | <critical section> | list_empty(&session->commands) is true, and PENDING_COMMAND not set | MFPutWorkItem(op) | list_add(&session->commands, op) <- (1) | </critical_section> call session_handle_source_shutdown | <critical_section> | call session_reset | call session_command_complete | list_head(&session->commands) | op is returned because of (1) | MFPutWorkItem(op) again | </critical_section> | ``` The result is this op will be executed twice, and also double freed, because `session_commands_callback_Invoke` releases the `op`. * * * There is even another way an `op` can be submitted twice, `session_handle_start_error` calls `session_reset`, which already calls `session_command_complete`, and later it calls `session_command_complete_with_event` again. I think a missing return? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8270
From: Yuxuan Shui <yshui(a)codeweavers.com> --- dlls/mf/session.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index 483ea6f904f..5ea93da3cee 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -972,6 +972,11 @@ static void session_command_complete(struct media_session *session) if ((e = list_head(&session->commands))) { op = LIST_ENTRY(e, struct session_op, entry); + if (op->submitted) + { + ERR("op %p already submitted\n", op); + return; + } hr = MFPutWorkItem(MFASYNC_CALLBACK_QUEUE_STANDARD, &session->commands_callback, &op->IUnknown_iface); op->submitted = SUCCEEDED(hr); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8270
On second thought, this doesn't need a race to happen. `session_events_callback_Invoke` is not a command so it shouldn't call `session_command_complete` at all I think? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8270#note_106234
@nsivov hi, can you help me and have a look at this? in summary there seems to be a couple of ways a session command can be submitted more than once, which results in crashes, use-after-frees, or weird behaviors. this is the cause of video playback problems in Bloodstained: Ritual of the Night. can you give me some advice what the best way of fixing this is? thanks. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8270#note_110875
The session command handling have been reworked in https://gitlab.winehq.org/wine/wine/-/merge_requests/8402 and https://gitlab.winehq.org/wine/wine/-/merge_requests/8415, with a dedicated state machine for the commands, and `session_purge_pending_commands` has been removed. Can this still happen? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8270#note_111533
From a quick look I guess it's possible that it does, if media source is shutdown while `command_state` is already `COMMAND_STATE_COMPLETE` and the next command has just been submitted but not yet started execution.
I suppose `session_command_complete` should perhaps check `op->submitted` before submitting it again, or we need another `COMMAND_STATE_SUBMITTED` state to replace the per-command submitted flag entirely? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8270#note_111536
On Wed Aug 6 11:34:46 2025 +0000, Rémi Bernon wrote:
From a quick look I guess it's possible that it does, if media source is shutdown while `command_state` is already `COMMAND_STATE_COMPLETE` and the next command has just been submitted but not yet started execution. I suppose `session_command_complete` should perhaps check `op->submitted` before submitting it again, or we need another `COMMAND_STATE_SUBMITTED` state to replace the per-command submitted flag entirely? Adding `COMMAND_STATE_SUBMITTED` makes more sense to me.
I'll close this MR, since your refactoring already fixed the memory bug this MR was originally about. I'll open another MR for the new problem. Though this new problem is much harder to test since it now genuinely requires a race condition. :/ -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8270#note_112208
This merge request was closed by Yuxuan Shui. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8270
participants (3)
-
Rémi Bernon -
Yuxuan Shui
-
Yuxuan Shui (＠yshui)