[PATCH] qedit: Remove "ugly" work around

[Brendan McGrath]

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 
---

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;

[Zebediah Figura]

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
>
> 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.

[PATCH v2 1/2] qedit: Add test for adding a reference during callback

[Brendan McGrath]

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 
---

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);

[PATCH v2 2/2] qedit: Remove "ugly" work around

[Brendan McGrath]

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 
---
 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);

[PATCH v2 1/2] qedit: Add test for adding a reference during callback

[David Kahurani]

On Sat, Jul 9, 2022 at 9:52 AM Brendan McGrath  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
>
> 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

[PATCH v2 1/2] qedit: Add test for adding a reference during callback

[Brendan McGrath]

On 15/7/22 05:02, David Kahurani wrote:
> On Sat, Jul 9, 2022 at 9:52 AM Brendan McGrath  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
> >
> > 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

[PATCH v2 1/2] qedit: Add test for adding a reference during callback

[Marvin]

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