[PATCH 1/5] msxml3: When writing to stream \0 terminate output.
This fixes heap corruption when running the domdoc tests. Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/msxml3/domdoc.c | 12 +++++++++++- dlls/msxml3/tests/domdoc.c | 3 --- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/dlls/msxml3/domdoc.c b/dlls/msxml3/domdoc.c index cf4f0433218..cb34632c203 100644 --- a/dlls/msxml3/domdoc.c +++ b/dlls/msxml3/domdoc.c @@ -2524,8 +2524,18 @@ static int XMLCALL domdoc_stream_save_writecallback(void *ctx, const char *buffe static int XMLCALL domdoc_stream_save_closecallback(void *ctx) { + ULONG written; + HRESULT hr; + + hr = IStream_Write((IStream*)ctx, "\0", 1, &written); + if (hr != S_OK) + { + WARN("stream write error: 0x%08x\n", hr); + hr = -1; + } + IStream_Release((IStream*)ctx); - return 0; + return hr; } static HRESULT WINAPI domdoc_save( diff --git a/dlls/msxml3/tests/domdoc.c b/dlls/msxml3/tests/domdoc.c index 6f8e2b44ed3..63fc57527b4 100644 --- a/dlls/msxml3/tests/domdoc.c +++ b/dlls/msxml3/tests/domdoc.c @@ -8586,7 +8586,6 @@ todo_wine hr = GetHGlobalFromStream(stream, &global); ok(hr == S_OK, "got 0x%08x\n", hr); p = GlobalLock(global); - p[GlobalSize(global)] = 0; ok(!strcmp(p, xml2) || !strcmp(p, xml2_wine), "got %s\n", wine_dbgstr_a(p)); GlobalUnlock(global); @@ -8609,7 +8608,6 @@ todo_wine hr = GetHGlobalFromStream(stream, &global); ok(hr == S_OK, "got 0x%08x\n", hr); p = GlobalLock(global); - p[GlobalSize(global)] = 0; ok(!strcmp(p, xml2) || !strcmp(p, xml2_wine), "got %s\n", wine_dbgstr_a(p)); GlobalUnlock(global); @@ -8631,7 +8629,6 @@ todo_wine ok(hr == S_OK, "got 0x%08x\n", hr); p = GlobalLock(global); - p[GlobalSize(global)] = 0; ok(!strcmp(p, xml3) || !strcmp(p, xml3_wine), "got %s\n", wine_dbgstr_a(p)); GlobalUnlock(global); -- 2.31.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=89395 Your paranoid android. === w2008s64 (32 bit report) === msxml3: domdoc.c:8632: Test failed: got "<?xml version=\"1.0\" standalone=\"yes\"?>\r\n<test/>\r\nest/>\r\n" === w7u_2qxl (32 bit report) === msxml3: domdoc.c:8589: Test failed: got "<?xml version=\"1.0\" encoding=\"windows-1252\"?>\r\n<test/>\r\npen>\r\n" domdoc.c:8611: Test failed: got "<?xml version=\"1.0\" encoding=\"windows-1252\"?>\r\n<test/>\r\npen>\r\n" domdoc.c:8632: Test failed: got "<?xml version=\"1.0\" standalone=\"yes\"?>\r\n<test/>\r\nest/>\r\npen>\r\n" === w7u_adm (32 bit report) === msxml3: domdoc.c:8632: Test failed: got "<?xml version=\"1.0\" standalone=\"yes\"?>\r\n<test/>\r\nest/>\r\n" === w7u_el (32 bit report) === msxml3: domdoc.c:8632: Test failed: got "<?xml version=\"1.0\" standalone=\"yes\"?>\r\n<test/>\r\nest/>\r\n" === wvistau64 (64 bit report) === msxml3: domdoc.c:8589: Test failed: got "<?xml version=\"1.0\" encoding=\"windows-1252\"?>\r\n<test/>\r\npen>\r\n" domdoc.c:8611: Test failed: got "<?xml version=\"1.0\" encoding=\"windows-1252\"?>\r\n<test/>\r\npen>\r\n" domdoc.c:8632: Test failed: got "<?xml version=\"1.0\" standalone=\"yes\"?>\r\n<test/>\r\nest/>\r\npen>\r\n" === w2008s64 (64 bit report) === msxml3: domdoc.c:8589: Test failed: got "<?xml version=\"1.0\" encoding=\"windows-1252\"?>\r\n<test/>\r\npen>\r\nd" domdoc.c:8611: Test failed: got "<?xml version=\"1.0\" encoding=\"windows-1252\"?>\r\n<test/>\r\npen>\r\nd" domdoc.c:8632: Test failed: got "<?xml version=\"1.0\" standalone=\"yes\"?>\r\n<test/>\r\nest/>\r\npen>\r\nd"
On 4/27/21 10:48 AM, Dmitry Timoshkov wrote:
This fixes heap corruption when running the domdoc tests.
Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/msxml3/domdoc.c | 12 +++++++++++- dlls/msxml3/tests/domdoc.c | 3 --- 2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/dlls/msxml3/domdoc.c b/dlls/msxml3/domdoc.c index cf4f0433218..cb34632c203 100644 --- a/dlls/msxml3/domdoc.c +++ b/dlls/msxml3/domdoc.c @@ -2524,8 +2524,18 @@ static int XMLCALL domdoc_stream_save_writecallback(void *ctx, const char *buffe
static int XMLCALL domdoc_stream_save_closecallback(void *ctx) { + ULONG written; + HRESULT hr; + + hr = IStream_Write((IStream*)ctx, "\0", 1, &written); + if (hr != S_OK) + { + WARN("stream write error: 0x%08x\n", hr); + hr = -1; + } + IStream_Release((IStream*)ctx); - return 0; + return hr; } This will need a test on its own, if it really does that. Callback should return 0 or -1, as documented.
Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
static int XMLCALL domdoc_stream_save_closecallback(void *ctx) { + ULONG written; + HRESULT hr; + + hr = IStream_Write((IStream*)ctx, "\0", 1, &written); + if (hr != S_OK) + { + WARN("stream write error: 0x%08x\n", hr); + hr = -1; + } + IStream_Release((IStream*)ctx); - return 0; + return hr; } This will need a test on its own, if it really does that.
It appears that Windows started to do that since Windows 8. What kind of test would you like to see?
Callback should return 0 or -1, as documented.
Yes, and the logic in domdoc_stream_save_closecallback() follows that. -- Dmitry.
On 4/27/21 11:35 AM, Dmitry Timoshkov wrote:
Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
static int XMLCALL domdoc_stream_save_closecallback(void *ctx) { + ULONG written; + HRESULT hr; + + hr = IStream_Write((IStream*)ctx, "\0", 1, &written); + if (hr != S_OK) + { + WARN("stream write error: 0x%08x\n", hr); + hr = -1; + } + IStream_Release((IStream*)ctx); - return 0; + return hr; } This will need a test on its own, if it really does that. It appears that Windows started to do that since Windows 8. What kind of test would you like to see?
The one that shows additional single 0 byte, regardless of document encoding. Or initial stream memory block contents.
Callback should return 0 or -1, as documented. Yes, and the logic in domdoc_stream_save_closecallback() follows that.
Ok, it's confusing. It's more readable to do SUCCEEDED(hr) ? 0 : -1, or similar.
Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
static int XMLCALL domdoc_stream_save_closecallback(void *ctx) { + ULONG written; + HRESULT hr; + + hr = IStream_Write((IStream*)ctx, "\0", 1, &written); + if (hr != S_OK) + { + WARN("stream write error: 0x%08x\n", hr); + hr = -1; + } + IStream_Release((IStream*)ctx); - return 0; + return hr; } This will need a test on its own, if it really does that. It appears that Windows started to do that since Windows 8. What kind of test would you like to see?
The one that shows additional single 0 byte, regardless of document encoding. Or initial stream memory block contents.
Callback should return 0 or -1, as documented. Yes, and the logic in domdoc_stream_save_closecallback() follows that.
Ok, it's confusing. It's more readable to do SUCCEEDED(hr) ? 0 : -1, or similar.
I decided to fix the tests without patching ::save() to avoid not related changes preventing reviewing further patches. Thanks for the help. -- Dmitry.
participants (3)
-
Dmitry Timoshkov -
Marvin -
Nikolay Sivov