Feedback-ID: i0fe9450f:Fastmail
Date: Tue, 16 Jan 2024 18:14:09 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Dmitry Koval <d.koval@postgrespro.ru>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18274: Error 'invalid XML content'
Message-ID: <ZaZI4WAHZ67ZHq7p@paquier.xyz>
References: <18274-98d16bc03520665f@postgresql.org>
 <1616257.1704651529@sss.pgh.pa.us>
 <5588db01-45be-48ff-b158-85f789bc502c@postgrespro.ru>
 <1314313.1705259848@sss.pgh.pa.us>
 <0eea123f-388a-41ea-9df3-d4606cf9ed8d@postgrespro.ru>
 <ZaRvbaQ8V9uGxXGD@paquier.xyz>
 <35ef78f9-8958-4225-ae7e-aead71b0feed@postgrespro.ru>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="Cj9glJYNyqkopYO8"
Content-Disposition: inline
In-Reply-To: <35ef78f9-8958-4225-ae7e-aead71b0feed@postgrespro.ru>
Archived-At: 
 <https://www.postgresql.org/message-id/ZaZI4WAHZ67ZHq7p%40paquier.xyz>
Precedence: bulk


--Cj9glJYNyqkopYO8
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Mon, Jan 15, 2024 at 06:47:17PM +0300, Dmitry Koval wrote:
> I think that's right (flag XML_PARSE_HUGE shouldn't cause any problems).
> My main doubts are related to the replacement of the
> xmlParseBalancedChunkMemory() function (that haven't argument for pass
> XML_PARSE_HUGE flag) with xmlNewNode() + xmlParseInNodeContext() functions
> (create a fake node and pass the XML_PARSE_HUGE flag to
> xmlParseInNodeContext function).
>
> I'm not sure if this replacement is 100% equivalent (although simple tests
> work the same).

Hmm, it looks like this is actually equivalent in terms of parsing a
well-balanced chunk.  This was suggested in the upstream ticket you
have opened and I find that pretty cool, reusing the trick of a fake
root node to use the other API.  Now, there are two things that we'd
better do here:
- Document in a comment why a fake root node is necessary (aka the
current routines don't give enough control over the limits you'd like
to enforce).
- The top comment of xml_parse() still mentions
xmlParseBalancedChunkMemory() as an effect of 483bdb2afec9, so this
needs to be updated.

The switch xmlParseMemory() -> xmlReadMemory() is recommended by the
upstream docs and the former is deprecated:
https://gnome.pages.gitlab.gnome.org/libxml2/devhelp/libxml2-parser.html#xmlParseMemory

Also, it may be worth double-checking if there are other things marked
as deprecated in the upstream doc, and switch to "newer" things.

It seems like anything discussed here should only be done on HEAD.
I suspect that the buildfarm may get bumpy on that, but let's see.
--
Michael

--Cj9glJYNyqkopYO8
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmWmSOEACgkQnvQgOdby
QH3ppw/9G0FDWZdHNC46kHqcr1JfvR7VPs07dQEhRl983i5bZhgjgRsyuH0jN68G
3IDOyJELCRWv7J2lcq8qAJUTH0Jsayop0Bzu+NQE3VwtSQePqucaRFYmn9UlbFII
gzTl6viOp1HwsktZWZ2uYZ4Lxz+3KV4wdPzhOiaBlpcVsZZBPTrUeEP33y/YX3lV
EEpTbppfXNhPspouUJ4suTTPiMwmim73o2D84nHuy2HmHhvysQ2lD8yePiO3qTXw
fLwg2mw6tFzj8QAwTmH9GYPMwzr4NOqPf9xW7TvUH77/vZr+UcJabHoEbJdHjtCn
X9pxeHelf86Ry8iaD2SPfnfbCpKtRnyu320aAD1QOk1IYxvfHnLaY7qm8LzjbnlI
x0C5353n1lLlUcriymbNpdM4aVxy4CtnJAoWlZWWEEPnC8hGsxG1qz1gX4FAypEX
fxXwDiDioyLVgdUIsJ4/8LZzdSWowCu2Vm1HHbnZK/b4beS8PA8Kd2F9CCgLsYF6
aap/fuIqgn2zOo+EoUifCUeXjKlUblKtFSvwIPbk9rST0RVD0i/HCqb9/WCzcgNd
vzmXKBZEA80lp8FNk+/eMgoBqsFXU1RzVXhwBl1S1TrYBfz0Rr4L5QBIBqnDxg0r
VX6xuLj+MVYDGCyvbVHvlcFpa1T82bHJuciqVVSWa8yi7HkwjNQ=
=gC2d
-----END PGP SIGNATURE-----

--Cj9glJYNyqkopYO8--