[PATCH v3 1/3] msxml3: Implement ::get_attributes() for "xml" processing instruction node. (Resend)

[Dmitry Timoshkov]

Signed-off-by: Dmitry Timoshkov 
---
 dlls/msxml3/pi.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 143 insertions(+), 1 deletion(-)

diff --git a/dlls/msxml3/pi.c b/dlls/msxml3/pi.c
index cea95708087..f37f3c01630 100644
--- a/dlls/msxml3/pi.c
+++ b/dlls/msxml3/pi.c
@@ -34,6 +34,7 @@
 #include "ole2.h"
 #include "msxml6.h"

+#include "xmlparser.h"
 #include "msxml_private.h"

#include "wine/debug.h"
@@ -287,6 +288,138 @@ static HRESULT WINAPI dom_pi_get_nextSibling(
     return node_get_next_sibling(&This->node, domNode);
 }

+static HRESULT xml_get_value(xmlChar **p, xmlChar **value)
+{
+    xmlChar *v;
+    int len;
+
+    while (isspace(**p)) *p += 1;
+    if (**p != '=') return XML_E_MISSINGEQUALS;
+    *p += 1;
+
+    while (isspace(**p)) *p += 1;
+    if (**p != '"') return XML_E_MISSINGQUOTE;
+    *p += 1;
+
+    v = *p;
+    while (**p && **p != '"') *p += 1;
+    if (!**p) return XML_E_EXPECTINGCLOSEQUOTE;
+    len = *p - v;
+    if (!len) return XML_E_MISSINGNAME;
+    *p += 1;
+
+    *value = heap_alloc(len + 1);
+    if (!*value) return E_OUTOFMEMORY;
+    memcpy(*value, v, len);
+    *(*value + len) = 0;
+
+    return S_OK;
+}
+
+static void set_prop(xmlNodePtr node, xmlAttrPtr attr)
+{
+    xmlAttrPtr prop;
+
+    if (!node->properties)
+    {
+        node->properties = attr;
+        return;
+    }
+
+    prop = node->properties;
+    while (prop->next) prop = prop->next;
+
+    prop->next = attr;
+}
+
+static HRESULT parse_xml_decl(xmlNodePtr node)
+{
+    xmlChar *version, *encoding, *standalone, *p;
+    xmlAttrPtr attr;
+    HRESULT hr = S_OK;
+
+    if (!node->content) return S_OK;
+
+    version = encoding = standalone = NULL;
+
+    p = node->content;
+
+    while (*p)
+    {
+        while (isspace(*p)) p++;
+        if (!*p) break;
+
+        if (!strncmp((const char *)p, "version", 7))
+        {
+            p += 7;
+            if ((hr = xml_get_value(&p, &version)) != S_OK) goto fail;
+        }
+        else if (!strncmp((const char *)p, "encoding", 8))
+        {
+            p += 8;
+            if ((hr = xml_get_value(&p, &encoding)) != S_OK) goto fail;
+        }
+        else if (!strncmp((const char *)p, "standalone", 10))
+        {
+            p += 10;
+            if ((hr = xml_get_value(&p, &standalone)) != S_OK) goto fail;
+        }
+        else
+        {
+            FIXME("unexpected XML attribute %s\n", debugstr_a((const char *)p));
+            hr = XML_E_UNEXPECTED_ATTRIBUTE;
+            goto fail;
+        }
+    }
+
+    /* xmlSetProp/xmlSetNsProp accept only nodes of type XML_ELEMENT_NODE,
+     * so we have to create and assign attributes to a node by hand.
+     */
+
+    if (version)
+    {
+        attr = xmlSetNsProp(NULL, NULL, (const xmlChar *)"version", version);
+        if (attr)
+        {
+            attr->doc = node->doc;
+            set_prop(node, attr);
+        }
+        else hr = E_OUTOFMEMORY;
+    }
+    if (encoding)
+    {
+        attr = xmlSetNsProp(NULL, NULL, (const xmlChar *)"encoding", encoding);
+        if (attr)
+        {
+            attr->doc = node->doc;
+            set_prop(node, attr);
+        }
+        else hr = E_OUTOFMEMORY;
+    }
+    if (standalone)
+    {
+        attr = xmlSetNsProp(NULL, NULL, (const xmlChar *)"standalone", standalone);
+        if (attr)
+        {
+            attr->doc = node->doc;
+            set_prop(node, attr);
+        }
+        else hr = E_OUTOFMEMORY;
+    }
+
+fail:
+    if (hr != S_OK)
+    {
+        xmlFreePropList(node->properties);
+        node->properties = NULL;
+    }
+
+    heap_free(version);
+    heap_free(encoding);
+    heap_free(standalone);
+    return hr;
+}
+
 static HRESULT WINAPI dom_pi_get_attributes(
     IXMLDOMProcessingInstruction *iface,
     IXMLDOMNamedNodeMap** map)
