Re: [HACKERS] possible encoding issues with libxml2 functions

[Pavel Stehule]

2017-11-11 21:19 GMT+01:00 Noah Misch :

> On Sun, Nov 05, 2017 at 06:10:04PM +0100, Pavel Stehule wrote:
> > Hi
> >
> > 2017-11-05 4:07 GMT+01:00 Noah Misch :
> >
> > > 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.
>

Thank you

Pavel

Re: [HACKERS] possible encoding issues with libxml2 functions

[Noah Misch]

On Sun, Nov 05, 2017 at 06:10:04PM +0100, Pavel Stehule wrote:
> Hi
> 
> 2017-11-05 4:07 GMT+01:00 Noah Misch :
> 
> > 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.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] possible encoding issues with libxml2 functions

[Pavel Stehule]

Hi

2017-11-05 4:07 GMT+01:00 Noah Misch :

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

Regards

Pavel

Re: [HACKERS] possible encoding issues with libxml2 functions

[Noah Misch]

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.
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index 24229c2..3e3aed8 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -3845,6 +3845,7 @@ xpath_internal(text *xpath_expr_text, xmltype *data, 
ArrayType *namespaces,
int32   xpath_len;
xmlChar*string;
xmlChar*xpath_expr;
+   size_t  xmldecl_len = 0;
int i;
int ndim;
Datum  *ns_names_uris;
@@ -3900,6 +3901,16 @@ xpath_internal(text *xpath_expr_text, xmltype *data, 
ArrayType *namespaces,
string = pg_xmlCharStrndup(datastr, len);
xpath_expr = pg_xmlCharStrndup(VARDATA_ANY(xpath_expr_text), xpath_len);
 
+   /*
+* In a UTF8 database, skip any xml declaration, which might assert
+* another encoding.  Ignore parse_xml_decl() failure, letting
+* xmlCtxtReadMemory() report parse errors.  Documentation disclaims
+* xpath() support for non-ASCII data in non-UTF8 databases, so leave
+* those scenarios bug-compatible with historical behavior.
+*/
+   if (GetDatabaseEncoding() == PG_UTF8)
+   parse_xml_decl(string, _len, NULL, NULL, NULL);
+
xmlerrcxt = pg_xml_init(PG_XML_STRICTNESS_ALL);
 
PG_TRY();
@@ -3914,7 +3925,8 @@ 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);
+   doc = xmlCtxtReadMemory(ctxt, (char *) string + xmldecl_len,
+   len - 
xmldecl_len, NULL, NULL, 0);
if (doc == NULL || xmlerrcxt->err_occurred)
xml_ereport(xmlerrcxt, ERROR, 
ERRCODE_INVALID_XML_DOCUMENT,
"could not parse XML document");
diff --git a/src/test/regress/expected/xml.out 
b/src/test/regress/expected/xml.out
index bcc585d..f7a8c38 100644
--- a/src/test/regress/expected/xml.out
+++ b/src/test/regress/expected/xml.out
@@ -670,6 +670,37 @@ SELECT xpath('/nosuchtag', '');
  {}
 (1 row)
 
+-- Round-trip non-ASCII data through xpath().
+DO $$
+DECLARE
+  xml_declaration text := '';
+  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 ||
+'' || degree_symbol || '')::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
+$$;
 -- Test xmlexists and xpath_exists
 SELECT xmlexists('//town[text() = ''Toronto'']' PASSING BY REF 
'Bidford-on-AvonCwmbranBristol');
  xmlexists 
diff --git a/src/test/regress/expected/xml_1.out 
b/src/test/regress/expected/xml_1.out
index d3bd8c9..1a9efa2 100644
--- a/src/test/regress/expected/xml_1.out
+++ b/src/test/regress/expected/xml_1.out
@@ -576,6 +576,41 @@ LINE 1: SELECT xpath('/nosuchtag', '');
^
 DETAIL:  This functionality requires the server to be built with libxml 
support.
 HINT:  You need to rebuild PostgreSQL using --with-libxml.
+-- Round-trip non-ASCII data through xpath().
+DO $$
+DECLARE
+  xml_declaration text := '';
+  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',
+  

Re: [HACKERS] possible encoding issues with libxml2 functions

[Pavel Stehule]

2017-10-17 1:57 GMT+02:00 Noah Misch :

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

Please, if you can, try it write. I am little bit lost :)

