[Bug 43377] New: valgrind shows an invalid read in dlls/msxml3/tests/ domdoc.c

https://bugs.winehq.org/show_bug.cgi?id=43377

Bug ID: 43377
           Summary: valgrind shows an invalid read in
                    dlls/msxml3/tests/domdoc.c
           Product: Wine
           Version: 2.12
          Hardware: x86
                OS: Linux
            Status: NEW
          Keywords: download, source, testcase, valgrind
          Severity: normal
          Priority: P2
         Component: msxml3
          Assignee: [e-mail]
          Reporter: [e-mail]
      Distribution: Debian

../../../tools/runtest -q -P wine -T ../../.. -M msxml3.dll -p
msxml3_test.exe.so domdoc && touch domdoc.ok
==30037== Invalid free() / delete / delete[] / realloc()
==30037==    at 0x7BC510DB: notify_free (heap.c:262)
==30037==    by 0x7BC556AC: RtlFreeHeap (heap.c:1762)
==30037==    by 0x5F97B28: heap_free (msxml_private.h:189)
==30037==    by 0x5F9837F: free_properties (domdoc.c:360)
==30037==    by 0x5F98CA3: xmldoc_release_refs (domdoc.c:624)
==30037==    by 0x5F98CEA: xmldoc_release (domdoc.c:635)
==30037==    by 0x5FC36D8: destroy_xmlnode (node.c:1481)
==30037==    by 0x5F99C3C: domdoc_Release (domdoc.c:966)
==30037==    by 0x4A525C0: IXMLDOMDocument2_Release (msxml2.h:4413)
==30037==    by 0x4A525C0: test_get_ownerDocument (???:0)
==30037==    by 0x4A76882: func_domdoc (domdoc.c:12609)
==30037==    by 0x4AB1624: run_test (test.h:603)
==30037==    by 0x4AB1A83: main (test.h:687)
==30037==  Address 0x48e0820 is 18 bytes after a recently re-allocated block of
size 46 alloc'd
==30037==    at 0x7BC51061: notify_alloc (heap.c:254)
==30037==    by 0x7BC5554F: RtlAllocateHeap (heap.c:1716)
==30037==    by 0x5608BD0: strdupW (freetype.c:1098)
==30037==    by 0x560A0ED: load_face (freetype.c:1650)
==30037==    by 0x560A862: load_font_list_from_cache (freetype.c:1778)
==30037==    by 0x56132B5: WineEngInit (freetype.c:4402)
==30037==    by 0x5622222: DllMain (gdiobj.c:677)
==30037==    by 0x5641491: __wine_spec_dll_entry (dll_entry.c:40)
==30037==    by 0x7BC57861: ??? (loader.c:150)
==30037==    by 0x7BC5A291: MODULE_InitDLL (loader.c:1121)
==30037==    by 0x7BC5A7E1: process_attach (loader.c:1222)
==30037==    by 0x7BC5A728: process_attach (loader.c:1210)
==30037==    by 0x7BC5A728: process_attach (loader.c:1210)
==30037==    by 0x7BC5A728: process_attach (loader.c:1210)
==30037==    by 0x7BC5A728: process_attach (loader.c:1210)
==30037==    by 0x7BC5FB42: attach_process_dlls (loader.c:3011)
==30037==    by 0x40413EC: ??? (port.c:78)
==30037==

