From 05eedce6d5393c15432fae63736d92bd60fb6ea8 Mon Sep 17 00:00:00 2001 From: colourbill-ctrl Date: Thu, 11 Jun 2026 13:24:11 -0700 Subject: [PATCH] Fix: leak in XML tag parsing on duplicate tag signatures (#1306) CIccProfileXml::ParseTag created a tag via CIccTag::Create() and then called AttachTag() to transfer ownership to the profile. AttachTag() refuses (returns false) and does NOT take ownership when a tag with the same signature is already present, but ParseTag ignored that result: - the legacy-by-type path unconditionally set bAttached=true, and - the by-name path discarded the return value entirely, so a duplicate tag signature in the input XML leaked the freshly created tag. LeakSanitizer reported the allocation at IccTagXmlFactory.cpp:84 (CIccTagXmlFactory::CreateTag) via ParseTag -> CIccTag::Create. Both paths now delete pTag and fail the parse when AttachTag() rejects the tag, so ownership is always well-defined. Repro (duplicate wtpt): iccFromXml ub-member-access-null-pointer-struct-xmlnode.xml oops.icc Verified with -fsanitize=address: pre-fix reports the 48-byte direct leak from the issue; post-fix is leak-clean. srgbRef/argbRef round-trip unchanged (exit 0, no double-free). Fixes #1306 Co-Authored-By: Claude Opus 4.8 --- IccXML/IccLibXML/IccProfileXml.cpp | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/IccXML/IccLibXML/IccProfileXml.cpp b/IccXML/IccLibXML/IccProfileXml.cpp index c3b8732c1..1e10cb6dc 100644 --- a/IccXML/IccLibXML/IccProfileXml.cpp +++ b/IccXML/IccLibXML/IccProfileXml.cpp @@ -698,7 +698,16 @@ bool CIccProfileXml::ParseTag(xmlNode *pNode, std::string &parseStr) if ((attr = icXmlFindAttr(pTypeNode, "reserved"))) { sscanf(icXmlAttrValue(attr), "%x", &pTag->m_nReserved); } - AttachTag(sigTag, pTag); + //AttachTag refuses (and does not take ownership) when a tag with this + //signature already exists; free pTag to avoid a leak on duplicates. + if (!AttachTag(sigTag, pTag)) { + parseStr += "Unable to attach duplicate tag \""; + parseStr += nodeName; + snprintf(str, strSize, "\" on line %d\n", pTypeNode->line); + parseStr += str; + delete pTag; + return false; + } } else { parseStr += "Unable to Parse \""; @@ -757,8 +766,11 @@ bool CIccProfileXml::ParseTag(xmlNode *pNode, std::string &parseStr) if (tagSigNode->type == XML_ELEMENT_NODE && !icXmlStrCmp(tagSigNode->name, "TagSignature")) { if ((const icChar*)tagSigNode->children != NULL) { sigTag = (icTagSignature)icGetSigVal((const icChar*)tagSigNode->children->content); - AttachTag(sigTag, pTag); - bAttached = true; + //Only flag ownership transfer when AttachTag actually accepts pTag. + //It refuses duplicate signatures without taking ownership, so a + //blanket bAttached=true here would leak pTag on a duplicate tag. + if (AttachTag(sigTag, pTag)) + bAttached = true; } } }