@@ -307,7 +440,16 @@ static HRESULT WINAPI dom_pi_get_attributes(

if (!strcmpW(name, xmlW))
     {
-        FIXME("created dummy map for <?xml ?>\n");
+        if (!This->node.node->properties)
+        {
+            hr = parse_xml_decl(This->node.node);
+            if (hr != S_OK)
+            {
+                SysFreeString(name);
+                return S_FALSE;
+            }
+        }
+
         *map = create_nodemap(This->node.node, &dom_pi_attr_map);
         SysFreeString(name);
         return S_OK;

[Nikolay Sivov]

On 5/28/21 1:38 PM, Dmitry Timoshkov wrote:
> +static HRESULT xml_get_value(xmlChar **p, xmlChar **value)
> +{
> - xmlChar *v;
> - int len;
> -
> - while (isspace(**p)) *p += 1;
> - if (**p != '=') return XML_E_MISSINGEQUALS;
> - *p += 1;
> -
> - while (isspace(**p)) *p += 1;
> - if (**p != '"') return XML_E_MISSINGQUOTE;
> - *p += 1;
> -
> - v = *p;
> - while (**p && **p != '"') *p += 1;
> - if (!**p) return XML_E_EXPECTINGCLOSEQUOTE;
> - len = *p - v;
> - if (!len) return XML_E_MISSINGNAME;
> - *p += 1;
> -
> - *value = heap_alloc(len + 1);
> - if (!*value) return E_OUTOFMEMORY;
> - memcpy(*value, v, len);
> - *(*value + len) = 0;
> -
> - return S_OK;
>
> +}

Is it useful to be that specific about error codes (and including
xmlparser.h for that)? Since that pi node is a product of already
performed parsing, it seems that it should always be valid anyway. I
don't see them reaching out of dom_pi_get_attributes():
> -
>    FIXME("created dummy map for <?xml ?>\n");
> -
>    if (!This->node.node->properties)
> -
>    {
> -
>        hr = parse_xml_decl(This->node.node);
> -
>        if (hr != S_OK)
> -
>        {
> -
>            SysFreeString(name);
> -
>            return S_FALSE;
> -
>        }
> -
>    }
> -

So it's either S_OK or S_FALSE.

[Dmitry Timoshkov]

Nikolay Sivov  wrote:
> > +static HRESULT xml_get_value(xmlChar **p, xmlChar **value)
> > +{
> > - xmlChar *v;
> > - int len;
> > -
> > - while (isspace(**p)) *p += 1;
> > - if (**p != '=') return XML_E_MISSINGEQUALS;
> > - *p += 1;
> > -
> > - while (isspace(**p)) *p += 1;
> > - if (**p != '"') return XML_E_MISSINGQUOTE;
> > - *p += 1;
> > -
> > - v = *p;
> > - while (**p && **p != '"') *p += 1;
> > - if (!**p) return XML_E_EXPECTINGCLOSEQUOTE;
> > - len = *p - v;
> > - if (!len) return XML_E_MISSINGNAME;
> > - *p += 1;
> > -
> > - *value = heap_alloc(len + 1);
> > - if (!*value) return E_OUTOFMEMORY;
> > - memcpy(*value, v, len);
> > - *(*value + len) = 0;
> > -
> > - return S_OK;
> >
> > +}
>
> Is it useful to be that specific about error codes (and including
> xmlparser.h for that)? Since that pi node is a product of already
> performed parsing, it seems that it should always be valid anyway. I
> don't see them reaching out of dom_pi_get_attributes():

You are probably correct that using such specific error codes is not
very useful, however I wanted to avoid generic E_FAIL, and particular
error values better help in diagnosing parser bugs.
> > -
> >    FIXME("created dummy map for <?xml ?>\n");
> > -
> >    if (!This->node.node->properties)
> > -
> >    {
> > -
> >        hr = parse_xml_decl(This->node.node);
> > -
> >        if (hr != S_OK)
> > -
> >        {
> > -
> >            SysFreeString(name);
> > -
> >            return S_FALSE;
> > -
> >        }
> > -
> >    }
> > -
>
> So it's either S_OK or S_FALSE.

Right. Thanks for the helpful review.

[Dmitry Timoshkov]

Dmitry Timoshkov  wrote:
> Nikolay Sivov  wrote:
> > > +static HRESULT xml_get_value(xmlChar **p, xmlChar **value)
> > > +{
> > > - xmlChar *v;
> > > - int len;
> > > -
> > > - while (isspace(**p)) *p += 1;
> > > - if (**p != '=') return XML_E_MISSINGEQUALS;
> > > - *p += 1;
> > > -
> > > - while (isspace(**p)) *p += 1;
> > > - if (**p != '"') return XML_E_MISSINGQUOTE;
> > > - *p += 1;
> > > -
> > > - v = *p;
> > > - while (**p && **p != '"') *p += 1;
> > > - if (!**p) return XML_E_EXPECTINGCLOSEQUOTE;
> > > - len = *p - v;
> > > - if (!len) return XML_E_MISSINGNAME;
> > > - *p += 1;
> > > -
> > > - *value = heap_alloc(len + 1);
> > > - if (!*value) return E_OUTOFMEMORY;
> > > - memcpy(*value, v, len);
> > > - *(*value + len) = 0;
> > > -
> > > - return S_OK;
> > >
> > > +}
> >
> > Is it useful to be that specific about error codes (and including
> > xmlparser.h for that)? Since that pi node is a product of already
> > performed parsing, it seems that it should always be valid anyway. I
> > don't see them reaching out of dom_pi_get_attributes():
>
> You are probably correct that using such specific error codes is not
> very useful, however I wanted to avoid generic E_FAIL, and particular
> error values better help in diagnosing parser bugs.

Also, according to the tests more verbose parse errors could be returned
by ::createProcessingInstruction(), and if desirable the parsing step
might be executed there. However, moving the parsing step requires large
restructuring of the node creation logic, and I'd prefer to not depend
these patches on it.

[Nikolay Sivov]

Signed-off-by: Nikolay Sivov