Thread: [HACKERS] possible encoding issues with libxml2 functions

On Mon, Feb 20, 2017 at 07:48:18PM +0100, Pavel Stehule wrote:
> Today I played with xml_recv function and with xml processing functions.
>
> xml_recv function ensures correct encoding from document encoding to server
> encoding. But the decl section holds original encoding info - that should
> be obsolete after encoding. Sometimes we solve this issue by removing decl
> section - see the xml_out function.
>
> Sometimes we don't do it - lot of functions uses direct conversion from
> xmltype to xmlChar.

> There are possible two fixes
>
> a) clean decl on input - the encoding info can be removed from decl part
>
> b) use xml_out_internal everywhere before transformation to
> xmlChar. pg_xmlCharStrndup can be good candidate.

I'd prefer (a) if the xml type were a new feature, because no good can come of
storing an encoding in each xml field when we know the actual encoding is the
database encoding.  However, if you implemented (a), we'd still see untreated
values brought over via pg_upgrade.  Therefore, I would try (b) first.  I
suspect the intent of xml_parse() was to implement (b); it will be interesting
to see your test case that malfunctions.

On Sun, Mar 12, 2017 at 08:36:58PM +0100, Pavel Stehule wrote:
> 2017-03-12 0:56 GMT+01:00 Noah Misch <noah@leadboat.com>:
> > On Mon, Feb 20, 2017 at 07:48:18PM +0100, Pavel Stehule wrote:
> > > There are possible two fixes
> > >
> > > a) clean decl on input - the encoding info can be removed from decl part
> > >
> > > b) use xml_out_internal everywhere before transformation to
> > > xmlChar. pg_xmlCharStrndup can be good candidate.
> >
> > I'd prefer (a) if the xml type were a new feature, because no good can
> > come of
> > storing an encoding in each xml field when we know the actual encoding is
> > the
> > database encoding.  However, if you implemented (a), we'd still see
> > untreated
> > values brought over via pg_upgrade.  Therefore, I would try (b) first.  I
> > suspect the intent of xml_parse() was to implement (b); it will be
> > interesting
> > to see your test case that malfunctions.
> >
>
> I looked there again and I found so this issue is related to xpath function
> only
>
> Functions based on xml_parse are working without problems. xpath_internal
> uses own direct xmlCtxtReadMemory without correct encoding sanitation.
>
> so fix is pretty simple

Please add a test case.

> --- a/src/backend/utils/adt/xml.c
> +++ b/src/backend/utils/adt/xml.c
> @@ -3874,9 +3874,11 @@ xpath_internal(text *xpath_expr_text, xmltype *data,
> ArrayType *namespaces,
>         ns_count = 0;
>     }
>
> -   datastr = VARDATA(data);
> -   len = VARSIZE(data) - VARHDRSZ;
> +   datastr = xml_out_internal(data, 0);

Why not use xml_parse() instead of calling xmlCtxtReadMemory() directly?  The
answer is probably in the archives, because someone understood the problem
enough to document "Some XML-related functions may not work at all on
non-ASCII data when the server encoding is not UTF-8. This is known to be an
issue for xpath() in particular." 

> +   len = strlen(datastr);
> +
>     xpath_len = VARSIZE(xpath_expr_text) - VARHDRSZ;
> +

The two lines of functional change don't create a cause for more newlines, so
don't add these two newlines.

2017-03-12 21:57 GMT+01:00 Noah Misch <noah@leadboat.com>:

On Sun, Mar 12, 2017 at 08:36:58PM +0100, Pavel Stehule wrote:
> 2017-03-12 0:56 GMT+01:00 Noah Misch <noah@leadboat.com>:
> > On Mon, Feb 20, 2017 at 07:48:18PM +0100, Pavel Stehule wrote:
> > > There are possible two fixes
> > >
> > > a) clean decl on input - the encoding info can be removed from decl part
> > >
> > > b) use xml_out_internal everywhere before transformation to
> > > xmlChar. pg_xmlCharStrndup can be good candidate.
> >
> > I'd prefer (a) if the xml type were a new feature, because no good can
> > come of
> > storing an encoding in each xml field when we know the actual encoding is
> > the
> > database encoding.  However, if you implemented (a), we'd still see
> > untreated
> > values brought over via pg_upgrade.  Therefore, I would try (b) first.  I
> > suspect the intent of xml_parse() was to implement (b); it will be
> > interesting
> > to see your test case that malfunctions.
> >
>
> I looked there again and I found so this issue is related to xpath function
> only
>
> Functions based on xml_parse are working without problems. xpath_internal
> uses own direct xmlCtxtReadMemory without correct encoding sanitation.
>
> so fix is pretty simple