==30037== Invalid free() / delete / delete[] / realloc()
==30037==    at 0x7BC510DB: notify_free (heap.c:262)
==30037==    by 0x7BC556AC: RtlFreeHeap (heap.c:1762)
==30037==    by 0x5F97B28: heap_free (msxml_private.h:189)
==30037==    by 0x5F9839F: free_properties (domdoc.c:362)
==30037==    by 0x5F98CA3: xmldoc_release_refs (domdoc.c:624)
==30037==    by 0x5F98CEA: xmldoc_release (domdoc.c:635)
==30037==    by 0x5FC36D8: destroy_xmlnode (node.c:1481)
==30037==    by 0x5F99C3C: domdoc_Release (domdoc.c:966)
==30037==    by 0x4A525C0: IXMLDOMDocument2_Release (msxml2.h:4413)
==30037==    by 0x4A525C0: test_get_ownerDocument (???:0)
==30037==    by 0x4A76882: func_domdoc (domdoc.c:12609)
==30037==    by 0x4AB1624: run_test (test.h:603)
==30037==    by 0x4AB1A83: main (test.h:687)
==30037==  Address 0x49065a0 is 4 bytes after a block of size 92 free'd
==30037==    at 0x7BC510DB: notify_free (heap.c:262)
==30037==    by 0x7BC556AC: RtlFreeHeap (heap.c:1762)
==30037==    by 0x55FF13A: FONT_DeleteObject (font.c:814)
==30037==    by 0x5622F04: DeleteObject (gdiobj.c:982)
==30037==    by 0x532BDF5: get_text_metr_size (sysparams.c:482)
==30037==    by 0x532C66E: normalize_nonclientmetrics (sysparams.c:653)
==30037==    by 0x532F087: SystemParametersInfoW (sysparams.c:1610)
==30037==    by 0x53321D2: GetSystemMetrics (sysparams.c:2459)
==30037==    by 0x5331F4F: GetSystemMetrics (sysparams.c:2394)
==30037==    by 0x53183B5: NC_AdjustRectOuter (nonclient.c:115)
==30037==    by 0x53190E0: NC_HandleNCCalcSize (nonclient.c:414)
==30037==    by 0x52CCDB6: DEFWND_DefWinProc (defwnd.c:262)
==30037==    by 0x52CEB60: DefWindowProcW (defwnd.c:1023)
==30037==    by 0x6DFA03C: notif_wnd_proc (bindprot.c:83)
==30037==    by 0x53502FD: ??? (winproc.c:174)
==30037==    by 0x535044C: call_window_proc (winproc.c:245)
==30037==    by 0x5352800: WINPROC_call_window (winproc.c:901)
==30037==    by 0x530DC60: call_window_proc (message.c:2224)
==30037==    by 0x5310C82: send_message (message.c:3269)
==30037==    by 0x5311413: SendMessageW (message.c:3469)
==30037==  Block was alloc'd at
==30037==    at 0x7BC51061: notify_alloc (heap.c:254)
==30037==    by 0x7BC5554F: RtlAllocateHeap (heap.c:1716)
==30037==    by 0x55FE62B: CreateFontIndirectExW (font.c:509)
==30037==    by 0x55FE908: CreateFontIndirectW (font.c:559)
==30037==    by 0x532BD42: get_text_metr_size (sysparams.c:470)
==30037==    by 0x532C66E: normalize_nonclientmetrics (sysparams.c:653)
==30037==    by 0x532F087: SystemParametersInfoW (sysparams.c:1610)
==30037==    by 0x53321D2: GetSystemMetrics (sysparams.c:2459)
==30037==    by 0x5331F4F: GetSystemMetrics (sysparams.c:2394)
==30037==    by 0x53183B5: NC_AdjustRectOuter (nonclient.c:115)
==30037==    by 0x53190E0: NC_HandleNCCalcSize (nonclient.c:414)
==30037==    by 0x52CCDB6: DEFWND_DefWinProc (defwnd.c:262)
==30037==    by 0x52CEB60: DefWindowProcW (defwnd.c:1023)
==30037==    by 0x6DFA03C: notif_wnd_proc (bindprot.c:83)
==30037==    by 0x53502FD: ??? (winproc.c:174)
==30037==    by 0x535044C: call_window_proc (winproc.c:245)
==30037==    by 0x5352800: WINPROC_call_window (winproc.c:901)
==30037==    by 0x530DC60: call_window_proc (message.c:2224)
==30037==    by 0x5310C82: send_message (message.c:3269)
==30037==    by 0x5311413: SendMessageW (message.c:3469)
==30037==

[Bug 43377] valgrind shows an invalid read in dlls/msxml3/tests/domdoc.c

https://bugs.winehq.org/show_bug.cgi?id=43377

André H.  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #1 from André H.  ---
This one seems highly related to the test crash on wine64, which happens in
test_get_ownerDocument at the end at IXMLDOMDocument2_Release(doc_owner)
which can be traced down to:

