This patch removes a work around that causes a crash in Unravel Two. There is a callback in Unravel Two that appears to add a reference to a IMediaSample, which this work around treats as a leak and releases. However, the application also releases the reference causing a negative overflow and an assertion failure on line 345 of dlls/quartz/memallocator.c. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51616 Signed-off-by: Brendan McGrath <brendan(a)redmandi.com> --- Although I'm not convinced just deleting 12 yr old code is a great idea, the original author did describe this work around as "ugly". I would describe it as unsafe. See Wine Bug 51616. I think it may also be possible for the work around to fail in a multi-threaded scenario, where the IMediaSample is accessed in parallel to the callback; potentially with a negative overflow of its own (and looping 4294967295 times). Although I'm not familiar enough with the uses of qedit to understand if this is even plausible. Unfortunately, I've only tested the removal of this work around with the one application; Unravel Two. Also note that I've only been able to test this "fix" with a local build of Proton 7 Experimental. I couldn't get the application to run in vanilla Wine (as the Origin launcher failed to install). Definitely interested in thoughts. dlls/qedit/samplegrabber.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/dlls/qedit/samplegrabber.c b/dlls/qedit/samplegrabber.c index 9b9780e564a8..cefb2f370785 100644 --- a/dlls/qedit/samplegrabber.c +++ b/dlls/qedit/samplegrabber.c @@ -164,9 +164,6 @@ static void SampleGrabber_callback(struct sample_grabber *This, IMediaSample *sa if (ref) { ERR("(%p) Callback referenced sample %p by %lu\n", This, sample, ref); - /* ugly as hell but some apps are sooo buggy */ - while (ref--) - IMediaSample_Release(sample); } } break; -- 2.34.1
On 7/8/22 01:28, Brendan McGrath wrote:
This patch removes a work around that causes a crash in Unravel Two.
There is a callback in Unravel Two that appears to add a reference to a IMediaSample, which this work around treats as a leak and releases. However, the application also releases the reference causing a negative overflow and an assertion failure on line 345 of dlls/quartz/memallocator.c.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51616 Signed-off-by: Brendan McGrath <brendan(a)redmandi.com> ---
Although I'm not convinced just deleting 12 yr old code is a great idea, the original author did describe this work around as "ugly". I would describe it as unsafe. See Wine Bug 51616.
I think it may also be possible for the work around to fail in a multi-threaded scenario, where the IMediaSample is accessed in parallel to the callback; potentially with a negative overflow of its own (and looping 4294967295 times). Although I'm not familiar enough with the uses of qedit to understand if this is even plausible.
Unfortunately, I've only tested the removal of this work around with the one application; Unravel Two.
Also note that I've only been able to test this "fix" with a local build of Proton 7 Experimental. I couldn't get the application to run in vanilla Wine (as the Origin launcher failed to install).
Definitely interested in thoughts.
I'd very much like to remove this if it's incorrect, but obviously it was put there for a reason, and I don't feel comfortable just removing it in that case. It'd be nice if we can test with whatever original application—I've CC'd the original author just in case he's still responsive and can remember what was affected—but in lieu of that I'd rather see tests justifying this change in behaviour.
Unravel Two adds a reference to the IMediaSample during its callback. This patch adds a test that checks if an application does do this, that it can free it later and the reference count will finish at zero. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51616 Signed-off-by: Brendan McGrath <brendan(a)redmandi.com> --- Thanks Zeb for taking a look. Agree with your feedback. I guess we need to prove the workaround is incorrect. This test I think replicates what Unravel Two does, and it does demonstrate that the workaround will cause an issue in this scenario. But it would be good to hear back from the original author; although 12 years is a long time! Unrelated to this patchset - but I noticed I get a seg fault when running the mediadet tests (the others seem ok). It happens right at the end: Segmentation fault (core dumped) make: *** [Makefile:120413: dlls/qedit/tests/mediadet.ok] Error 139 I didn't see it on the Wine test servers, but I'm wondering if you get the same when you run locally? Anyway, thanks again. Appreciate your time. dlls/qedit/tests/mediadet.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/dlls/qedit/tests/mediadet.c b/dlls/qedit/tests/mediadet.c index 3202baca9244..3ea752176618 100644 --- a/dlls/qedit/tests/mediadet.c +++ b/dlls/qedit/tests/mediadet.c @@ -866,6 +866,11 @@ static void test_put_filter(void) ok(!ref, "Got outstanding refcount %ld.\n", ref); } +struct MySample { + IMediaSample sample; + LONG refcount; +}; + static HRESULT WINAPI ms_QueryInterface(IMediaSample *iface, REFIID riid, void **ppvObject) { @@ -874,12 +879,18 @@ static HRESULT WINAPI ms_QueryInterface(IMediaSample *iface, REFIID riid, static ULONG WINAPI ms_AddRef(IMediaSample *iface) { - return 2; + struct MySample *my_sample = (struct MySample*) iface; + ULONG refcount = InterlockedIncrement(&my_sample->refcount); + + return refcount; } static ULONG WINAPI ms_Release(IMediaSample *iface) { - return 1; + struct MySample *my_sample = (struct MySample*) iface; + ULONG refcount = InterlockedDecrement(&my_sample->refcount); + + return refcount; } static HRESULT WINAPI ms_GetPointer(IMediaSample *iface, BYTE **ppBuffer) @@ -989,7 +1000,7 @@ static const IMediaSampleVtbl my_sample_vt = { ms_SetMediaTime }; -static IMediaSample my_sample = { &my_sample_vt }; +static struct MySample my_sample = { {&my_sample_vt}, 0 }; static BOOL samplecb_called = FALSE; @@ -1012,8 +1023,9 @@ static ULONG WINAPI sgcb_Release(ISampleGrabberCB *iface) static HRESULT WINAPI sgcb_SampleCB(ISampleGrabberCB *iface, double SampleTime, IMediaSample *pSample) { - ok(pSample == &my_sample, "Got wrong IMediaSample: %p, expected %p\n", pSample, &my_sample); + ok(pSample == &my_sample.sample, "Got wrong IMediaSample: %p, expected %p\n", pSample, &my_sample); samplecb_called = TRUE; + IMediaSample_AddRef(pSample); return E_NOTIMPL; } @@ -1043,6 +1055,7 @@ static void test_samplegrabber(void) IEnumPins *pins; HRESULT hr; FILTER_STATE fstate; + ULONG refcount; /* Invalid RIID */ hr = CoCreateInstance(&CLSID_SampleGrabber, NULL, CLSCTX_INPROC_SERVER, &IID_IClassFactory, @@ -1074,10 +1087,14 @@ static void test_samplegrabber(void) hr = IPin_QueryInterface(pin, &IID_IMemInputPin, (void**)&inpin); ok(hr == S_OK, "Got hr %#lx.\n", hr); - hr = IMemInputPin_Receive(inpin, &my_sample); + hr = IMemInputPin_Receive(inpin, &my_sample.sample); ok(hr == S_OK, "Got hr %#lx.\n", hr); ok(samplecb_called == TRUE, "SampleCB should have been called\n"); + // Release the Ref we added in the callback + refcount = IUnknown_Release(&my_sample.sample); + todo_wine ok(refcount == 0, "refcount should be zero\n"); + IMemInputPin_Release(inpin); IPin_Release(pin); -- 2.34.1
This patch removes a work around that causes a crash in Unravel Two. There is a callback in Unravel Two that appears to add a reference to a IMediaSample, which this work around treats as a leak and releases. However, the application also releases the reference causing a negative overflow and an assertion failure on line 345 of dlls/quartz/memallocator.c. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51616 Signed-off-by: Brendan McGrath <brendan(a)redmandi.com> --- dlls/qedit/samplegrabber.c | 3 --- dlls/qedit/tests/mediadet.c | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/dlls/qedit/samplegrabber.c b/dlls/qedit/samplegrabber.c index 9b9780e564a8..cefb2f370785 100644 --- a/dlls/qedit/samplegrabber.c +++ b/dlls/qedit/samplegrabber.c @@ -164,9 +164,6 @@ static void SampleGrabber_callback(struct sample_grabber *This, IMediaSample *sa if (ref) { ERR("(%p) Callback referenced sample %p by %lu\n", This, sample, ref); - /* ugly as hell but some apps are sooo buggy */ - while (ref--) - IMediaSample_Release(sample); } } break; diff --git a/dlls/qedit/tests/mediadet.c b/dlls/qedit/tests/mediadet.c index 3ea752176618..8026b37f53ef 100644 --- a/dlls/qedit/tests/mediadet.c +++ b/dlls/qedit/tests/mediadet.c @@ -1093,7 +1093,7 @@ static void test_samplegrabber(void) // Release the Ref we added in the callback refcount = IUnknown_Release(&my_sample.sample); - todo_wine ok(refcount == 0, "refcount should be zero\n"); + ok(refcount == 0, "refcount should be zero\n"); IMemInputPin_Release(inpin); IPin_Release(pin); -- 2.34.1
On Sat, Jul 9, 2022 at 9:52 AM Brendan McGrath <brendan(a)redmandi.com> wrote:
Unravel Two adds a reference to the IMediaSample during its callback.
This patch adds a test that checks if an application does do this, that it can free it later and the reference count will finish at zero.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51616 Signed-off-by: Brendan McGrath <brendan(a)redmandi.com> ---
Thanks Zeb for taking a look. Agree with your feedback. I guess we need to prove the workaround is incorrect.
This test I think replicates what Unravel Two does, and it does demonstrate that the workaround will cause an issue in this scenario.
But it would be good to hear back from the original author; although 12 years is a long time!
Unrelated to this patchset - but I noticed I get a seg fault when running the mediadet tests (the others seem ok). It happens right at the end: Segmentation fault (core dumped) make: *** [Makefile:120413: dlls/qedit/tests/mediadet.ok] Error 139
I didn't see it on the Wine test servers, but I'm wondering if you get the same when you run locally?
Yes, I also get that.
Anyway, thanks again. Appreciate your time.
dlls/qedit/tests/mediadet.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/dlls/qedit/tests/mediadet.c b/dlls/qedit/tests/mediadet.c index 3202baca9244..3ea752176618 100644 --- a/dlls/qedit/tests/mediadet.c +++ b/dlls/qedit/tests/mediadet.c @@ -866,6 +866,11 @@ static void test_put_filter(void) ok(!ref, "Got outstanding refcount %ld.\n", ref); }
+struct MySample { + IMediaSample sample; + LONG refcount; +}; + static HRESULT WINAPI ms_QueryInterface(IMediaSample *iface, REFIID riid, void **ppvObject) { @@ -874,12 +879,18 @@ static HRESULT WINAPI ms_QueryInterface(IMediaSample *iface, REFIID riid,
static ULONG WINAPI ms_AddRef(IMediaSample *iface) { - return 2; + struct MySample *my_sample = (struct MySample*) iface; + ULONG refcount = InterlockedIncrement(&my_sample->refcount); + + return refcount; }
static ULONG WINAPI ms_Release(IMediaSample *iface) { - return 1; + struct MySample *my_sample = (struct MySample*) iface; + ULONG refcount = InterlockedDecrement(&my_sample->refcount); + + return refcount; }
static HRESULT WINAPI ms_GetPointer(IMediaSample *iface, BYTE **ppBuffer) @@ -989,7 +1000,7 @@ static const IMediaSampleVtbl my_sample_vt = { ms_SetMediaTime };
-static IMediaSample my_sample = { &my_sample_vt }; +static struct MySample my_sample = { {&my_sample_vt}, 0 };
static BOOL samplecb_called = FALSE;
@@ -1012,8 +1023,9 @@ static ULONG WINAPI sgcb_Release(ISampleGrabberCB *iface) static HRESULT WINAPI sgcb_SampleCB(ISampleGrabberCB *iface, double SampleTime, IMediaSample *pSample) { - ok(pSample == &my_sample, "Got wrong IMediaSample: %p, expected %p\n", pSample, &my_sample); + ok(pSample == &my_sample.sample, "Got wrong IMediaSample: %p, expected %p\n", pSample, &my_sample); samplecb_called = TRUE; + IMediaSample_AddRef(pSample); return E_NOTIMPL; }
@@ -1043,6 +1055,7 @@ static void test_samplegrabber(void) IEnumPins *pins; HRESULT hr; FILTER_STATE fstate; + ULONG refcount;
/* Invalid RIID */ hr = CoCreateInstance(&CLSID_SampleGrabber, NULL, CLSCTX_INPROC_SERVER, &IID_IClassFactory, @@ -1074,10 +1087,14 @@ static void test_samplegrabber(void) hr = IPin_QueryInterface(pin, &IID_IMemInputPin, (void**)&inpin); ok(hr == S_OK, "Got hr %#lx.\n", hr);
- hr = IMemInputPin_Receive(inpin, &my_sample); + hr = IMemInputPin_Receive(inpin, &my_sample.sample); ok(hr == S_OK, "Got hr %#lx.\n", hr); ok(samplecb_called == TRUE, "SampleCB should have been called\n");
+ // Release the Ref we added in the callback + refcount = IUnknown_Release(&my_sample.sample); + todo_wine ok(refcount == 0, "refcount should be zero\n"); + IMemInputPin_Release(inpin); IPin_Release(pin);
-- 2.34.1
On 15/7/22 05:02, David Kahurani wrote:
On Sat, Jul 9, 2022 at 9:52 AM Brendan McGrath<brendan(a)redmandi.com> wrote:
Unravel Two adds a reference to the IMediaSample during its callback.
This patch adds a test that checks if an application does do this, that it can free it later and the reference count will finish at zero.
Wine-Bug:https://bugs.winehq.org/show_bug.cgi?id=51616 Signed-off-by: Brendan McGrath<brendan(a)redmandi.com> ---
Thanks Zeb for taking a look. Agree with your feedback. I guess we need to prove the workaround is incorrect.
This test I think replicates what Unravel Two does, and it does demonstrate that the workaround will cause an issue in this scenario.
But it would be good to hear back from the original author; although 12 years is a long time!
Unrelated to this patchset - but I noticed I get a seg fault when running the mediadet tests (the others seem ok). It happens right at the end: Segmentation fault (core dumped) make: *** [Makefile:120413: dlls/qedit/tests/mediadet.ok] Error 139
I didn't see it on the Wine test servers, but I'm wondering if you get the same when you run locally? Yes, I also get that.
I've created a wine bug for this: https://bugs.winehq.org/show_bug.cgi?id=53556 I've added a lot more detail there, and an experimental fix. I'd be curious to know if the segfault you are seeing is the same thing.
Anyway, thanks again. Appreciate your time.
dlls/qedit/tests/mediadet.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/dlls/qedit/tests/mediadet.c b/dlls/qedit/tests/mediadet.c index 3202baca9244..3ea752176618 100644 --- a/dlls/qedit/tests/mediadet.c +++ b/dlls/qedit/tests/mediadet.c @@ -866,6 +866,11 @@ static void test_put_filter(void) ok(!ref, "Got outstanding refcount %ld.\n", ref); }
+struct MySample { + IMediaSample sample; + LONG refcount; +}; + static HRESULT WINAPI ms_QueryInterface(IMediaSample *iface, REFIID riid, void **ppvObject) { @@ -874,12 +879,18 @@ static HRESULT WINAPI ms_QueryInterface(IMediaSample *iface, REFIID riid,
static ULONG WINAPI ms_AddRef(IMediaSample *iface) { - return 2; + struct MySample *my_sample = (struct MySample*) iface; + ULONG refcount = InterlockedIncrement(&my_sample->refcount); + + return refcount; }
static ULONG WINAPI ms_Release(IMediaSample *iface) { - return 1; + struct MySample *my_sample = (struct MySample*) iface; + ULONG refcount = InterlockedDecrement(&my_sample->refcount); + + return refcount; }
static HRESULT WINAPI ms_GetPointer(IMediaSample *iface, BYTE **ppBuffer) @@ -989,7 +1000,7 @@ static const IMediaSampleVtbl my_sample_vt = { ms_SetMediaTime };
-static IMediaSample my_sample = { &my_sample_vt }; +static struct MySample my_sample = { {&my_sample_vt}, 0 };
static BOOL samplecb_called = FALSE;
@@ -1012,8 +1023,9 @@ static ULONG WINAPI sgcb_Release(ISampleGrabberCB *iface) static HRESULT WINAPI sgcb_SampleCB(ISampleGrabberCB *iface, double SampleTime, IMediaSample *pSample) { - ok(pSample == &my_sample, "Got wrong IMediaSample: %p, expected %p\n", pSample, &my_sample); + ok(pSample == &my_sample.sample, "Got wrong IMediaSample: %p, expected %p\n", pSample, &my_sample); samplecb_called = TRUE; + IMediaSample_AddRef(pSample); return E_NOTIMPL; }
@@ -1043,6 +1055,7 @@ static void test_samplegrabber(void) IEnumPins *pins; HRESULT hr; FILTER_STATE fstate; + ULONG refcount;
/* Invalid RIID */ hr = CoCreateInstance(&CLSID_SampleGrabber, NULL, CLSCTX_INPROC_SERVER, &IID_IClassFactory, @@ -1074,10 +1087,14 @@ static void test_samplegrabber(void) hr = IPin_QueryInterface(pin, &IID_IMemInputPin, (void**)&inpin); ok(hr == S_OK, "Got hr %#lx.\n", hr);
- hr = IMemInputPin_Receive(inpin, &my_sample); + hr = IMemInputPin_Receive(inpin, &my_sample.sample); ok(hr == S_OK, "Got hr %#lx.\n", hr); ok(samplecb_called == TRUE, "SampleCB should have been called\n");
+ // Release the Ref we added in the callback + refcount = IUnknown_Release(&my_sample.sample); + todo_wine ok(refcount == 0, "refcount should be zero\n"); + IMemInputPin_Release(inpin); IPin_Release(pin);
-- 2.34.1
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=121193 Your paranoid android. === build (build log) === error: patch failed: dlls/qedit/tests/mediadet.c:866 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/qedit/tests/mediadet.c:866 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/qedit/tests/mediadet.c:866 Task: Patch failed to apply
participants (4)
-
Brendan McGrath -
David Kahurani -
Marvin -
Zebediah Figura