From: Tom Lane <tgl@sss.pgh.pa.us>
To: Jim Jones <jim.jones@uni-muenster.de>
cc: Erik Wienhold <ewie@ewie.name>, Michael Paquier <michael@paquier.xyz>,
        Robert Treat <rob@xzilla.net>,
        Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Regression with large XML data input
In-reply-to: <6ac2f605-2caf-447c-abea-ad45b19833ab@uni-muenster.de>
References: <1691195.1753383449@sss.pgh.pa.us>
 <1721385.1753385039@sss.pgh.pa.us>
 <7a9a9804-01d8-4489-a831-1a222bae6705@uni-muenster.de>
 <aILK3nMlw6pp-XhA@paquier.xyz>
 <CABV9wwOY3pH+pA0R1hSq5g_DXqeDaGWRuoBEE4QwWLfTiw+nKw@mail.gmail.com>
 <1944118.1753467686@sss.pgh.pa.us> <aIbb3TOq3XP923e5@paquier.xyz>
 <314265.1753669007@sss.pgh.pa.us> <aIbk39Uit4oAigig@paquier.xyz>
 <cc0bd778-9730-4ef9-98b3-a965f8895331@uni-muenster.de>
 <457dd309-158f-43cd-81d4-df7284f30d4f@ewie.name>
 <483421.1753727323@sss.pgh.pa.us> <628881.1753733767@sss.pgh.pa.us>
 <93116cb2-e3e2-40bf-be9b-77a2f68fd10a@uni-muenster.de>
 <865404.1753791071@sss.pgh.pa.us>
 <6ac2f605-2caf-447c-abea-ad45b19833ab@uni-muenster.de>
Comments: In-reply-to Jim Jones <jim.jones@uni-muenster.de>
	message dated "Tue, 29 Jul 2025 16:01:19 +0200"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <997667.1753802857.1@sss.pgh.pa.us>
Date: Tue, 29 Jul 2025 11:27:37 -0400
Message-ID: <997668.1753802857@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/997668.1753802857%40sss.pgh.pa.us>
Precedence: bulk

Jim Jones <jim.jones@uni-muenster.de> writes:
> On 29.07.25 14:11, Tom Lane wrote:
>> In the original coding, there was a hazard of the node list getting
>> leaked if the caller passed parsed_nodes == NULL.  Or at least I
>> thought there was.  It may be that all releases of libxml2 are smart
>> enough to free the node list if there's no way to pass it back,
>> but I guess we had reason not to trust it.  Possibly there's something
>> about that in the discussion that led up to 6082b3d5d, though I see
>> I neglected to mention it in the commit message.

> I see.. thanks for explaining.

Re-reading the prior thread, I see that my memory above is quite
faulty: we added the node_list intermediate variable as a way to
detect errors when the return code from xmlParseBalancedChunkMemory
couldn't be trusted.  So I think you're right to question whether we
still need it.  I tried reverting to just passing parsed_nodes, and
I don't see any leak in either the normal or error paths --- so at
least with the quite-old version of libxml2 I'm testing, there is no
such bug.

> I went through the discussions and the libxml2 issue, and I also think
> it is prudent to keep it like that :)

I've got mixed feelings about it now.  I think the $64 question
is whether there are any cases in which xmlParseBalancedChunkMemory
thinks things are fine (and returns a node list) but then we conclude
there's an error, perhaps as a consequence of xmlerrcxt->err_occurred
having become set earlier.  That's a little bit of a stretch.

In any case, I now realize that I broke that scenario yesterday
because I forgot that xml_errsave could throw a longjmp --- so freeing
the node list after calling it is too late :-(

On the whole I'm inclined to revert to the previous coding without
a node_list variable.

			regards, tom lane