Regards

Pavel

Re: [HACKERS] possible encoding issues with libxml2 functions

[Noah Misch]

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.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] possible encoding issues with libxml2 functions

[Pavel Stehule]

2017-08-21 6:25 GMT+02:00 Pavel Stehule :

>
>
>> 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()', ('' || convert_from('\xc2b0', 'LATIN1') ||
>> '')::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 :)
>
>
ping?

will we continue in this topic?



>

Re: [HACKERS] possible encoding issues with libxml2 functions

[Pavel Stehule]

>
> 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()', ('' || convert_from('\xc2b0', 'LATIN1') ||
> '')::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 :)

Re: [HACKERS] possible encoding issues with libxml2 functions

[Pavel Stehule]

Hi


> 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()', ('' || convert_from('\xc2b0', 'LATIN1') ||
> '')::xml);
>

I don't understand, why it should not work?


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

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");

This fix only UTF8 servers and has no any effect on other cases

?

Regards

Pavel



> Thanks,
> nm
>

Re: [HACKERS] possible encoding issues with libxml2 functions

[Noah Misch]

On Sun, Aug 20, 2017 at 10:54:57AM +0200, Pavel Stehule wrote:
> 2017-08-20 9:21 GMT+02:00 Noah Misch :
> > 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, _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?
> 
> Now, I am not sure so it is correct fix. We will fix case, when server is
> in UTF8, but maybe we will break some cases when server is not in UTF8
> (although it is broken already).

That's right.

> I am think so correct solution is encoding to UTF8 and passing encoding
> parameter. It will works safely in all cases - and we don't need cut
> header. We should not to cut header if server encoding is not in UTF8 and
> we don't pass encoding as parameter. When we pass encoding as parameter,
> then cutting header is not necessary.

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()', ('' || convert_from('\xc2b0', 'LATIN1') || 
'')::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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] possible encoding issues with libxml2 functions

[Pavel Stehule]

2017-08-20 9:21 GMT+02:00 Noah Misch :

> On Sun, Aug 20, 2017 at 08:46:03AM +0200, Pavel Stehule wrote:
> > 2017-08-20 4:17 GMT+02:00 Noah Misch :
> > > 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.
>

I agree

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

I understand to this argument.

>
> 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, _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?
>

Now, I am not sure so it is correct fix. We will fix case, when server is
in UTF8, but maybe we will break some cases when server is not in UTF8
(although it is broken already).

I am think so correct solution is encoding to UTF8 and passing encoding
parameter. It will works safely in all cases - and we don't need cut
header. We should not to cut header if server encoding is not in UTF8 and
we don't pass encoding as parameter. When we pass encoding as parameter,
then cutting header is not necessary.

What do you think about last patch?

Regards

Pavel



>
> Thanks,
> nm
>
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index c47624eff6..5ceda8034d 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -3895,6 +3895,13 @@ xpath_internal(text *xpath_expr_text, xmltype *data, ArrayType *namespaces,
  errmsg("empty XPath expression")));
 
 	string = pg_xmlCharStrndup(datastr, len);
+
+	/* It does nothing when string is in UTF8 already */
+	string = pg_do_encoding_conversion(string,
+	   len,
+	   GetDatabaseEncoding(),
+	   PG_UTF8);
+
 	xpath_expr = pg_xmlCharStrndup(VARDATA_ANY(xpath_expr_text), xpath_len);
 
 	xmlerrcxt = pg_xml_init(PG_XML_STRICTNESS_ALL);
@@ -3911,7 +3918,7 @@ 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);
+		doc = xmlCtxtReadMemory(ctxt, (char *) string, len, NULL, "UTF-8", 0);
 		if (doc == NULL || xmlerrcxt->err_occurred)
 			xml_ereport(xmlerrcxt, ERROR, 

Re: [HACKERS] possible encoding issues with libxml2 functions

[Noah Misch]

On Sun, Aug 20, 2017 at 08:46:03AM +0200, Pavel Stehule wrote:
> 2017-08-20 4:17 GMT+02:00 Noah Misch :
> > 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, _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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] possible encoding issues with libxml2 functions

[Pavel Stehule]

2017-08-20 4:17 GMT+02:00 Noah Misch :

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

libxml2 encodes XML to UTF8 by self. All others should be in UTF8. I found
some references to xmlSwitchEncoding function - but I didn't find any
examples of usage - probably nobody use it. Result is in UTF8 always.


