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