[PATCH v2] urlmon: Always report available data before stop_binding.
The HTML launcher of several Rebellion games (all probably based on the same code) use a custom protocol handler which calls ReportResult in LockRequest, causing an invalid memory access when OnDataAvailable callback is called, as the binding has already been stopped and terminated. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=46213 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52286 Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- v2: Use an even more ad-hoc flag to detect LockRequest calls during ReportData. dlls/mshtml/tests/htmldoc.c | 18 +++++++++++++++++- dlls/urlmon/binding.c | 23 +++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/dlls/mshtml/tests/htmldoc.c b/dlls/mshtml/tests/htmldoc.c index 89311c5d8c8..f8df7d7736d 100644 --- a/dlls/mshtml/tests/htmldoc.c +++ b/dlls/mshtml/tests/htmldoc.c @@ -522,6 +522,8 @@ static const IDispatchVtbl ExternalVtbl = { static IDispatch External = { &ExternalVtbl }; +static IInternetProtocolSink *protocol_sink; + static HRESULT WINAPI Protocol_QueryInterface(IInternetProtocol *iface, REFIID riid, void **ppv) { if(IsEqualGUID(&IID_IUnknown, riid) || IsEqualGUID(&IID_IInternetProtocol, riid)) { @@ -556,6 +558,8 @@ static HRESULT WINAPI Protocol_Start(IInternetProtocol *iface, LPCWSTR szUrl, CHECK_EXPECT(Start); + protocol_sink = pOIProtSink; + ok(pOIProtSink != NULL, "pOIProtSink == NULL\n"); ok(pOIBindInfo != NULL, "pOIBindInfo == NULL\n"); ok(!grfPI, "grfPI = %x\n", grfPI); @@ -619,7 +623,9 @@ static HRESULT WINAPI Protocol_Start(IInternetProtocol *iface, LPCWSTR szUrl, ok(hres == S_OK, "ReportData failed: %08x\n", hres); hres = IInternetProtocolSink_ReportResult(pOIProtSink, S_OK, 0, NULL); - ok(hres == S_OK, "ReportResult failed: %08x\n", hres); + ok(hres == E_FAIL, "ReportResult failed: %08x\n", hres); + + protocol_sink = NULL; return S_OK; } @@ -689,7 +695,17 @@ static HRESULT WINAPI Protocol_Seek(IInternetProtocol *iface, static HRESULT WINAPI Protocol_LockRequest(IInternetProtocol *iface, DWORD dwOptions) { + HRESULT hres; + CHECK_EXPECT(LockRequest); + + if(protocol_sink) { + hres = IInternetProtocolSink_ReportResult(protocol_sink, S_OK, 0, NULL); + ok(hres == S_OK, "ReportResult failed: %08x\n", hres); + ok(!called_UnlockRequest, "unexpected UnlockRequest\n"); + ok(!called_Terminate, "unexpected Terminate\n"); + } + return S_OK; } diff --git a/dlls/urlmon/binding.c b/dlls/urlmon/binding.c index a74970033b3..56c479d6d2f 100644 --- a/dlls/urlmon/binding.c +++ b/dlls/urlmon/binding.c @@ -64,6 +64,8 @@ typedef enum { #define BINDING_STOPPED 0x0002 #define BINDING_OBJAVAIL 0x0004 #define BINDING_ABORTED 0x0008 +#define BINDING_LOCKING 0x0010 +#define BINDING_TERMINATE 0x0020 typedef struct { IBinding IBinding_iface; @@ -1126,7 +1128,9 @@ static void report_data(Binding *This, DWORD bscf, ULONG progress, ULONG progres HRESULT hres; if(!(This->state & BINDING_LOCKED)) { + This->state |= BINDING_LOCKING; hres = IInternetProtocolEx_LockRequest(&This->protocol->IInternetProtocolEx_iface, 0); + This->state &= ~BINDING_LOCKING; if(SUCCEEDED(hres)) This->state |= BINDING_LOCKED; } @@ -1142,6 +1146,13 @@ static void report_data(Binding *This, DWORD bscf, ULONG progress, ULONG progres hres = IBindStatusCallback_OnDataAvailable(This->callback, bscf, progress, &formatetc, &stgmed); + + if(This->state & BINDING_TERMINATE) { + stop_binding(This, This->hres, NULL); + IInternetProtocolEx_Terminate(&This->protocol->IInternetProtocolEx_iface, 0); + return; + } + if(hres != S_OK) { if(This->download_state != END_DOWNLOAD) { This->download_state = END_DOWNLOAD; @@ -1177,6 +1188,18 @@ static HRESULT WINAPI InternetProtocolSink_ReportResult(IInternetProtocolSink *i TRACE("(%p)->(%08x %d %s)\n", This, hrResult, dwError, debugstr_w(szResult)); + /* Make sure we don't call stop_binding before available data has been reported, + as some custom protocol handlers (in Zombie Army Trilogy launcher and similar) + call ReportResult within LockRequest, in report_data, before OnDataAvailable + has been called. Stopping or terminating the binding there causes an invalid + memory access later. + */ + if(This->state & BINDING_LOCKING) { + This->hres = hrResult; + This->state |= BINDING_TERMINATE; + return S_OK; + } + stop_binding(This, hrResult, szResult); IInternetProtocolEx_Terminate(&This->protocol->IInternetProtocolEx_iface, 0); -- 2.34.1
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=104871 Your paranoid android. === w8 (32 bit report) === mshtml: htmldoc.c:2557: Test failed: unexpected call UpdateUI htmldoc.c:2869: Test failed: unexpected call Exec_UPDATECOMMANDS htmldoc.c:350: Test failed: expected Exec_SETTITLE htmldoc.c:2875: Test failed: unexpected call Exec_SETTITLE
Hi Rémi, On 1/4/22 21:31, Rémi Bernon wrote:
The HTML launcher of several Rebellion games (all probably based on the same code) use a custom protocol handler which calls ReportResult in LockRequest, causing an invalid memory access when OnDataAvailable callback is called, as the binding has already been stopped and terminated.
Wine-Bug:https://bugs.winehq.org/show_bug.cgi?id=46213 Wine-Bug:https://bugs.winehq.org/show_bug.cgi?id=52286 Signed-off-by: Rémi Bernon<rbernon(a)codeweavers.com> ---
v2: Use an even more ad-hoc flag to detect LockRequest calls during ReportData.
dlls/mshtml/tests/htmldoc.c | 18 +++++++++++++++++- dlls/urlmon/binding.c | 23 +++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-)
Those tests in htmldoc.c are a few layers above the problem, so they are not very informative. I did a quick experiment (see the attachment) with more precise test and an ad-hoc fix. It still needs better understanding, but does it help in your case? Thanks, Jacek
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=104872 Your paranoid android. === w8adm (32 bit report) === urlmon: protocol.c:1186: Test failed: hrResult = 800c0005, expected: 00000000 protocol.c:3446: Test failed: wait timed out protocol.c:3450: Test failed: expected ReportData protocol: Timeout === w1064v1809 (32 bit report) === urlmon: protocol.c:1178: Test failed: unexpected call ReportResult protocol.c:1186: Test failed: hrResult = 00000000, expected: 80004004 protocol.c:3320: Test failed: Read failed: 00000001 === w10pro64_he (64 bit report) === urlmon: protocol.c:1178: Test failed: unexpected call ReportResult protocol.c:1186: Test failed: hrResult = 00000000, expected: 80004004 protocol.c:3320: Test failed: Read failed: 00000001
On 1/4/22 23:23, Jacek Caban wrote:
Hi Rémi,
On 1/4/22 21:31, Rémi Bernon wrote:
The HTML launcher of several Rebellion games (all probably based on the same code) use a custom protocol handler which calls ReportResult in LockRequest, causing an invalid memory access when OnDataAvailable callback is called, as the binding has already been stopped and terminated.
Wine-Bug:https://bugs.winehq.org/show_bug.cgi?id=46213 Wine-Bug:https://bugs.winehq.org/show_bug.cgi?id=52286 Signed-off-by: Rémi Bernon<rbernon(a)codeweavers.com> ---
v2: Use an even more ad-hoc flag to detect LockRequest calls during ReportData.
dlls/mshtml/tests/htmldoc.c | 18 +++++++++++++++++- dlls/urlmon/binding.c | 23 +++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-)
Those tests in htmldoc.c are a few layers above the problem, so they are not very informative. I did a quick experiment (see the attachment) with more precise test and an ad-hoc fix. It still needs better understanding, but does it help in your case?
Thanks,
Jacek
Hi Jacek, I must admit that I have very little idea how all this urlmon thing works or does. Your patch only partially fix the launcher issue (it doesn't crash anymore but it still doesn't show any window). As far as I understand, the custom protocol handler is: 1) Calling ReportResult within LockRequest. 2) Expecting OnDataAvailable to be called after. 3) Expecting OnStopBinding to be called after. I believe your patch doesn't do 3)? I'm not sure it expects this to happen while ReportData is called, but it's how it usually happens, and it definitely expect OnStopBinding to be called after ReportResult has been, to continue. It also causes ReportResult to return E_FAIL when called within LockRequest, which is not what native does (at least it's not with the mshtml version of the test). Cheers, -- Rémi Bernon <rbernon(a)codeweavers.com>
On 1/5/22 08:22, Rémi Bernon wrote:
On 1/4/22 23:23, Jacek Caban wrote:
Hi Rémi,
On 1/4/22 21:31, Rémi Bernon wrote:
The HTML launcher of several Rebellion games (all probably based on the same code) use a custom protocol handler which calls ReportResult in LockRequest, causing an invalid memory access when OnDataAvailable callback is called, as the binding has already been stopped and terminated.
Wine-Bug:https://bugs.winehq.org/show_bug.cgi?id=46213 Wine-Bug:https://bugs.winehq.org/show_bug.cgi?id=52286 Signed-off-by: Rémi Bernon<rbernon(a)codeweavers.com> ---
v2: Use an even more ad-hoc flag to detect LockRequest calls during ReportData.
dlls/mshtml/tests/htmldoc.c | 18 +++++++++++++++++- dlls/urlmon/binding.c | 23 +++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-)
Those tests in htmldoc.c are a few layers above the problem, so they are not very informative. I did a quick experiment (see the attachment) with more precise test and an ad-hoc fix. It still needs better understanding, but does it help in your case?
Thanks,
Jacek
Hi Jacek,
I must admit that I have very little idea how all this urlmon thing works or does. Your patch only partially fix the launcher issue (it doesn't crash anymore but it still doesn't show any window).
As far as I understand, the custom protocol handler is:
1) Calling ReportResult within LockRequest. 2) Expecting OnDataAvailable to be called after. 3) Expecting OnStopBinding to be called after.
I believe your patch doesn't do 3)? I'm not sure it expects this to happen while ReportData is called, but it's how it usually happens, and it definitely expect OnStopBinding to be called after ReportResult has been, to continue.
Sure, but I suspect that it should be handled one layer bellow what your patch does. bindprot.c is between Binding object and protocol itself and it already has some tools to do deal with such cases. For example, the attached patch fixes Zombie Army Trilogy and is close to what I'd expect. I'd like to do some more tests to understand how exactly it is supposed to work.
It also causes ReportResult to return E_FAIL when called within LockRequest, which is not what native does (at least it's not with the mshtml version of the test).
That matched what my test showed, but it seems that it fails on some Windows, so there was probably a problem with the test itself. Thanks, Jacek
On 1/5/22 16:49, Jacek Caban wrote:
On 1/5/22 08:22, Rémi Bernon wrote:
On 1/4/22 23:23, Jacek Caban wrote:
Hi Rémi,
On 1/4/22 21:31, Rémi Bernon wrote:
The HTML launcher of several Rebellion games (all probably based on the same code) use a custom protocol handler which calls ReportResult in LockRequest, causing an invalid memory access when OnDataAvailable callback is called, as the binding has already been stopped and terminated.
Wine-Bug:https://bugs.winehq.org/show_bug.cgi?id=46213 Wine-Bug:https://bugs.winehq.org/show_bug.cgi?id=52286 Signed-off-by: Rémi Bernon<rbernon(a)codeweavers.com> ---
v2: Use an even more ad-hoc flag to detect LockRequest calls during ReportData.
dlls/mshtml/tests/htmldoc.c | 18 +++++++++++++++++- dlls/urlmon/binding.c | 23 +++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-)
Those tests in htmldoc.c are a few layers above the problem, so they are not very informative. I did a quick experiment (see the attachment) with more precise test and an ad-hoc fix. It still needs better understanding, but does it help in your case?
Thanks,
Jacek
Hi Jacek,
I must admit that I have very little idea how all this urlmon thing works or does. Your patch only partially fix the launcher issue (it doesn't crash anymore but it still doesn't show any window).
As far as I understand, the custom protocol handler is:
1) Calling ReportResult within LockRequest. 2) Expecting OnDataAvailable to be called after. 3) Expecting OnStopBinding to be called after.
I believe your patch doesn't do 3)? I'm not sure it expects this to happen while ReportData is called, but it's how it usually happens, and it definitely expect OnStopBinding to be called after ReportResult has been, to continue.
Sure, but I suspect that it should be handled one layer bellow what your patch does. bindprot.c is between Binding object and protocol itself and it already has some tools to do deal with such cases. For example, the attached patch fixes Zombie Army Trilogy and is close to what I'd expect. I'd like to do some more tests to understand how exactly it is supposed to work.
It also causes ReportResult to return E_FAIL when called within LockRequest, which is not what native does (at least it's not with the mshtml version of the test).
That matched what my test showed, but it seems that it fails on some Windows, so there was probably a problem with the test itself.
A better test confirmed my theory. I sent a series that fixes the problem for me. Thanks, Jacek
participants (3)
-
Jacek Caban -
Marvin -
Rémi Bernon