[PATCH 0/1] MR29: mfplat: Avoid a race condition when releasing a tracked sample.

[Giovanni Mascellani (＠giomasce)]

Right now, if the same tracked sample is released at the same time
from two different threads it might happen that neither of them calls
the callback, because they might go through the critical section at
the same time (while neither has decremented the reference count yet).

Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com>

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/29

[PATCH 1/1] mfplat: Avoid a race condition when releasing a tracked sample.

[Giovanni Mascellani]

From: Giovanni Mascellani <gmascellani(a)codeweavers.com>

Right now, if the same tracked sample is released at the same time
from two different threads it might happen that neither of them calls
the callback, because they might go through the critical section at
the same time (while neither has decremented the reference count yet).

Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com>
---
 dlls/mfplat/sample.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/dlls/mfplat/sample.c b/dlls/mfplat/sample.c
index 8ef80eb5f24..ed39b5f7297 100644
--- a/dlls/mfplat/sample.c
+++ b/dlls/mfplat/sample.c
@@ -182,11 +182,11 @@ static ULONG WINAPI sample_Release(IMFSample *iface)
 static ULONG WINAPI sample_tracked_Release(IMFSample *iface)
 {
     struct sample *sample = impl_from_IMFSample(iface);
-    ULONG refcount;
+    ULONG refcount = InterlockedDecrement(&sample->attributes.ref);
     HRESULT hr;
 
     EnterCriticalSection(&sample->attributes.cs);
-    if (sample->tracked_result && sample->tracked_refcount == (sample->attributes.ref - 1))
+    if (sample->tracked_result && sample->tracked_refcount == refcount)
     {
         IRtwqAsyncResult *tracked_result = sample->tracked_result;
         sample->tracked_result = NULL;
@@ -199,8 +199,6 @@ static ULONG WINAPI sample_tracked_Release(IMFSample *iface)
     }
     LeaveCriticalSection(&sample->attributes.cs);
 
-    refcount = InterlockedDecrement(&sample->attributes.ref);
-
     TRACE("%p, refcount %lu.\n", iface, refcount);
 
     if (!refcount)
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/29

MR29v1 - mfplat: Avoid a race condition when releasing a tracked sample.

[Nikolay Sivov (＠nsivov)]

I don't think this works if RtwqInvokeCallback() fails. The point of checking first and decrementing later is because of a circular reference between tracked_result and the sample.

In normal case after SetAllocator() you get refcount == 2 and tracked_refcount == 1. Now it queues are not working:

* Release() decrements to refcount == 1 and tracked_refcount == 1;
* condition matches and tracked result it released, recursing back to sample_tracked_Release();
* recursed call decrements refcount to 0 and releases the sample.

So you're losing 2 references on a single Release() call. I supposed decrementing while still locked will work?

P.S. note that there is almost identical copy of this in dlls/evr/sample.c that needs the same fix, and probably use of an explicit enter/leave instead of a LockStore().

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/29#note_567

MR29v1 - mfplat: Avoid a race condition when releasing a tracked sample.

[Giovanni Mascellani (＠giomasce)]

I might be missing something, but isn't that what is supposed to happen? If `RtwqInvokeCallback()` fails (and `tracked_result` is then released), then nobody will ever pick the sample up again, so it is appropriate to have it freed, isn't it?

As you say, after `SetAllocator()` is called `refcount == 2` (one reference is the client, the other is the object self-reference itself) and `tracked_refcount == 1` (the self-reference again). Once the client code is done, it calls `Release()` and `refcount` drops to 1, so the callback is called.

* If the callback succeeds, presumably the client (specifically, the sample allocator, I believe) code will take a reference to the sample again, so `refcount` will become 2 again. After the async result is released and presumably freed, it will release the self-reference to the sample, so `refcount` will go to 1 again. And of course `tracked_refcount` is set to zero. This seems to be correct: at the end of the process there is precisely a reference to the sample, by the sample allocator.

* If the callback fails, the client will never recover a reference to the sample, so `refcount` will remain at 1, just to drop to zero once the async result is freed. This is correct: nobody holds a reference to the sample any more, and then it's correct to free it.

So I don't understand what is the problem that you're seeing.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/29#note_588

MR29v1 - mfplat: Avoid a race condition when releasing a tracked sample.

[Nikolay Sivov (＠nsivov)]

Ok, thanks, I think I understand now. It looks like refcount drops in case on uninitialized rtworkq as well, so we probably should do the same, and in fact it does already happen without your patch. Remaining concern is reworking allocator sample lists so stale pointers are never kept. I'm going to look at that.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/29#note_594