And clear it if the call failed.
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@codeweavers.com ---
I tried to keep the changes as simple as possible for the code freeze, but overall the call sequence doesn't look completely right compared to native, although it may not matter too much.
The patches are fixing a regression with several launchers, and as they are quite ad-hoc, hopefully they will not have any impact on anything else...
dlls/urlmon/binding.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/dlls/urlmon/binding.c b/dlls/urlmon/binding.c index a74970033b3..b7bea04c4d1 100644 --- a/dlls/urlmon/binding.c +++ b/dlls/urlmon/binding.c @@ -1126,9 +1126,10 @@ static void report_data(Binding *This, DWORD bscf, ULONG progress, ULONG progres HRESULT hres;
if(!(This->state & BINDING_LOCKED)) { + This->state |= BINDING_LOCKED; hres = IInternetProtocolEx_LockRequest(&This->protocol->IInternetProtocolEx_iface, 0); - if(SUCCEEDED(hres)) - This->state |= BINDING_LOCKED; + if(FAILED(hres)) + This->state &= ~BINDING_LOCKED; }
hres = This->stgmed_obj->vtbl->fill_stgmed(This->stgmed_obj, &stgmed);
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@codeweavers.com --- dlls/mshtml/tests/htmldoc.c | 18 +++++++++++++++++- dlls/urlmon/binding.c | 20 ++++++++++++++++++++ 2 files changed, 37 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 b7bea04c4d1..7e8d33fdf97 100644 --- a/dlls/urlmon/binding.c +++ b/dlls/urlmon/binding.c @@ -64,6 +64,7 @@ typedef enum { #define BINDING_STOPPED 0x0002 #define BINDING_OBJAVAIL 0x0004 #define BINDING_ABORTED 0x0008 +#define BINDING_TERMINATE 0x0010
typedef struct { IBinding IBinding_iface; @@ -1143,6 +1144,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; @@ -1178,6 +1186,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_LOCKED) { + This->hres = hrResult; + This->state |= BINDING_TERMINATE; + return S_OK; + } + stop_binding(This, hrResult, szResult);
IInternetProtocolEx_Terminate(&This->protocol->IInternetProtocolEx_iface, 0);
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=104861
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
=== debian11 (32 bit report) ===
urlmon: url: Timeout
=== debian11 (32 bit Chinese:China report) ===
urlmon: url: Timeout
=== debian11 (32 bit WoW report) ===
urlmon: url: Timeout
=== debian11 (64 bit WoW report) ===
urlmon: url: Timeout
-
Marvin -
Rémi Bernon