Please add a test case.

It needs a application - currently there is not possibility to import XML document via recv API :(

I wrote a pgimportdoc utility, but it is not part of core

 

> --- a/src/backend/utils/adt/xml.c
> +++ b/src/backend/utils/adt/xml.c
> @@ -3874,9 +3874,11 @@ xpath_internal(text *xpath_expr_text, xmltype *data,
> ArrayType *namespaces,
>         ns_count = 0;
>     }
>
> -   datastr = VARDATA(data);
> -   len = VARSIZE(data) - VARHDRSZ;
> +   datastr = xml_out_internal(data, 0);

Why not use xml_parse() instead of calling xmlCtxtReadMemory() directly?  The
answer is probably in the archives, because someone understood the problem
enough to document "Some XML-related functions may not work at all on
non-ASCII data when the server encoding is not UTF-8. This is known to be an
issue for xpath() in particular." 

Probably there are two possible issues

1. what I touched - recv function does encoding to database encoding - but document encoding is not updated.

2. there are not possibility to encode from document encoding to database encoding.

> +   len = strlen(datastr);
> +
>     xpath_len = VARSIZE(xpath_expr_text) - VARHDRSZ;
> +

The two lines of functional change don't create a cause for more newlines, so
don't add these two newlines.

ok

On Sun, Mar 12, 2017 at 10:26:33PM +0100, Pavel Stehule wrote:
> 2017-03-12 21:57 GMT+01:00 Noah Misch <noah@leadboat.com>:
> > On Sun, Mar 12, 2017 at 08:36:58PM +0100, Pavel Stehule wrote:
> > > 2017-03-12 0:56 GMT+01:00 Noah Misch <noah@leadboat.com>:
> > Please add a test case.

>
> It needs a application - currently there is not possibility to import XML
> document via recv API :(

I think xml_in() can create every value that xml_recv() can create; xml_recv()
is just more convenient given diverse source encodings.  If you make your
application store the value into a table, does "pg_dump --inserts" emit code
that reproduces the same value?  If so, you can use that in your test case.
If not, please provide precise instructions (code, SQL commands) for
reproducing the bug manually.

> > Why not use xml_parse() instead of calling xmlCtxtReadMemory() directly?
> > The
> > answer is probably in the archives, because someone understood the problem
> > enough to document "Some XML-related functions may not work at all on
> > non-ASCII data when the server encoding is not UTF-8. This is known to be
> > an
> > issue for xpath() in particular."
>
>
> Probably there are two possible issues

Would you research in the archives to confirm?

> 1. what I touched - recv function does encoding to database encoding - but
> document encoding is not updated.

Using xml_parse() would fix that, right?

> 2. there are not possibility to encode from document encoding to database
> encoding.

Both xml_in() and xml_recv() require the value to be representable in the
database encoding, so I don't think this particular problem can remain by the
time we reach an xpath_internal() call.

On Wed, Apr 05, 2017 at 10:53:39PM +0200, Pavel Stehule wrote:
> 2017-03-17 4:23 GMT+01:00 Noah Misch <noah@leadboat.com>:
> > On Sun, Mar 12, 2017 at 10:26:33PM +0100, Pavel Stehule wrote:
> > > 2017-03-12 21:57 GMT+01:00 Noah Misch <noah@leadboat.com>:
> > > > On Sun, Mar 12, 2017 at 08:36:58PM +0100, Pavel Stehule wrote:
> > > > > 2017-03-12 0:56 GMT+01:00 Noah Misch <noah@leadboat.com>:
> > > > Please add a test case.
>
> I am sending test case.
>
> I am afraid so this cannot be part of regress tests due strong dependency
> on locale win1250.

Right.  Based on that, I've distilled this portable test case:

-- Round-trip non-ASCII data through xpath().
DO $$
DECLARE
        xml_declaration text := '<?xml version="1.0" encoding="ISO-8859-1"?>';
        degree_symbol text;
        res xml[];
BEGIN
        -- Per the documentation, xpath() doesn't work on non-ASCII data when
        -- the server encoding is not UTF8.  The EXCEPTION block below,
        -- currently dead code, will be relevant if we remove this limitation.
        IF current_setting('server_encoding') <> 'UTF8' THEN
                RAISE LOG 'skip: encoding % unsupported for xml',
                        current_setting('server_encoding');
                RETURN;
        END IF;

        degree_symbol := convert_from('\xc2b0', 'UTF8');
        res := xpath('text()', (xml_declaration ||
                '<x>' || degree_symbol || '</x>')::xml);
        IF degree_symbol <> res[1]::text THEN
                RAISE 'expected % (%), got % (%)',
                        degree_symbol, convert_to(degree_symbol, 'UTF8'),
                        res[1], convert_to(res[1]::text, 'UTF8');
        END IF;
EXCEPTION
        -- character with byte sequence 0xc2 0xb0 in encoding "UTF8" has no equivalent in encoding "LATIN8"
        WHEN untranslatable_character THEN RAISE LOG 'skip: %', SQLERRM;
        -- default conversion function for encoding "UTF8" to "MULE_INTERNAL" does not exist
        WHEN undefined_function THEN RAISE LOG 'skip: %', SQLERRM;
END
$$

 
> > > > Why not use xml_parse() instead of calling xmlCtxtReadMemory()
> > > > directly?  The answer is probably in the archives, because someone
> > > > understood the problem enough to document "Some XML-related functions
> > > > may not work at all on non-ASCII data when the server encoding is not
> > > > UTF-8. This is known to be an issue for xpath() in particular."
> > >
> > > Probably there are two possible issues
> >
> > Would you research in the archives to confirm?
> >
> > > 1. what I touched - recv function does encoding to database encoding -
> > > but document encoding is not updated.
> >
> > Using xml_parse() would fix that, right?
> >
>
> It should to help, because it cuts a header - but it does little bit more
> work, than we would - it check if xml is doc or not too.

That's true.  xpath() currently works on both DOCUMENT and CONTENT xml values.
If xpath() used xml_parse(), this would stop working:

  SELECT xpath('text()', XMLPARSE (DOCUMENT '<!DOCTYPE foo []><x>bar</x>'));

> One possible fix - and similar technique is used more times in xml.c code
> .. xmlroot

> +   /* remove header */
> +   parse_xml_decl(string, &header_len, NULL, NULL, NULL);

> -       doc = xmlCtxtReadMemory(ctxt, (char *) string, len, NULL, NULL, 0);
> +       doc = xmlCtxtReadMemory(ctxt, (char *) string + header_len, len -

> another possibility is using xml_out_internal - that is used in XMLTABLE
> function - what was initial fix.
>
> xml_out_internal uses parse_xml_decl too, but it is little bit more
> expensive due call print_xml_decl that has not any effect in this case
> (where only encoding is interesting) - but it can has sense, when server
> encoding is not UTF8, because in this case, xmlstr is not encoded to UTF8 -
> so now, I am think the correct solution should be using xml_out_internal -
> because it appends a header with server encoding to XML doc

As you may have been implying here, your patch never adds an xml declaration
that bears an encoding.  (That is because it passes targetencoding=0 to
xml_out_internal().)  If we were going to fix xpath() to support non-ASCII
characters in non-UTF8 databases, we would not do that by adding xml
declarations.  We would do our own conversion to UTF8, like xml_parse() does.
In that light, I like this parse_xml_decl() approach better.  Would you update
your patch to use it and to add the test case I give above?

This change can make things worse in a non-UTF8 database.  The following
happens to work today in a LATIN1 database, but it will cease to work:

SELECT xpath('string-length()', ('<?xml version="1.0" encoding="ISO-8859-1"?>' ||
                '<x>' || chr(176) || '</x>')::xml);

However, extracting actual text already gives wrong answers, because we treat
the UTF8 response from libxml2 as though it were LATIN1:

SELECT xpath('string()', ('<?xml version="1.0" encoding="ISO-8859-1"?>' ||
                '<x>' || chr(176) || '</x>')::xml);

I plan to back-patch this.  The documentation says "Some XML-related functions
may not work at all on non-ASCII data when the server encoding is not
UTF-8. This is known to be an issue for xpath() in particular."  Your patch
fixes a case where even a UTF8 database mishandles non-ASCII data.  (Sometimes
it mishandles the data silently.  Other times, it raises an error.)  It's
worth further breaking a case where we already disclaim support to repair a
supported case.  Other opinions?

Isn't the most correct solution to call xml_parse function?

Regards

Pavel

Hi

I am sending some POC  - it does support XPATH and XMLTABLE for not UTF8 server encoding.

In this case, all strings should be converted to UTF8 before call libXML2 functions, and result should be converted back from UTF8.

I found some previous experiments https://marc.info/?l=pgsql-bugs&m=123407176408688

Note: I got some information so used xmlNodeDump function is deprecated - so we should to replace it too sometime.

Regards

Pavel

On Fri, Aug 18, 2017 at 11:43:19PM +0200, Pavel Stehule wrote:
> yes, probably libXML2 try to do check from utf8 encoding to header
> specified encoding.

Yes.  That has been the topic of this thread.

> a) all values created by  xml_in iterface are in database encoding - input
> string is stored without any change. xml_parse is called only due
> validation.
>
> b) inside xml_parse, the input is converted to UTF8, and document is read
> by xmlCtxtReadDoc with explicitly specified "UTF-8" encoding or
> by xmlParseBalancedChunkMemory with explicitly specified encoding "UTF8"
> and removed decl section.
>
> So for "xml_parse" based functions (xml_in, texttoxml, xml_is_document,
> wellformated_xml) the database encoding is not important
>
> c) xml_recv function does validation by xml_parse and translation to
> database encoding.
>
> Now I don't see a difference between @b and @c - so my hypotheses about
> necessity to use recv interface was wrong.