static void free_properties(domdoc_properties* properties)
{
    if (properties)
    {
        if (properties->schemaCache)
            IXMLDOMSchemaCollection2_Release(properties->schemaCache);
... crash ...

because properties is 0x8b400 I get:
wine: Unhandled page fault on read access to 0x0008b410 at address 0x8b410
(thread 002a), starting debugger...

That's because the exact same properties are already used and freed somewhere
else, which should not happen, because there is copy_properties(), but I found
a  candidate:

static HRESULT attach_xmldoc(domdoc *This, xmlDocPtr xml )
{
    release_namespaces(This);

if(This->node.node)
    {
        priv_from_xmlDocPtr(get_doc(This))->properties = NULL;
        if (xmldoc_release(get_doc(This)) != 0)
            priv_from_xmlDocPtr(get_doc(This))->properties =
                copy_properties(This->properties);
    }

This->node.node = (xmlNodePtr) xml;

if(This->node.node)
    {
        xmldoc_add_ref(get_doc(This));
        priv_from_xmlDocPtr(get_doc(This))->properties = This->properties;

changing this to
        priv_from_xmlDocPtr(get_doc(This))->properties =
copy_properties(This->properties);
works fine here, I'll send a patch

[Bug 43377] valgrind shows an invalid read in dlls/msxml3/tests/domdoc.c

https://bugs.winehq.org/show_bug.cgi?id=43377

Serge Gautherie  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |winehq-bugs_serge_180716@ga
                   |                            |utherie.fr

[Bug 43377] valgrind shows an invalid read in dlls/msxml3/tests/domdoc.c

https://bugs.winehq.org/show_bug.cgi?id=43377

--- Comment #2 from Serge Gautherie  ---
This bug affects ReactOS test run too:
permanent invalid free reported, plus intermittent heap corruption detected.
See https://jira.reactos.org/browse/CORE-14579 .

Thomas Faber commented:
"[Adding copy_properties()] fixes it for me ... but that's probably gonna cause
a leak instead. Someone needs to review the lifetime management of those
properties objects."

[Bug 43377] msxml3:domdoc Crashes in wow64 due to double free of properties

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=43377

François Gouget  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|valgrind shows an invalid   |msxml3:domdoc Crashes in
                   |read in                     |wow64 due to double free of
                   |dlls/msxml3/tests/domdoc.c  |properties
                 CC|                            |[e-mail]

[Bug 43377] msxml3:domdoc Crashes in wow64 due to double free of properties

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=43377

--- Comment #3 from François Gouget  ---
The crash is pretty systematic in wow64 builds nowadays.
(it did succeed for a while probably due to layout changes)

https://test.winehq.org/data/patterns.html#msxml3:domdoc

[Bug 43377] msxml3:domdoc Crashes in wow64 due to double free of properties

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=43377

François Gouget  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #4 from François Gouget  ---
The msxml3:domdoc crash is fixed:

commit 0e87427500b1564ebdaa0a7ad7f627dce2762be9
Author: Francois Gouget 
Date:   Thu May 6 18:08:06 2021 +0300

msxml3: Refcount the domdoc/xmldoc properties.

Multiple domdoc and xmlDoc objects may need to share a common
    properties object but may be released independently. So add a
    reference count on the properties object.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=43377
    Signed-off-by: Francois Gouget 
    Signed-off-by: Nikolay Sivov 
    Signed-off-by: Alexandre Julliard

(unfortunately another bug was introduced in the test a couple of days later
that causes it to crash on Windows this time)

[Bug 43377] msxml3:domdoc Crashes in wow64 due to double free of properties

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=43377

Gijs Vermeulen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
      Fixed by SHA1|                            |0e87427500b1564ebdaa0a7ad7f
                   |                            |627dce2762be9

[Bug 43377] msxml3:domdoc Crashes in wow64 due to double free of properties

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=43377

Serge Gautherie  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|winehq-bugs_serge_180716@ga |
                   |utherie.fr                  |

[Bug 43377] msxml3:domdoc Crashes in wow64 due to double free of properties

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=43377

Alexandre Julliard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #5 from Alexandre Julliard  ---
Closing bugs fixed in 6.9.