>
> 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)

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.



> I found some previous experiments https://marc.info/?l=pgsql-
> bugs=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.
>

good information - thank you. I'll start new thread for @2 and @3 issues -
not sure if I prepare good enough patch for next commit fest - and later
commiter can decide if will do backpatching.

Regards

Pavel



>
> Thanks,
> nm
>

Re: [HACKERS] possible encoding issues with libxml2 functions

[Noah Misch]

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=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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] possible encoding issues with libxml2 functions

[Pavel Stehule]

Hi

2017-08-19 22:53 GMT+02:00 Pavel Stehule :

> 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=
> 123407176408688
>
> Note: I got some information so used xmlNodeDump function is deprecated -
> so we should to replace it too sometime.
>
> Regards
>
>
I forgot a debug elog in previous patch




> Pavel
>
>
>
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index c47624eff6..a43cf13d16 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -147,6 +147,7 @@ static int xml_xpathobjtoxmlarray(xmlXPathObjectPtr xpathobj,
 	   ArrayBuildState *astate,
 	   PgXmlErrorContext *xmlerrcxt);
 static xmlChar *pg_xmlCharStrndup(char *str, size_t len);
+static xmlChar *pg_xmlCharUtf8(char *str, size_t len);
 #endif			/* USE_LIBXML */
 
 static void xmldata_root_element_start(StringInfo result, const char *eltname,
@@ -459,8 +460,28 @@ cstring_to_xmltype(const char *string)
 static xmltype *
 xmlBuffer_to_xmltype(xmlBufferPtr buf)
 {
-	return (xmltype *) cstring_to_text_with_len((const char *) xmlBufferContent(buf),
+	if (GetDatabaseEncoding() != PG_UTF8)
+	{
+		char *utf8str = (char *) xmlBufferContent(buf);
+		char *str;
+		xmltype *result;
+
+		str = (char *) pg_do_encoding_conversion((unsigned char *) utf8str,
+			   xmlBufferLength(buf),
+			   PG_UTF8,
+			   GetDatabaseEncoding());
+
+		Assert(str != utf8str);
+		result = (xmltype *) cstring_to_text(str);
+		pfree(str);
+
+		return result;
+	}
+	else
+	{
+		return (xmltype *) cstring_to_text_with_len((const char *) xmlBufferContent(buf),
 xmlBufferLength(buf));
+	}
 }
 #endif
 
@@ -1176,6 +1197,28 @@ pg_xmlCharStrndup(char *str, size_t len)
 }
 
 /*
+ * LibXML2 internal encoding is UTF8. Sometimes LibXML2 enforce
+ * encoding to UTF8 by self, sometimes it expects UTF8 strings.
+ * This function is used for encoding from database encoding to
+ * UTF8.
+ */
+static xmlChar *
+pg_xmlCharUtf8(char *str, size_t len)
+{
+	char *result;
+
+	result = (char *) pg_do_encoding_conversion((unsigned char *) str,
+		   len,
+		   GetDatabaseEncoding(),
+		   PG_UTF8);
+
+	if (result != str)
+		return BAD_CAST result;
+
+	return pg_xmlCharStrndup(str, len);
+}
+
+/*
  * str is the null-terminated input string.  Remaining arguments are
  * output arguments; each can be NULL if value is not wanted.
  * version and encoding are returned as locally-palloc'd strings.
@@ -3714,9 +3757,16 @@ xml_xmlnodetoxmltype(xmlNodePtr cur, PgXmlErrorContext *xmlerrcxt)
 	}
 	else
 	{
-		xmlChar*str;
+		xmlChar*utf8str;
+		char	   *str = NULL;
+
+		utf8str = xmlXPathCastNodeToString(cur);
+
+		str = (char *) pg_do_encoding_conversion((unsigned char *) utf8str,
+		   strlen((char *) utf8str),
+		   PG_UTF8,
+		   GetDatabaseEncoding());
 
-		str = xmlXPathCastNodeToString(cur);
 		PG_TRY();
 		{
 			/* Here we rely on XML having the same representation as TEXT */
@@ -3727,11 +3777,18 @@ xml_xmlnodetoxmltype(xmlNodePtr cur, PgXmlErrorContext *xmlerrcxt)
 		}
 		PG_CATCH();
 		{
-			xmlFree(str);
+			if (str != (char *) utf8str)
+pfree(str);
+
+			xmlFree(utf8str);
 			PG_RE_THROW();
 		}
 		PG_END_TRY();
