Hi Tom
On 21.05.25 22:20, Tom Lane wrote:
> Just when you thought it was safe to go back in the water ...
>
> Experimenting with the improved valgrind leak detection code at [1],
> I discovered that XMLSERIALIZE(... INDENT) has yet a different memory
> leak problem. It turns out that xmlDocSetRootElement() doesn't
> merely install the given root node: it unlinks the document's old
> root node and returns it to you. If you don't free it, it's leaked
> (for the session, since this is a malloc not palloc).
Yeah, I just read the same in the docs
/"returns the unlinked old root element or NULL if the document didn't
have a root element or a memory allocation failed. "/
The xmlsoft examples are a bit misleading though [1]
/*
* Creates a new document, a node and set it as a root node
*/
doc = xmlNewDoc(BAD_CAST "1.0");
root_node = xmlNewNode(NULL, BAD_CAST "root");
xmlDocSetRootElement(doc, root_node);
and [2]
/* Make ELEMENT the root node of the tree */
xmlDocSetRootElement(doc, node);
It seems that xml_parse has the same issue[3]
Should we attempt to free the result of xmlDocSetRootElement() there too? v2
attached.
> The amount of
> leakage isn't that large, seems to be a couple hundred bytes per
> iteration, which may explain why this escaped our notice in the
> previous testing. Still, it could add up under extensive usage.
> So I think we need to apply the attached, back to PG 16.
Definitely. It could add up quickly under heavy usage.
Thanks for fixing it!
Best, Jim
1 - http://xmlsoft.org/examples/tree2.c
2 - http://xmlsoft.org/examples/testWriter.c
3 -
https://github.com/postgres/postgres/blob/f3622b64762bb5ee5242937f0fadcacb1a10f30e/src/backend/utils/adt/xml.c#L1872
From c01b19d747bb4e3ffff3f6eb2f4641c268fe2b21 Mon Sep 17 00:00:00 2001
From: Jim Jones <jim.jo...@uni-muenster.de>
Date: Thu, 22 May 2025 00:55:37 +0200
Subject: [PATCH v2] fix leakage in xmlserialize indent
---
src/backend/utils/adt/xml.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index db8d0d6a7e..a8d5eeda54 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -754,6 +754,7 @@ xmltotext_with_options(xmltype *data, XmlOptionType xmloption_arg, bool indent)
* content nodes, and then iterate over the nodes.
*/
xmlNodePtr root;
+ xmlNodePtr oldroot;
xmlNodePtr newline;
root = xmlNewNode(NULL, (const xmlChar *) "content-root");
@@ -761,8 +762,14 @@ xmltotext_with_options(xmltype *data, XmlOptionType xmloption_arg, bool indent)
xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
"could not allocate xml node");
- /* This attaches root to doc, so we need not free it separately. */
- xmlDocSetRootElement(doc, root);
+ /*
+ * This attaches root to doc, so we need not free it separately...
+ * but instead, we have to free the old root if there was one.
+ */
+ oldroot = xmlDocSetRootElement(doc, root);
+ if (oldroot != NULL)
+ xmlFreeNode(oldroot);
+
xmlAddChildList(root, content_nodes);
/*
@@ -1850,6 +1857,7 @@ xml_parse(text *data, XmlOptionType xmloption_arg,
else
{
xmlNodePtr root;
+ xmlNodePtr oldroot;
/* set up document with empty root node to be the context node */
doc = xmlNewDoc(version);
@@ -1868,8 +1876,13 @@ xml_parse(text *data, XmlOptionType xmloption_arg,
if (root == NULL || xmlerrcxt->err_occurred)
xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
"could not allocate xml node");
- /* This attaches root to doc, so we need not free it separately. */
- xmlDocSetRootElement(doc, root);
+ /*
+ * This attaches root to doc, so we need not free it separately...
+ * but instead, we have to free the old root if there was one.
+ */
+ oldroot = xmlDocSetRootElement(doc, root);
+ if (oldroot != NULL)
+ xmlFreeNode(oldroot);
/* allow empty content */
if (*(utf8string + count))
--
2.34.1
