Re: XML with invalid chars

On 05/09/2011 11:25 PM, Noah Misch wrote:
>
> I see you've gone with doing it unconditionally.  I'd lean toward testing the
> library in pg_xml_init and setting a flag indicating whether we need the extra
> pass.  However, a later patch can always optimize that.
>


I wasn't terribly keen on the idea, but we can look at it again later.

>>
>> Please review and try to break.
> Here are the test cases I tried:
>
> -- caught successfully
> SELECT E'\x01'::xml;
> SELECT xmlcomment(E'\x01');
> SELECT xmlelement(name foo, xmlattributes(E'\x01' AS bar), '');
> SELECT xmlelement(name foo, NULL, E'\x01');
> SELECT xmlforest(E'\x01' AS foo);
> SELECT xmlpi(name foo, E'\x01');
> SELECT query_to_xml($$SELECT E'\x01'$$, true, false, '');
>
> -- not caught
> SELECT xmlroot('<root/>', version E'\x01');


That's an easy fix.


> SELECT xmlcomment(E'\ufffe');


That's a bit harder. Do we want to extend these checks to cover 
surrogates and end of plane characters, which are the remaining 
forbidden chars? It certainly seems likely to be a somewhat slower test 
since I think we'd need to process the input strings a Unicode char at a 
time, but we do that in other places and it seems acceptable. What do 
people think?


> -- not directly related, but also wrongly accepted
> SELECT xmlroot('<root/>', version ' ');
> SELECT xmlroot('<root/>', version 'foo');
>
> Offhand, I don't find libxml2's handling of XML declarations particularly
> consistent.  My copy's xmlCtxtReadDoc() API (used by xml_in when xmloption =
> document) accepts '<?xml version="foo"?>' but rejects'<?xml version=" "?>'.
> Its xmlParseBalancedChunkMemory() API (used by xml_in when xmloption = content)
> accepts anything, even control characters.  The XML 1.0 standard is stricter:
> the version must match ^1\.[0-9]+$.  We might want to tighten this at the same
> time.


We can add some stuff to check the version strings. Doesn't seem 
terribly difficult.

> libxml2's error message for this case is "PCDATA invalid Char value 1"
> (assuming \x01).  Mentioning PCDATA seems redundant, since no other context
> offers greater freedom.  How about:
>
> ereport(ERROR,
> 		(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
> 		 errmsg("invalid XML 1.0 Char \\U%08x", char_val)));
>
>

That would also mean processing the string a unicode char at a time. So 
maybe that's what we need to do.

Thanks for the input.

cheers

andrew