Repository navigation
Memory leak in mxml_load_data() error handling has regressed since the fix for #278 #354
Description
Filing here as requested. I sent this to the security address first and was asked to open it as an issue instead.
The leak fixed in #278 is back. I think it returned with the 4.x rewrite rather than being a new bug.
Reproducer
Two bytes, a less-than followed by a space:
mxml_options_t *o = mxmlOptionsNew();
mxml_node_t *t = mxmlLoadString(NULL, o, "< "); /* returns NULL */
if (t) mxmlDelete(t);
mxmlOptionsDelete(o);Under AddressSanitizer, 50 iterations of that against v4.0.4:
Direct leak of 4400 byte(s) in 50 object(s) allocated from:
#2 mxmlNewElement mxml-node.c:541
Indirect leak of 50 byte(s) in 50 object(s) allocated from:
#1 mxmlNewElement mxml-node.c:542
SUMMARY: AddressSanitizer: 4450 byte(s) leaked in 100 allocation(s).
89 bytes per call. mxmlLoadString() returns NULL, so the caller has no handle to free anything with. A document with attributes leaks more, around 340 bytes per call for <a b="1".
The whitespace is what selects the path. "<a" takes a different branch and does not leak.
| build | result |
|---|---|
| v4.0.4 | leaks |
master e6824d8 |
leaks |
Why it happens
Line numbers from master at e6824d8.
mxml_load_data() creates the element at 1325 and records it in first at 1362. There are two goto error between those two points:
1325: if ((node = mxmlNewElement(parent, buffer)) == NULL) { ...; goto error; }
1332: if (mxml_isspace(ch))
1336: if ((ch = mxml_parse_element(...)) == EOF)
1337: goto error; /* first is still NULL here */
1355: if (options && options->sax_cb)
1358: if (!(options->sax_cb)(..., MXML_SAX_EVENT_ELEMENT_OPEN))
1359: goto error; /* and here */
1362: if (!first)
1363: first = node;The error label already does the right thing:
error:
mxmlDelete(first);
free(buffer);
return (NULL);but on those two paths first has not been set yet, so it deletes nothing, and the element node plus any attributes already parsed into it become unreachable.
Same shape as #278, where the reproducer was a less-than followed by a newline.
Suggested fix
Record the node as soon as it exists, so the error label can free it:
if ((node = mxmlNewElement(parent, buffer)) == NULL)
{
_mxml_error(options, "Unable to add element node to parent <%s> on line %d.", ...);
goto error;
}
+
+ if (!first)
+ first = node;The existing if (!first) first = node; at 1362 then becomes a no-op and can stay or go. Happy to open a PR with this plus a regression test if that is useful, just say the word.
Scope, and what held up (click to expand)
I found this with a libFuzzer harness over mxmlLoadIO() under ASan and UBSan, seeded from your own afl-input/ corpus and driven with your xml.dict.
One hour on eight workers grew the corpus to roughly 56,000 inputs at 898 edges and produced no crashes at all. No out of bounds access, no use after free, no integer UB anywhere in the parser. This leak was the only thing it found, and I think that is worth saying plainly.
For what it is worth, the make afl target builds with COMMONFLAGS="-g", so no LeakSanitizer. That is probably why a regression of a 2021 leak fix could sit through two releases without anyone noticing.
Activity
michaelrsweet commentedon Aug 19, 2026
I don't think this is a regression, the code in question didn't change and still contains the old fixes.
michaelrsweet commentedon Aug 19, 2026
The issue with moving the "first = node" test is that there are multiple places nodes are created. Investigating...
spartan8806 commentedon Aug 19, 2026
Your comment made me go back and check v3.3.1 properly, and you're right. The same structure is there, so this is long-standing rather than something 4.x introduced. I should have tested that before calling it a regression.
On the multiple creation sites, I think that concern points at a smaller fix than the one I suggested. Between
mxmlNewElement()andif (!first) first = node;there are three exits, and one of them already cleans up:The middle one is the fix from #278. So rather than moving the
firstassignment, the same two lines on the other two exits would match what is already there and would not touch any other creation site:That also explains why it survived #278: the fix covered the
/path, and the whitespace path was never reached by it.Happy to send that as a PR with a regression test if it is useful.
Fix potential memory leak when loading bad XML (Issue #354)
michaelrsweet commentedon Aug 20, 2026
This should fix it for all error paths: