[PATCH] quartz: Increment reference count for the advise thread to avoid crashes caused by DLL unloading.

[Hiroki Awata]

Some games crash when the DLL is unloaded during SleepConditionVariableCS on SystemClockAdviseThread.
This is due to the fact that DllCanUnloadNow returns S_OK even though the advise thread is running.
Incrementing the reference count when creating the advise thread resolves it.

Signed-off-by: Hiroki Awata 
---
 dlls/quartz/systemclock.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/dlls/quartz/systemclock.c b/dlls/quartz/systemclock.c
index 73a923a7b99..7302de0e58f 100644
--- a/dlls/quartz/systemclock.c
+++ b/dlls/quartz/systemclock.c
@@ -104,6 +104,7 @@ static ULONG WINAPI system_clock_inner_Release(IUnknown *iface)
             WakeConditionVariable(&clock->cv);
             WaitForSingleObject(clock->thread, INFINITE);
             CloseHandle(clock->thread);
+            InterlockedDecrement(&object_locks);
         }

LIST_FOR_EACH_ENTRY_SAFE(sink, cursor, &clock->sinks, struct advise_sink, entry)
@@ -207,6 +208,8 @@ static HRESULT add_sink(struct system_clock *clock, DWORD_PTR handle,
     if (!InterlockedCompareExchange(&clock->thread_created, TRUE, FALSE))
     {
         clock->thread = CreateThread(NULL, 0, SystemClockAdviseThread, clock, 0, NULL);
+        if (clock->thread)
+            InterlockedIncrement(&object_locks);
     }
     WakeConditionVariable(&clock->cv);

[Zebediah Figura (she/her)]

Hello Hiroki, thanks for the patch!

On 8/10/21 5:38 PM, Hiroki Awata wrote:
> Some games crash when the DLL is unloaded during SleepConditionVariableCS on SystemClockAdviseThread.
> This is due to the fact that DllCanUnloadNow returns S_OK even though the advise thread is running.
> Incrementing the reference count when creating the advise thread resolves it.

This doesn't seem right; the system clock object itself should be 
holding a reference [added by DSCF_CreateInstance(), removed by 
system_clock_inner_Release()] and waits for the thread to stop before 
removing it. Are you sure we're not leaking a reference elsewhere?

[粟田大樹]

Hi, Zebediah. Thank you for your quick reply.

It was a very interesting problem and I logged the changes in object_locks.
In my tests, object_locks goes to 0 without system_clock_inner_Release
being called.
The game I tested does not have a free version, so I apologize for not
sharing it. The game is clickable to skip video playback, and the crash
occurs when clicking.

I have attached the log of my test for your reference. Thank you,

2021年8月11日(水) 7:52 Zebediah Figura (she/her) :
> Hello Hiroki, thanks for the patch!
>
> On 8/10/21 5:38 PM, Hiroki Awata wrote:
> > Some games crash when the DLL is unloaded during
>
> SleepConditionVariableCS on SystemClockAdviseThread.
> > This is due to the fact that DllCanUnloadNow returns S_OK even though
>
> the advise thread is running.
> > Incrementing the reference count when creating the advise thread
>
> resolves it.
>
> This doesn't seem right; the system clock object itself should be
> holding a reference [added by DSCF_CreateInstance(), removed by
> system_clock_inner_Release()] and waits for the thread to stop before
> removing it. Are you sure we're not leaking a reference elsewhere?

[Zebediah Figura (she/her)]

On 8/10/21 6:40 PM, 粟田大樹 wrote:
> Hi, Zebediah. Thank you for your quick reply.
>
> It was a very interesting problem and I logged the changes in object_locks.
> In my tests, object_locks goes to 0 without system_clock_inner_Release
> being called.
> The game I tested does not have a free version, so I apologize for not
> sharing it. The game is clickable to skip video playback, and the crash
> occurs when clicking.
>
> I have attached the log of my test for your reference. Thank you,

From examination of the code I believe the problem is that we call 
system_clock_create() from dsound_render_create(), which bypasses the 
increment.

Either that should be changed to CoCreateInstance, or we should only 
increment object_locks from creation functions. I'm kind of leaning 
toward the latter, even though it's more code, since it's more 
consistent (along the lines of "always grab and release a resource in 
the same place").

I note also that enum_reg_filter_create() and enum_moniker_create() 
don't reference the module, and should.

[粟田大樹]

Hi, Zebediah.
Thanks for the detailed research.
I've revert the first patch, applied the next one and the test was
successful!
Is the code fix as intended? If it is ok, I will send a PATCH v2.

diff --git a/dlls/quartz/systemclock.c b/dlls/quartz/systemclock.c
index 73a923a7b99..0eb339420db 100644
--- a/dlls/quartz/systemclock.c
+++ b/dlls/quartz/systemclock.c
@@ -338,6 +338,7 @@ HRESULT system_clock_create(IUnknown *outer, IUnknown
**out)
     list_init(&object->sinks);
     InitializeCriticalSection(&object->cs);
     object->cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ":
SystemClockImpl.cs");
+    InterlockedIncrement(&object_locks);

TRACE("Created system clock %p.\n", object);
     *out = &object->IUnknown_inner;

2021年8月11日(水) 8:56 Zebediah Figura (she/her) :
> On 8/10/21 6:40 PM, 粟田大樹 wrote:
> > Hi, Zebediah. Thank you for your quick reply.
> >
> > It was a very interesting problem and I logged the changes in
>
> object_locks.
> > In my tests, object_locks goes to 0 without system_clock_inner_Release
> > being called.
> > The game I tested does not have a free version, so I apologize for not
> > sharing it. The game is clickable to skip video playback, and the crash
> > occurs when clicking.
> >
> > I have attached the log of my test for your reference. Thank you,
>
> From examination of the code I believe the problem is that we call
> system_clock_create() from dsound_render_create(), which bypasses the
> increment.
>
> Either that should be changed to CoCreateInstance, or we should only
> increment object_locks from creation functions. I'm kind of leaning
> toward the latter, even though it's more code, since it's more
> consistent (along the lines of "always grab and release a resource in
> the same place").
>
> I note also that enum_reg_filter_create() and enum_moniker_create()
> don't reference the module, and should.

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=95434

Your paranoid android.

=== debiant2 (build log) ===

error: corrupt patch at line 12
Task: Patch failed to apply

=== debiant2 (build log) ===

error: corrupt patch at line 12
Task: Patch failed to apply

[Zebediah Figura (she/her)]

On 8/10/21 7:18 PM, 粟田大樹 wrote:
> Hi, Zebediah.
> Thanks for the detailed research.
> I've revert the first patch, applied the next one and the test was
> successful!
> Is the code fix as intended? If it is ok, I will send a PATCH v2.
>
> diff --git a/dlls/quartz/systemclock.c b/dlls/quartz/systemclock.c
> index 73a923a7b99..0eb339420db 100644
> --- a/dlls/quartz/systemclock.c
> +++ b/dlls/quartz/systemclock.c
> @@ -338,6 +338,7 @@ HRESULT system_clock_create(IUnknown *outer, IUnknown
> **out)
>       list_init(&object->sinks);
>       InitializeCriticalSection(&object->cs);
>       object->cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ":
> SystemClockImpl.cs");
> -
>
> InterlockedIncrement(&object_locks);
>
> TRACE("Created system clock %p.\n", object);
>  *out = &object->IUnknown_inner;

Well, it's not quite as simple as that hunk; now you're leaking 
references for any system clock created via CoCreateInstance(). You'll 
need to do the same for all other quartz objects, and remove the 
InterlockedIncrement() from DSCF_CreateInstance().

[Hiroki Awata]

Oh, I see. I'm not familiar with quartz and I'm not sure I can cover
everything. Would you be able to create a patch?

2021年8月11日(水) 10:23 Zebediah Figura (she/her) :
> On 8/10/21 7:18 PM, 粟田大樹 wrote:
> > Hi, Zebediah.
> > Thanks for the detailed research.
> > I've revert the first patch, applied the next one and the test was
> > successful!
> > Is the code fix as intended? If it is ok, I will send a PATCH v2.
> >
> > diff --git a/dlls/quartz/systemclock.c b/dlls/quartz/systemclock.c
> > index 73a923a7b99..0eb339420db 100644
> > --- a/dlls/quartz/systemclock.c
> > +++ b/dlls/quartz/systemclock.c
> > @@ -338,6 +338,7 @@ HRESULT system_clock_create(IUnknown *outer, IUnknown
> > **out)
> >       list_init(&object->sinks);
> >       InitializeCriticalSection(&object->cs);
> >       object->cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ":
> > SystemClockImpl.cs");
> > -
> >
> > InterlockedIncrement(&object_locks);
> >
> > TRACE("Created system clock %p.\n", object);
> >  *out = &object->IUnknown_inner;
>
> Well, it's not quite as simple as that hunk; now you're leaking
> references for any system clock created via CoCreateInstance(). You'll
> need to do the same for all other quartz objects, and remove the
> InterlockedIncrement() from DSCF_CreateInstance().