[PATCH 0/2] MR10202: mxsml3/tests: Add tests for put_text.
calling IXMLDOMNode_put_text on a text node leads to xmlTextSetContent in libxml2 which duplicates the text (xmlStr[n]dup) and frees the old content before setting the new calling IXMLDOMElement_put_text on an element node leads to xmlNodeParseContent in libxml2 which sets the text on the xmlNode's child and calls xmlFreeNodeList to free the old content an IXMLDOMNode reference to that internal text node holds a pointer to that freed content and crashes on IXMLDOMNode_Release <open> <-- IXMLDOMElement_put_text -> xmlNodeParseContent frees old 'content' held by IXMLDOMNode below sesame <-- IXMLDOMNode_Release on this node double-frees 'content' freed above </open> -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10202
From: Daniel Lehman <dlehman25@gmail.com> calling IXMLDOMNode_put_text on a text node leads to xmlTextSetContent in libxml2 which duplicates the text (xmlStr[n]dup) and frees the old content before setting the new calling IXMLDOMElement_put_text on an element node leads to xmlNodeParseContent in libxml2 which sets the text on the xmlNode's child and calls xmlFreeNodeList to free the old content an IXMLDOMNode reference to that internal text node holds a pointer to that freed content and crashes on IXMLDOMNode_Release <open> <-- IXMLDOMElement_put_text -> xmlNodeParseContent frees old 'content' held by IXMLDOMNode below sesame <-- IXMLDOMNode_Release on this node double-frees 'content' freed above </open> --- dlls/msxml3/tests/domdoc.c | 170 +++++++++++++++++++++++++++++++++++++ 1 file changed, 170 insertions(+) diff --git a/dlls/msxml3/tests/domdoc.c b/dlls/msxml3/tests/domdoc.c index 9c16436bc1c..913571ad263 100644 --- a/dlls/msxml3/tests/domdoc.c +++ b/dlls/msxml3/tests/domdoc.c @@ -14438,6 +14438,175 @@ static void test_validate_on_parse_values(void) } } +static void test_put_text(void) +{ + IXMLDOMElement *element; + IXMLDOMNamedNodeMap *map; + IXMLDOMDocument2 *doc; + IXMLDOMNode *node, *node2; + VARIANT_BOOL b; + BSTR str; + const WCHAR *expected; + HRESULT hr; + + doc = create_document(&IID_IXMLDOMDocument2); + + b = VARIANT_FALSE; + str = SysAllocString(L"<?xml version='1.0' encoding='UTF-16'?>\n<open what=\"door\">sesame</open>\n"); + hr = IXMLDOMDocument2_loadXML(doc, str, &b); + ok(hr == S_OK, "Unable to create instance hr %#lx.\n", hr); + SysFreeString(str); + + element = NULL; + hr = IXMLDOMDocument2_get_documentElement(doc, &element); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + node = NULL; + hr = IXMLDOMElement_get_firstChild(element, &node); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + /* put_text on text node */ + hr = IXMLDOMNode_put_text(node, _bstr_("SESAME")); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + str = NULL; + hr = IXMLDOMElement_get_xml(element, &str); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + expected = L"<open what=\"door\">SESAME</open>"; + ok(!lstrcmpW(str, expected), "Incorrect element string, got '%s'\n", wine_dbgstr_w(str)); + SysFreeString(str); + + /* put_text on attribute node */ + map = NULL; + hr = IXMLDOMElement_get_attributes(element, &map); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + node2 = NULL; + hr = IXMLDOMNamedNodeMap_get_item(map, 0, &node2); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + hr = IXMLDOMNode_put_text(node2, _bstr_("window")); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + /* put_text on element parent */ + hr = IXMLDOMElement_put_text(element, _bstr_("sesame")); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + str = NULL; + hr = IXMLDOMElement_get_xml(element, &str); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + expected = L"<open what=\"window\">sesame</open>"; + ok(!lstrcmpW(str, expected), "Incorrect element string, got '%s'\n", wine_dbgstr_w(str)); + SysFreeString(str); + + /* calling put_text on text node again doesn't affect element */ + hr = IXMLDOMNode_put_text(node, _bstr_("SeSaMe")); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + str = NULL; + hr = IXMLDOMNode_get_text(node, &str); + expected = L"SeSaMe"; + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + ok(!lstrcmpW(str, expected), "Incorrect node string, got '%s'\n", wine_dbgstr_w(str)); + SysFreeString(str); + + str = NULL; + hr = IXMLDOMElement_get_xml(element, &str); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + expected = L"<open what=\"window\">sesame</open>"; + todo_wine + ok(!lstrcmpW(str, expected), "Incorrect element string, got '%s'\n", wine_dbgstr_w(str)); + SysFreeString(str); + + IXMLDOMNode_Release(node2); + IXMLDOMNamedNodeMap_Release(map); + if (0) /* crashes on wine with IXMLDOMElement_put_text */ + IXMLDOMNode_Release(node); + IXMLDOMElement_Release(element); + IXMLDOMDocument2_Release(doc); + + /* put_text on element when children is not a text node */ + doc = create_document(&IID_IXMLDOMDocument2); + + b = VARIANT_FALSE; + str = SysAllocString(L"<?xml version='1.0' encoding='UTF-16'?>\n" + "<open>\r\n" + "\t<inside>sesame</inside>\r\n" + "</open>\n"); + hr = IXMLDOMDocument2_loadXML(doc, str, &b); + ok(hr == S_OK, "Unable to create instance hr %#lx.\n", hr); + SysFreeString(str); + + element = NULL; + hr = IXMLDOMDocument2_get_documentElement(doc, &element); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + str = NULL; + hr = IXMLDOMElement_get_xml(element, &str); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + node = NULL; + hr = IXMLDOMElement_get_firstChild(element, &node); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + expected = L"<open>\r\n" + "\t<inside>sesame</inside>\r\n" + "</open>"; + ok(!lstrcmpW(str, expected), "Incorrect element string, got '%s'\n", wine_dbgstr_w(str)); + SysFreeString(str); + + hr = IXMLDOMElement_put_text(element, _bstr_("sesame")); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + str = NULL; + hr = IXMLDOMElement_get_xml(element, &str); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + expected = L"<open>sesame</open>"; + ok(!lstrcmpW(str, expected), "Incorrect element string, got '%s'\n", wine_dbgstr_w(str)); + SysFreeString(str); + + IXMLDOMNode_Release(node); + IXMLDOMElement_Release(element); + IXMLDOMDocument2_Release(doc); + + /* put_text on element with no children */ + doc = create_document(&IID_IXMLDOMDocument2); + + b = VARIANT_FALSE; + str = SysAllocString(L"<?xml version='1.0' encoding='UTF-16'?>\n" + "<open></open>\n"); + hr = IXMLDOMDocument2_loadXML(doc, str, &b); + ok(hr == S_OK, "Unable to create instance hr %#lx.\n", hr); + SysFreeString(str); + + element = NULL; + hr = IXMLDOMDocument2_get_documentElement(doc, &element); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + node = NULL; + hr = IXMLDOMElement_get_firstChild(element, &node); + ok(hr == S_FALSE, "Unexpected hr %#lx.\n", hr); + + hr = IXMLDOMElement_put_text(element, _bstr_("sesame")); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + str = NULL; + hr = IXMLDOMElement_get_xml(element, &str); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + expected = L"<open>sesame</open>"; + ok(!lstrcmpW(str, expected), "Incorrect element string, got '%s'\n", wine_dbgstr_w(str)); + SysFreeString(str); + + IXMLDOMElement_Release(element); + IXMLDOMDocument2_Release(doc); + free_bstrs(); +} + static void test_indent(void) { HRESULT hr; @@ -14596,6 +14765,7 @@ START_TEST(domdoc) test_xsltext(); test_max_element_depth_values(); test_get_parentNode(); + test_put_text(); if (is_clsid_supported(&CLSID_MXNamespaceManager40, &IID_IMXNamespaceManager)) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10202
From: Daniel Lehman <dlehman25@gmail.com> Calling put_text on IXMLDOMElement calls xmlNodeParseContent which deletes the child Text node with xmlFreeNodeList An IXMLDOMNode created that references that Text node will then double-free it on Release, probably leading to a crash The fix here is to possibly pre-free the Text node child if there's no COM reference to it (xmlNodePtr->_private == 0) and set children = NULL so that libxml2 won't free it If there's a COM reference to that Text node, it'll free it on Release If there's none, IXMLDOMElement_put_text frees the memory The tests show that once IXMLDOMElement_put_text is called, any IXMLDOMText object is detached from the tree and doesn't need updated --- dlls/msxml3/element.c | 14 ++++++++++++++ dlls/msxml3/msxml_private.h | 1 + dlls/msxml3/node.c | 2 +- dlls/msxml3/tests/domdoc.c | 2 -- 4 files changed, 16 insertions(+), 3 deletions(-) diff --git a/dlls/msxml3/element.c b/dlls/msxml3/element.c index fd9a2973cd8..e38aea723dd 100644 --- a/dlls/msxml3/element.c +++ b/dlls/msxml3/element.c @@ -405,6 +405,20 @@ static HRESULT WINAPI domelem_put_text( { domelem *This = impl_from_IXMLDOMElement( iface ); TRACE("(%p)->(%s)\n", This, debugstr_w(p)); + + /* libxml2 calls xmlNodeParseContent from node_put_text for Elements + xmlNodeParseContent calls xmlFreeNodeList on the child Text node + an IXMLDOMTextNode was created from that Text node will crash + on Release because it holds a pointer to that freed node + pre-free and NULL the pointer before-hand to avoid that crash + but only if there's no outstanding references to it */ + if (This->node.node && This->node.node->children && + This->node.node->children->type == XML_TEXT_NODE) + { + if (!node_get_inst_cnt(This->node.node->children)) + xmlFreeNodeList(This->node.node->children); + This->node.node->children = NULL; + } return node_put_text( &This->node, p ); } diff --git a/dlls/msxml3/msxml_private.h b/dlls/msxml3/msxml_private.h index 9709f8247ab..a6b645f5233 100644 --- a/dlls/msxml3/msxml_private.h +++ b/dlls/msxml3/msxml_private.h @@ -247,6 +247,7 @@ extern HRESULT node_transform_node(const xmlnode*,IXMLDOMNode*,BSTR*); extern HRESULT node_transform_node_params(const xmlnode*,IXMLDOMNode*,BSTR*,ISequentialStream*, const struct xslprocessor_params*); extern HRESULT node_create_supporterrorinfo(const tid_t*,void**); +extern int node_get_inst_cnt(xmlNodePtr); extern HRESULT get_domdoc_from_xmldoc(xmlDocPtr xmldoc, IXMLDOMDocument3 **document); diff --git a/dlls/msxml3/node.c b/dlls/msxml3/node.c index 349e154e05c..bc536c87adb 100644 --- a/dlls/msxml3/node.c +++ b/dlls/msxml3/node.c @@ -386,7 +386,7 @@ HRESULT node_get_next_sibling(xmlnode *This, IXMLDOMNode **ret) return get_node(This, "next", This->node->next, ret); } -static int node_get_inst_cnt(xmlNodePtr node) +int node_get_inst_cnt(xmlNodePtr node) { int ret = *(LONG *)&node->_private & NODE_PRIV_REFCOUNT_MASK; xmlNodePtr child; diff --git a/dlls/msxml3/tests/domdoc.c b/dlls/msxml3/tests/domdoc.c index 913571ad263..7c7f4287985 100644 --- a/dlls/msxml3/tests/domdoc.c +++ b/dlls/msxml3/tests/domdoc.c @@ -14517,13 +14517,11 @@ static void test_put_text(void) ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); expected = L"<open what=\"window\">sesame</open>"; - todo_wine ok(!lstrcmpW(str, expected), "Incorrect element string, got '%s'\n", wine_dbgstr_w(str)); SysFreeString(str); IXMLDOMNode_Release(node2); IXMLDOMNamedNodeMap_Release(map); - if (0) /* crashes on wine with IXMLDOMElement_put_text */ IXMLDOMNode_Release(node); IXMLDOMElement_Release(element); IXMLDOMDocument2_Release(doc); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10202
I'm currently in the process of reworking DOM handling, that will make this fixup unnecessary. Could you submit a separate MR just with the tests? We'll definitely need that. Also, "value" property behaves the same regarding creating a new child text node. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10202#note_132045
This merge request was closed by Daniel Lehman. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10202
participants (3)
-
Daniel Lehman -
Daniel Lehman (＠dlehman25)
-
Nikolay Sivov (＠nsivov)