Yes.  You posted, on 2017-04-05, a test case not requiring the recv interface.

On Sat, Aug 19, 2017 at 09:13:50AM +0200, Pavel Stehule wrote:
> I didn't find any info how to enable libXML2 XPath functions for other
> encoding than UTF8 :( ??

http://xmlsoft.org/encoding.html is the relevant authority.  To summarize, we
should send only UTF8 to libxml2.

On Sat, Aug 19, 2017 at 10:53:19PM +0200, Pavel Stehule wrote:
> I am sending some POC  - it does support XPATH and XMLTABLE for not UTF8
> server encoding.
>
> In this case, all strings should be converted to UTF8 before call libXML2
> functions, and result should be converted back from UTF8.

Adding support for xpath in non-UTF8 databases is a v11 feature proposal.
Please start a new thread for this, and add it to the open CommitFest.

In this thread, would you provide the version of your patch that I described
in my 2017-08-08 post to this thread?  That's a back-patchable bug fix.

> I found some previous experiments https://marc.info/?l=pgsql-bugs&m=123407176408688

https://wiki.postgresql.org/wiki/Todo#XML links to other background on this
feature proposal.  See Tom Lane's review of a previous patch.  Ensure your
patch does not have the problems he found during that review.  Do that before
starting a thread for this feature.

Thanks,
nm

On Sun, Aug 20, 2017 at 08:46:03AM +0200, Pavel Stehule wrote:
> 2017-08-20 4:17 GMT+02:00 Noah Misch <noah@leadboat.com>:
> > On Sat, Aug 19, 2017 at 10:53:19PM +0200, Pavel Stehule wrote:
> > > I am sending some POC  - it does support XPATH and XMLTABLE for not UTF8
> > > server encoding.
> > >
> > > In this case, all strings should be converted to UTF8 before call libXML2
> > > functions, and result should be converted back from UTF8.
> >
> > Adding support for xpath in non-UTF8 databases is a v11 feature proposal.
> > Please start a new thread for this, and add it to the open CommitFest.
> >
> > In this thread, would you provide the version of your patch that I
> > described
> > in my 2017-08-08 post to this thread?  That's a back-patchable bug fix.
>
>
> There are three issues:
>
> 1. processing 1byte encoding XMLs documents with encoding declaration -
> should be fixed by ecoding_for_xmlCtxtReadMemory.patch. This patch is very
> short and safe - can be apply immediately (there is regress tests)

We should fix that problem, yes.  encoding_for_xmlCtxtReadMemory.patch is not
the right fix; see below.

> 2 encoding issues in XPath specification (and  namespaces) - because
> multibytes chars are not usually used in tag names, this issue hit minimum
> users.
>
> 3. encoding issues in XPath and XMLTABLE results - this is bad issue - the
> function XMLTABLE will not be functional on non UTF8 databases. Fortunately
> - there are less users with this encoding, but probably should be apply as
> fix in 10/11 Postgres.

(2) and (3) are long-documented (since commit 14180f9, 2009-06) limitations in
xpath functions.  That's why I would treat an improvement as a new feature and
not back-patch it.  It is tempting to fix v10 so XMLTABLE is born without this
limitation, but it is too late in the release cycle.

Reviewing encoding_for_xmlCtxtReadMemory.patch:

On Sat, Aug 19, 2017 at 09:13:50AM +0200, Pavel Stehule wrote:
> -             doc = xmlCtxtReadMemory(ctxt, (char *) string, len, NULL, NULL, 0);
> +             doc = xmlCtxtReadMemory(ctxt, (char *) string, len, NULL,
> +                                                             pg_encoding_to_char(GetDatabaseEncoding()), 0);

This assumes libxml2 understands every encoding name that
pg_encoding_to_char() can return, but it does not.  xpath-bugfix.patch was
better.  Here are the relevant parts of my review of that patch.

On Mon, Aug 07, 2017 at 05:10:14PM -0700, Noah Misch wrote:
> On Wed, Apr 05, 2017 at 10:53:39PM +0200, Pavel Stehule wrote:
> > One possible fix - and similar technique is used more times in xml.c code
> > .. xmlroot
>
> > +   /* remove header */
> > +   parse_xml_decl(string, &header_len, NULL, NULL, NULL);
>
> > -       doc = xmlCtxtReadMemory(ctxt, (char *) string, len, NULL, NULL, 0);
> > +       doc = xmlCtxtReadMemory(ctxt, (char *) string + header_len, len -

> I like this parse_xml_decl() approach better.  Would you update
> your patch to use it and to add the test case I give above?

Again, would you make those two edits?

Thanks,
nm

xpath-bugfix.patch affected only xml values containing an xml declaration with
"encoding" attribute.  In UTF8 databases, this latest proposal
(xpath-parsing-error-fix.patch) is equivalent to xpath-bugfix.patch.  In
non-UTF8 databases, xpath-parsing-error-fix.patch affects all xml values
containing non-ASCII data.  In a LATIN1 database, the following works today
but breaks under your latest proposal:

  SELECT xpath('text()', ('<x>' || convert_from('\xc2b0', 'LATIN1') || '</x>')::xml);

It's acceptable to break that, since the documentation explicitly disclaims
support for it.  xpath-bugfix.patch breaks different use cases, which are
likewise acceptable to break.  See my 2017-08-08 review for details.

We have xpath-bugfix.patch and xpath-parsing-error-fix.patch.  Both are
equivalent under supported use cases (xpath in UTF8 databases).  Among
non-supported use cases, they each make different things better and different
things worse.  We should prefer to back-patch the version harming fewer
applications.  I expect non-ASCII data is more common than xml declarations
with "encoding" attribute, so xpath-bugfix.patch will harm fewer applications.

Having said that, I now see a third option.  Condition this thread's patch's
effects on GetDatabaseEncoding()==PG_UTF8.  That way, we fix supported cases,
and we remain bug-compatible in unsupported cases.  I think that's better than
the other options discussed so far.  If you agree, please send a patch based
on xpath-bugfix.patch with the GetDatabaseEncoding()==PG_UTF8 change and the
two edits I described earlier.

Thanks,
nm

xpath-bugfix.patch affected only xml values containing an xml declaration with
"encoding" attribute.  In UTF8 databases, this latest proposal
(xpath-parsing-error-fix.patch) is equivalent to xpath-bugfix.patch.  In
non-UTF8 databases, xpath-parsing-error-fix.patch affects all xml values
containing non-ASCII data.  In a LATIN1 database, the following works today
but breaks under your latest proposal:

  SELECT xpath('text()', ('<x>' || convert_from('\xc2b0', 'LATIN1') || '</x>')::xml);

It's acceptable to break that, since the documentation explicitly disclaims
support for it.  xpath-bugfix.patch breaks different use cases, which are
likewise acceptable to break.  See my 2017-08-08 review for details.

xpath-bugfix.patch affected only xml values containing an xml declaration with
"encoding" attribute.  In UTF8 databases, this latest proposal
(xpath-parsing-error-fix.patch) is equivalent to xpath-bugfix.patch.  In
non-UTF8 databases, xpath-parsing-error-fix.patch affects all xml values
containing non-ASCII data.  In a LATIN1 database, the following works today
but breaks under your latest proposal:

  SELECT xpath('text()', ('<x>' || convert_from('\xc2b0', 'LATIN1') || '</x>')::xml);

It's acceptable to break that, since the documentation explicitly disclaims
support for it.  xpath-bugfix.patch breaks different use cases, which are
likewise acceptable to break.  See my 2017-08-08 review for details.

The fact so this code is working shows so a universe is pretty dangerous place :) 

On Sun, Aug 20, 2017 at 10:37:10PM +0200, Pavel Stehule wrote:
> > We have xpath-bugfix.patch and xpath-parsing-error-fix.patch.  Both are
> > equivalent under supported use cases (xpath in UTF8 databases).  Among
> > non-supported use cases, they each make different things better and
> > different
> > things worse.  We should prefer to back-patch the version harming fewer
> > applications.  I expect non-ASCII data is more common than xml declarations
> > with "encoding" attribute, so xpath-bugfix.patch will harm fewer
> > applications.
> >
> > Having said that, I now see a third option.  Condition this thread's
> > patch's
> > effects on GetDatabaseEncoding()==PG_UTF8.  That way, we fix supported
> > cases,
> > and we remain bug-compatible in unsupported cases.  I think that's better
> > than
> > the other options discussed so far.  If you agree, please send a patch
> > based
> > on xpath-bugfix.patch with the GetDatabaseEncoding()==PG_UTF8 change and
> > the
> > two edits I described earlier.
> >
>
> I am sorry -  too long day today. Do you think some like
>
> diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
> index 24229c2dff..9fd6f3509f 100644
> --- a/src/backend/utils/adt/xml.c
> +++ b/src/backend/utils/adt/xml.c
> @@ -3914,7 +3914,14 @@ xpath_internal(text *xpath_expr_text, xmltype *data,
> ArrayType *namespaces,
>         if (ctxt == NULL || xmlerrcxt->err_occurred)
>             xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
>                         "could not allocate parser context");
> -       doc = xmlCtxtReadMemory(ctxt, (char *) string, len, NULL, NULL, 0);
> +
> +       /*
> +        * Passed XML is always in server encoding. When server encoding
> +        * is UTF8, we can pass this information to libxml2 to ignore
> +        * possible invalid encoding declaration in XML document.
> +        */
> +       doc = xmlCtxtReadMemory(ctxt, (char *) string, len, NULL,
> +               GetDatabaseEncoding() == PG_UTF8 ? "UTF-8" : NULL, 0);
>         if (doc == NULL || xmlerrcxt->err_occurred)
>             xml_ereport(xmlerrcxt, ERROR, ERRCODE_INVALID_XML_DOCUMENT,
>                         "could not parse XML document");

No, that doesn't match my description above.  I don't see a way to clarify the
description.  Feel free to try again.  Alternately, if you wait, I will
eventually construct the patch I described.

On Sun, Nov 05, 2017 at 06:10:04PM +0100, Pavel Stehule wrote:
> Hi
>
> 2017-11-05 4:07 GMT+01:00 Noah Misch <noah@leadboat.com>:
>
> > On Tue, Oct 17, 2017 at 06:06:40AM +0200, Pavel Stehule wrote:
> > > Please, if you can, try it write. I am little bit lost :)
> >
> > I'm attaching the patch I desired.  Please review.  This will probably miss
> > this week's minor releases.  If there's significant support, I could
> > instead
> > push before the wrap.
> >
>
> I have not any objection to this solution. It fixes my regress tests too.
>
> I checked it and it is working.

Pushed, but the buildfarm shows I didn't get the test quite right for the
non-xml, non-UTF8 case.  Fixing.