Обсуждение: WIP: Fix invalid XML explain plans for track_io_timing

Hi!

The XML output of explain potentially outputs the XML tag names "I/O-Write-Time"
and "I/O-Read-Time", which are invalid due to the slash.

This can easily be seen with this:
   set track_io_timing = on;
   explain (analyze true, buffers true, format xml) select 1;

   [...]
   <I/O-Read-Time>0.000</I/O-Read-Time>
   <I/O-Write-Time>0.000</I/O-Write-Time>
   [...]

Attached is a patch against master that translates slashes to dashes during XML
formatting (very much like spaces are already translated to dashes).

Removing the slash from those property names is another option, but is an
incompatible change to the other formats (neither JSON nor YAML have issues
with '/‘ in key names).

Although the patch fixes the problem for the moment, it is incomplete in that
sense that it continues to check against an incomplete black list. I guess
this is how it slipped in: XML explain was added in 9.0, I/O timings in 9.2.

Checking against an (abbreviated?) white list would be more future proof if new
explain properties are added. Let me know if you consider this a better approach.

I've also done a simple check to see if there are other dangerous
characters used in explain properties at the moment:

   sed -n 's/.*ExplainProperty[^(]*(\s*"\([^"]*\)\".*/\1/p' src/backend/commands/explain.c |grep  '[^-A-Za-z /]'

Result: none.

A similar check could be used at build-time to prevent introducing new property
names that invalidate the XML output (not sure if this could ever reach 100%
safety).

Comments?

--
Markus Winand - winand.at

Markus Winand <markus.winand@winand.at> writes:
> The XML output of explain potentially outputs the XML tag names "I/O-Write-Time"
> and "I/O-Read-Time", which are invalid due to the slash.

Ooops.

> Although the patch fixes the problem for the moment, it is incomplete in that
> sense that it continues to check against an incomplete black list. I guess
> this is how it slipped in: XML explain was added in 9.0, I/O timings in 9.2.

Yeah.  The whitelist approach would look something like
appendStringInfoChar(es->str, strchr(XMLCHARS, *s) ? *s : '-');

which would be quite a few more cycles than just testing for ' ' and '/'.
So I'm not sure it's worth it.  On the other hand, I have little faith
that we wouldn't make a similar mistake in future.
        regards, tom lane

I wrote:
> Markus Winand <markus.winand@winand.at> writes:
>> The XML output of explain potentially outputs the XML tag names "I/O-Write-Time"
>> and "I/O-Read-Time", which are invalid due to the slash.

> Ooops.

After further thought I decided we should go with the whitelist solution.
The extra time needed to produce XML-format output isn't really likely to
bother anyone.  And given that this bug escaped notice for several years,
it seems likely that the next time somebody decides to be creative about
picking a tag name, we might not notice an XML syntax violation for
several more years.  So a future-proof fix seems advisable.

I pushed a patch using the strchr() approach.  Thanks for reporting this!
        regards, tom lane