[PATCH 1/5] msxml3: When writing to stream \0 terminate output.

[Dmitry Timoshkov]

This fixes heap corruption when running the domdoc tests.

Signed-off-by: Dmitry Timoshkov 
---
 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);

[Marvin]

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"

[Nikolay Sivov]

On 4/27/21 10:48 AM, Dmitry Timoshkov wrote:
> This fixes heap corruption when running the domdoc tests.
> Signed-off-by: Dmitry Timoshkov
>
> 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.

[Dmitry Timoshkov]

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

[Nikolay Sivov]

On 4/27/21 11:35 AM, Dmitry Timoshkov wrote:
> Nikolay Sivov  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.

[Dmitry Timoshkov]

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