-		xmlFree(str);
+
+		if (str != (char *) utf8str)
+			pfree(str);
+
+		xmlFree(utf8str);
 	}
 
 	return result;
@@ -3758,6 +3815,7 @@ xml_xpathobjtoxmlarray(xmlXPathObjectPtr xpathobj,
 	Datum		datum;
 	Oid			datumtype;
 	char	   *result_str;
+	char	   *str = NULL;
 
 	switch (xpathobj->type)
 	{
@@ -3797,7 +3855,18 @@ xml_xpathobjtoxmlarray(xmlXPathObjectPtr xpathobj,
 		case XPATH_STRING:
 			if (astate == NULL)
 return 1;
-			datum = CStringGetDatum((char *) xpathobj->stringval);
+
+			/*
+			 * returned string is in UTF8 encoding - should be encoded
+			 * to database encoding first.
+			 */
+			str = (char *) pg_do_encoding_conversion((unsigned char *) xpathobj->stringval,
+		   strlen((char *) xpathobj->stringval),
+		   PG_UTF8,
+		   GetDatabaseEncoding());
+
+			datum = CStringGetDatum(str);
+
 			datumtype = CSTRINGOID;
 			break;
 
@@ -3812,6 +3881,7 @@ xml_xpathobjtoxmlarray(xmlXPathObjectPtr xpathobj,
 	datum = PointerGetDatum(cstring_to_xmltype(result_str));
 	(void) accumArrayResult(astate, datum, false,
 			XMLOID, CurrentMemoryContext);
+
 	return 1;
 }
 
@@ -3895,7 +3965,7 @@ xpath_internal(text *xpath_expr_text, xmltype *data, ArrayType *namespaces,
  errmsg("empty XPath expression")));
 
 	string = pg_xmlCharStrndup(datastr, len);
-	xpath_expr = pg_xmlCharStrndup(VARDATA_ANY(xpath_expr_text), xpath_len);
+	xpath_expr = pg_xmlCharUtf8(VARDATA_ANY(xpath_expr_text), xpath_len);
 
 	xmlerrcxt = 

Re: [HACKERS] possible encoding issues with libxml2 functions

[Pavel Stehule]

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-bug
s=123407176408688

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

Regards

Pavel
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index c47624eff6..d02cec88ab 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -147,6 +147,7 @@ static int xml_xpathobjtoxmlarray(xmlXPathObjectPtr xpathobj,
 	   ArrayBuildState *astate,
 	   PgXmlErrorContext *xmlerrcxt);
 static xmlChar *pg_xmlCharStrndup(char *str, size_t len);
+static xmlChar *pg_xmlCharUtf8(char *str, size_t len);
 #endif			/* USE_LIBXML */
 
 static void xmldata_root_element_start(StringInfo result, const char *eltname,
@@ -459,8 +460,28 @@ cstring_to_xmltype(const char *string)
 static xmltype *
 xmlBuffer_to_xmltype(xmlBufferPtr buf)
 {
-	return (xmltype *) cstring_to_text_with_len((const char *) xmlBufferContent(buf),
+	if (GetDatabaseEncoding() != PG_UTF8)
+	{
+		char *utf8str = (char *) xmlBufferContent(buf);
+		char *str;
+		xmltype *result;
+
+		str = (char *) pg_do_encoding_conversion((unsigned char *) utf8str,
+			   xmlBufferLength(buf),
+			   PG_UTF8,
+			   GetDatabaseEncoding());
+
+		Assert(str != utf8str);
+		result = (xmltype *) cstring_to_text(str);
+		pfree(str);
+
+		return result;
+	}
+	else
+	{
+		return (xmltype *) cstring_to_text_with_len((const char *) xmlBufferContent(buf),
 xmlBufferLength(buf));
+	}
 }
 #endif
 
@@ -1176,6 +1197,28 @@ pg_xmlCharStrndup(char *str, size_t len)
 }
 
 /*
+ * LibXML2 internal encoding is UTF8. Sometimes LibXML2 enforce
+ * encoding to UTF8 by self, sometimes it expects UTF8 strings.
+ * This function is used for encoding from database encoding to
+ * UTF8.
+ */
+static xmlChar *
+pg_xmlCharUtf8(char *str, size_t len)
+{
+	char *result;
+
+	result = (char *) pg_do_encoding_conversion((unsigned char *) str,
+		   len,
+		   GetDatabaseEncoding(),
+		   PG_UTF8);
+
+	if (result != str)
+		return BAD_CAST result;
+
+	return pg_xmlCharStrndup(str, len);
+}
+
+/*
  * str is the null-terminated input string.  Remaining arguments are
  * output arguments; each can be NULL if value is not wanted.
  * version and encoding are returned as locally-palloc'd strings.
@@ -3714,9 +3757,16 @@ xml_xmlnodetoxmltype(xmlNodePtr cur, PgXmlErrorContext *xmlerrcxt)
 	}
 	else
 	{
-		xmlChar*str;
+		xmlChar*utf8str;
+		char	   *str = NULL;
+
+		utf8str = xmlXPathCastNodeToString(cur);
+
+		str = (char *) pg_do_encoding_conversion((unsigned char *) utf8str,
+		   strlen((char *) utf8str),
+		   PG_UTF8,
+		   GetDatabaseEncoding());
 
-		str = xmlXPathCastNodeToString(cur);
 		PG_TRY();
 		{
 			/* Here we rely on XML having the same representation as TEXT */
@@ -3727,11 +3777,18 @@ xml_xmlnodetoxmltype(xmlNodePtr cur, PgXmlErrorContext *xmlerrcxt)
 		}
 		PG_CATCH();
 		{
-			xmlFree(str);
+			if (str != (char *) utf8str)
+pfree(str);
+
+			xmlFree(utf8str);
 			PG_RE_THROW();
 		}
 		PG_END_TRY();
-		xmlFree(str);
+
+		if (str != (char *) utf8str)
+			pfree(str);
+
+		xmlFree(utf8str);
 	}
 
 	return result;
@@ -3758,6 +3815,7 @@ xml_xpathobjtoxmlarray(xmlXPathObjectPtr xpathobj,
 	Datum		datum;
 	Oid			datumtype;
 	char	   *result_str;
+	char	   *str = NULL;
 
 	switch (xpathobj->type)
 	{
@@ -3797,7 +3855,20 @@ xml_xpathobjtoxmlarray(xmlXPathObjectPtr xpathobj,
 		case XPATH_STRING:
 			if (astate == NULL)
 return 1;
-			datum = CStringGetDatum((char *) xpathobj->stringval);
+
+			/*
+			 * returned string is in UTF8 encoding - should be encoded
+			 * to database encoding first.
+			 */
+			str = (char *) pg_do_encoding_conversion((unsigned char *) xpathobj->stringval,
+		   strlen((char *) xpathobj->stringval),
+		   PG_UTF8,
+		   GetDatabaseEncoding());
+
+elog(NOTICE, ">>>%s<<<", str);
+
+			datum = CStringGetDatum(str);
+
 			datumtype = CSTRINGOID;
 			break;
 
@@ -3812,6 +3883,7 @@ xml_xpathobjtoxmlarray(xmlXPathObjectPtr xpathobj,
 	datum = PointerGetDatum(cstring_to_xmltype(result_str));
 	(void) accumArrayResult(astate, datum, false,
 			XMLOID, CurrentMemoryContext);
+
 	return 1;
 }
 
@@ -3895,7 +3967,7 @@ xpath_internal(text *xpath_expr_text, xmltype *data, ArrayType *namespaces,
  errmsg("empty XPath expression")));
 
 	string = pg_xmlCharStrndup(datastr, len);
-	xpath_expr = pg_xmlCharStrndup(VARDATA_ANY(xpath_expr_text), xpath_len);
+	xpath_expr = pg_xmlCharUtf8(VARDATA_ANY(xpath_expr_text), xpath_len);
 
 	xmlerrcxt = pg_xml_init(PG_XML_STRICTNESS_ALL);
 
@@ -3911,7 +3983,9 @@ xpath_internal(text *xpath_expr_text, xmltype *data, ArrayType *namespaces,
 		if 

Re: [HACKERS] possible encoding issues with libxml2 functions

[Pavel Stehule]

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

I am reply to self. Probably not.

Now, I am thinking so I found a reason of this issue. The document
processed in xpath_internal is passed to libXML2 by

doc = xmlCtxtReadMemory(ctxt, (char *) string, len, NULL, NULL, 0);

We don't pass a encoding parameter so libXML2 expecting "UTF8" or expecting
correct encoding decl in XML document. When we pass incorrect document -
XML is in database encoding, but encoding decl is original, then it should
to fail.

the regress test can looks like your (but all chars are valid there)

postgres=# do $$
declare str text;
begin
  if current_setting('server_encoding') <> 'UTF8' then return; end if;
  str = '909'
  || convert_from('\xc588', 'UTF8')
  || '';
  raise notice '%', xpath('/enprimeur/vino/id', str::xml);
end; $$;
ERROR:  could not parse XML document
DETAIL:  input conversion failed due to input error, bytes 0x88 0x3C 0x2F
0x72
line 1: switching encoding: encoder error
�
 ^
CONTEXT:  PL/pgSQL function inline_code_block line 8 at RAISE

After correct fix:

doc = xmlCtxtReadMemory(ctxt, (char *) string, len, NULL,
pg_encoding_to_char(GetDatabaseEncoding()),
0);

It is working

postgres=# do $$
declare str text;
begin
  if current_setting('server_encoding') <> 'UTF8' then return; end if;
  str = '909'
  || convert_from('\xc588', 'UTF8')
  || '';
  raise notice '%', xpath('/enprimeur/vino/id', str::xml);
end; $$;
NOTICE:  {909}
DO

This fix should be apply to xmltable function too.

patch attached

It doesn't fix xpath and xmltable functions issues when server encoding is
not UTF8. Looks so XPath functions from libXML2 requires UTF8 encoded
strings and the result is in UTF8 too - so result should be recoded to
server encoding.

I didn't find any info how to enable libXML2 XPath functions for other
encoding than UTF8 :( ??

Regards

Pavel



> Regards
>
> Pavel
>
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index c47624eff6..b09ce23cdb 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -3911,7 +3911,9 @@ 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);
+		doc = xmlCtxtReadMemory(ctxt, (char *) string, len, NULL,
+pg_encoding_to_char(GetDatabaseEncoding()), 0);
+
 		if (doc == NULL || xmlerrcxt->err_occurred)
 			xml_ereport(xmlerrcxt, ERROR, ERRCODE_INVALID_XML_DOCUMENT,
 		"could not parse XML document");
@@ -4242,18 +4244,14 @@ XmlTableSetDocument(TableFuncScanState *state, Datum value)
 
 	xtCxt = GetXmlTableBuilderPrivateData(state, "XmlTableSetDocument");
 
-	/*
-	 * Use out function for casting to string (remove encoding property). See
-	 * comment in xml_out.
-	 */
-	str = xml_out_internal(xmlval, 0);
-
-	length = strlen(str);
+	str = VARDATA(xmlval);
+	length = VARSIZE(xmlval) - VARHDRSZ;
 	xstr = pg_xmlCharStrndup(str, length);
 
 	PG_TRY();
 	{
-		doc = xmlCtxtReadMemory(xtCxt->ctxt, (char *) xstr, length, NULL, NULL, 0);
+		doc = xmlCtxtReadMemory(xtCxt->ctxt, (char *) xstr, length, NULL,
+pg_encoding_to_char(GetDatabaseEncoding()), 0);
 		if (doc == NULL || xtCxt->xmlerrcxt->err_occurred)
 			xml_ereport(xtCxt->xmlerrcxt, ERROR, ERRCODE_INVALID_XML_DOCUMENT,
 		"could not parse XML document");
diff --git a/src/test/regress/expected/xml.out b/src/test/regress/expected/xml.out
index bcc585d427..6a43896d40 100644
--- a/src/test/regress/expected/xml.out
+++ b/src/test/regress/expected/xml.out
@@ -1452,3 +1452,24 @@ SELECT xmltable.* FROM xmltest2, LATERAL xmltable(('/d/r/' || lower(_path) || 'c
  14
 (4 rows)
 
+-- XML is saved in database encoding with original encoding declaration.
+-- There can be incosistency based on wrong user input, different server/client
+-- encoding or reading XML with recv function. All XML functions should to
+-- work with this partially broken XML.
+DO $$
+DECLARE str text;
+BEGIN
+  -- leave early without error, when we are not sure about result of conversion
+  IF current_setting('server_encoding') NOT IN ('UTF8', 'LATIN2') THEN return; END IF;
+
+  -- build valid UTF8 XML with broken encoding declaration
+  str = '909'
+  || convert_from('\xf2', 'windows-1250')
+  || '';
+
+  -- should to work
+  RAISE NOTICE '%', xpath('/enprimeur/vino/id', str::xml);
+  RAISE NOTICE '%', (SELECT id FROM xmltable('/enprimeur/vino' PASSING (str::xml) COLUMNS id int));
+END; $$;
+NOTICE:  {909}
+NOTICE:  909
diff --git a/src/test/regress/sql/xml.sql b/src/test/regress/sql/xml.sql
index eb4687fb09..97a3aa9de2 100644
--- a/src/test/regress/sql/xml.sql
+++ b/src/test/regress/sql/xml.sql
@@ -558,3 +558,23 @@ INSERT INTO xmltest2 VALUES('2', 'D');
 

Re: [HACKERS] possible encoding issues with libxml2 functions

[Pavel Stehule]

Hi

2017-08-08 2:10 GMT+02:00 Noah Misch :

> On Wed, Apr 05, 2017 at 10:53:39PM +0200, Pavel Stehule wrote:
> > 2017-03-17 4:23 GMT+01:00 Noah Misch :
> > > On Sun, Mar 12, 2017 at 10:26:33PM +0100, Pavel Stehule wrote:
> > > > 2017-03-12 21:57 GMT+01:00 Noah Misch :
> > > > > On Sun, Mar 12, 2017 at 08:36:58PM +0100, Pavel Stehule wrote:
> > > > > > 2017-03-12 0:56 GMT+01:00 Noah Misch :
> > > > > 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 := ' 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 ||
> '' || degree_symbol || '')::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
> $$
>

yes, probably libXML2 try to do check from utf8 encoding to header
specified encoding.


> > > > > 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 ' []>bar'));
>
> > One possible fix - and similar technique is used more times in xml.c code
> > .. xmlroot
>
> > +   /* remove header */
> > +   parse_xml_decl(string, _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?
>
>
I need to do some recapitulation (my analyses was wrong):

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 

Re: [HACKERS] possible encoding issues with libxml2 functions

[Noah Misch]

On Wed, Apr 05, 2017 at 10:53:39PM +0200, Pavel Stehule wrote:
> 2017-03-17 4:23 GMT+01:00 Noah Misch :
> > On Sun, Mar 12, 2017 at 10:26:33PM +0100, Pavel Stehule wrote:
> > > 2017-03-12 21:57 GMT+01:00 Noah Misch :
> > > > On Sun, Mar 12, 2017 at 08:36:58PM +0100, Pavel Stehule wrote:
> > > > > 2017-03-12 0:56 GMT+01:00 Noah Misch :
> > > > 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 := '';
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 ||
'' || degree_symbol || '')::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 'bar'));

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

> +   /* remove header */
> +   parse_xml_decl(string, _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()', ('' 
||
'' || chr(176) || '')::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()', ('' ||
'' || chr(176) || '')::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 

Re: [HACKERS] possible encoding issues with libxml2 functions

[Pavel Stehule]

2017-03-17 4:23 GMT+01:00 Noah Misch :

> On Sun, Mar 12, 2017 at 10:26:33PM +0100, Pavel Stehule wrote:
> > 2017-03-12 21:57 GMT+01:00 Noah Misch :
> > > On Sun, Mar 12, 2017 at 08:36:58PM +0100, Pavel Stehule wrote:
> > > > 2017-03-12 0:56 GMT+01:00 Noah Misch :
> > > 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.


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

I was wrong - both methods produces broken internal XML document




1
Alter Ego de Palmer
63
2012
0
0
0
0
1425
1085
0
0
51 % Merlot, 40 % Cabernet Sauvignon,9 %
Petit Verdot
2017 - 2026
2
0
0
0
0
0
13,4 %
Premiant oblasti Margaux Ch. Palmer
tentokrát ve svých obou vínech těžil z dokonale zralého Merlotu, kterého do
svých směsí naládoval více než Cabernet Sauvignonu. 2. víno Alter Ego de
Palmer 2012 se může p
ochlubit skvělou kondicí. Není pochyb o tom, že na Ch. Palmer sklízeli z
vinice dokonale zralé hrozny. Alter Ego je mohutné, husté a syté víno,
nabízí objemnou dávku ovoce, voní po ostružinách, chuť je krásně kulatá,
pevná a svěží, taniny
 vykazují fenomenální strukturu, takto krásné spojení všech aromatických a
chuťových složek s příměsí úžasných kyselin a alkoholu je radost mít ve
sklepě.
Robert Parker: /100
TOPVINO SCORE: 92-94/100
James Suckling: 92-93/100
Wine Spectator: 90-93/100






1

8
1




Document is well encoded from win1250 to UTF8 - it is well readable, but
the header is wrong - it is not in win1250 now (after encoding). This xml
is attached (in original form (win1250 encoding)).

Import with

create table target(x xml);
\set xml `cat ~/Downloads/e6.xml`
postgres=# set client_encoding to win1250;
postgres=# insert into target values(:'xml');

disconnect or reset client encoding

Document should be correctly inserted.

Then run a query

postgres=# select * from target, xpath('/enprimeur/vino', x);
ERROR:  could not parse XML document
DETAIL:  input conversion failed due to input error, bytes 0x88 0x73 0x6B
0x75
input conversion failed due to input error, bytes 0x88 0x73 0x6B 0x75
encoder error
line 25: Premature end of data in tag remark line 25
line 25: Premature end of data in tag vino line 3
line 25: Premature end of data in tag enprimeur line 2


select xmltable.* from target, xmltable('/enprimeur/vino' passing x columns
id int, nazev text, remark text);

XMLTABLE is not vulnerable against this bug and result should be correct.

when you do

select x from target

you can see a correct xml without header

but select x::text from target; shows wrong header



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

In mostly functions there the important work is done in parse_xml_decl
function

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

Re: [HACKERS] possible encoding issues with libxml2 functions

[Noah Misch]

On Sun, Mar 12, 2017 at 10:26:33PM +0100, Pavel Stehule wrote:
> 2017-03-12 21:57 GMT+01:00 Noah Misch :
> > On Sun, Mar 12, 2017 at 08:36:58PM +0100, Pavel Stehule wrote:
> > > 2017-03-12 0:56 GMT+01:00 Noah Misch :
> > 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.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] possible encoding issues with libxml2 functions

[Pavel Stehule]

2017-03-12 22:26 GMT+01:00 Pavel Stehule :

>
>
> 2017-03-12 21:57 GMT+01:00 Noah Misch :
>
>> On Sun, Mar 12, 2017 at 08:36:58PM +0100, Pavel Stehule wrote:
>> > 2017-03-12 0:56 GMT+01:00 Noah Misch :
>> > > 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
>
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index 1908b13db5..2786d5b1cb 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -3874,8 +3874,8 @@ 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);
+   len = strlen(datastr);
xpath_len = VARSIZE_ANY_EXHDR(xpath_expr_text);
if (xpath_len == 0)
ereport(ERROR,
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index 1908b13db5..2786d5b1cb 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -3874,8 +3874,8 @@ 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);
+	len = strlen(datastr);
 	xpath_len = VARSIZE_ANY_EXHDR(xpath_expr_text);
 	if (xpath_len == 0)
 		ereport(ERROR,

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] possible encoding issues with libxml2 functions

[Pavel Stehule]

2017-03-12 21:57 GMT+01:00 Noah Misch :

> On Sun, Mar 12, 2017 at 08:36:58PM +0100, Pavel Stehule wrote:
> > 2017-03-12 0:56 GMT+01:00 Noah Misch :
> > > 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

Re: [HACKERS] possible encoding issues with libxml2 functions

[Noah Misch]

On Sun, Mar 12, 2017 at 08:36:58PM +0100, Pavel Stehule wrote:
> 2017-03-12 0:56 GMT+01:00 Noah Misch :
> > 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.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] possible encoding issues with libxml2 functions

[Pavel Stehule]

2017-03-12 0:56 GMT+01:00 Noah Misch :

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

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

 diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c

index f81cf489d2..89aae48cb3 100644
--- 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);
+   len = strlen(datastr);
+
xpath_len = VARSIZE(xpath_expr_text) - VARHDRSZ;
+
if (xpath_len == 0)
ereport(ERROR,
(errcode(ERRCODE_DATA_EXCEPTION),

Regards

Pavel

Re: [HACKERS] possible encoding issues with libxml2 functions

[Noah Misch]

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.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers