fix(#1779): skip an unserializable tag instead of discarding the whole XML document - #1884
Merged
Merged
Conversation
…e XML document
CIccProfileXml::ToXmlWithBlanks returned false as soon as any one tag's ToXml()
failed, so iccToXml wrote no file at all for a profile carrying a single corrupt
tag, while iccToJson skipped that tag and produced a document from the same
input. Reported as "Unable to output tag with type swpt | Json Ok"; the 'swpt' is
incidental -- it is just the first tag that fails, not an unsupported type.
Bring the XML writer to parity with the JSON writer at the two levels where the
container is keyed by name and an omission is representable:
* IccProfileXml.cpp -- top-level tags
* IccTagXml.cpp -- tagStructType members
Both now rewind the opening element fragment that was appended before ToXml() was
called, emit an XML comment naming what was dropped, and continue. Neither
records the skipped entry in offsetTags, so a later tag at the same offset cannot
emit a SameAs reference to something no longer in the document.
The other two emit points stay strict, deliberately:
* CIccTagXmlArray::ToXml -- array elements are positional and this format
cannot express a hole (ParseXml sizes the array by counting element children,
then rejects any unfilled slot), so a skip would silently renumber every
later element. JSON can keep the slot because it has a null placeholder.
* CIccMpeXmlTintArray::ToXml -- the array is what defines the element, and the
JSON writer returns false here too, so relaxing it would exceed parity.
Both now merely fail their own tag, which the profile-level skip absorbs, so the
document is still written.
Also fixes a pre-existing markup defect this exposed: CIccTagXmlStruct and
CIccTagXmlArray wrote "<privateStruct .../>" and "<privateArray .../>"
self-closed while still emitting the matching end tag, so any profile using an
unregistered struct or array signature serialized to XML that libxml2 rejects
with "Opening and ending tag mismatch". It was unobservable while such documents
were being discarded wholesale.
Comment text is escaped through a new icFixXmlComment(): entity escaping does not
apply inside a comment, and a fuzzed signature may contain '-', which would
otherwise close or malform the comment.
Adds .github/ci/regression/xml-writer-graceful-degrade.cpp, driving both reported
PoCs from memory through CIccProfileXml::Read/ToXml and parsing the result back
with libxml2 to assert it is well-formed -- the assertion that fails if either
fix is reverted.
Retargets the #1394 ui08-array DoS assertions, which used a non-zero iccToXml
exit code as a stand-in for "the size cap fired". That stand-in only held while
one bad tag aborted the whole document; the tool now correctly exits 0 here. The
checks instead assert the properties that matter -- the output does not balloon,
the over-cap <Array> is absent, and the refusal is recorded -- which is strictly
stronger than the old proxy. Verified by removing the cap: the output balloons to
79,692,671 bytes and the updated test fails.
Verified byte-identical output against master for all 237 Testing/**/*.icc
profiles that produce output; no regressions. Full ctest 121/122, the sole
failure being the pre-existing spectral-tiff-preview environment skip (missing
python imagecodecs).
colourbill-ctrl
requested review from
ChrisCoxArt,
dwtza,
maxderhak and
xsscx
as code owners
July 28, 2026 19:04
Merged
11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1779.
Implements the direction ruled in #1779 (comment) — Option A, resolve toward parity by making the XML writer degrade gracefully.
The defect
CIccProfileXml::ToXmlWithBlanksreturnedfalseas soon as any one tag'sToXml()failed, soiccToXmlwrote no file at all for a profile carrying a single corrupt tag, whileiccToJsonskipped that tag and produced a document from the same input. Theswptin the reported message is incidental — it is simply the first tag that fails, not a type the XML side lacks a handler for.What changed, and what deliberately did not
Investigating each of the four emit points against the reader (not just the writer) showed only two of them should change. The JSON writer already makes this same distinction, so the policy has an in-repo precedent rather than being invented here.
IccProfileXml.cpptop-level tagscontinueIccTagXml.cppstruct memberscontinueIccTagXml.cpparray elementsIccMpeXml.cpptint arrayreturn falseCIccTagXmlArray::ParseXmlsizes the array by countingXML_ELEMENT_NODEchildren of<ArrayTags>and then rejects the profile if any slot is left unfilled ("Undefined Array Tag at index"). A bare comment would silently shrink the array and renumber every later element; a placeholder element would not re-parse. JSON can keep the slot only because it has anull"type"placeholder, which XML has no equivalent for here.CIccMpeXmlTintArray's array is what defines the element, andCIccMpeJsonTintArray::ToJsonreturnsfalsethere too — so relaxing it would exceed parity, not achieve it.Both strict sites now merely fail their own tag, which the profile-level skip absorbs, so the document is still written either way.
The two relaxed sites rewind the opening element fragment appended before
ToXml()was called, emit a comment naming what was dropped, and continue. Neither records the skipped entry inoffsetTags, so a later tag at the same offset cannot emit aSameAsreference to something no longer in the document.Second defect, surfaced by validating the new output
CIccTagXmlStructandCIccTagXmlArraywrote<privateStruct .../>and<privateArray .../>self-closed while still emitting the matching end tag, so any profile using an unregistered struct or array signature serialized to XML that libxml2 rejects withOpening and ending tag mismatch. Pre-existing and unrelated in cause, but it sits directly in this change's blast radius: without it the second PoC's newly-produced document is unparseable, and the regression test could not assert anything meaningful. Fixed here so the feature actually delivers usable output.Comment text goes through a new
icFixXmlComment(): entity escaping does not apply inside an XML comment, and a fuzzed signature may legitimately contain-, which would otherwise malform or close the comment.Test
.github/ci/regression/xml-writer-graceful-degrade.cppdrives both reported PoCs from memory throughCIccProfileXml::Read/ToXmland parses the result back with libxml2 to assert it is well-formed — the assertion that fails if either fix is reverted. Registered in both CMake call lists.Red/green proven independently:
ToXml returned false, no document, not well-formed)/>markup fix → exactly one assertion fails (struct member -- emitted XML is not well-formed)#1394 assertion retargeted
The
issue-1394ui08-array DoS test used a non-zeroiccToXmlexit code as its stand-in for "the size cap fired". That stand-in only held while one bad tag aborted the whole document; the tool now correctly exits 0 on that fixture. The cap itself is untouched and still fires — output is 850 bytes with zero<Array>elements.The assertions now check the properties that actually matter, which is strictly stronger than the old proxy: the output must not balloon, the over-cap
<Array>must be absent, and the refusal must be recorded. Verified by removing the cap: output balloons to 79,692,671 bytes and the updated test fails.The two diagnostics are also now qualified (
- tag skipped/- sub-tag skipped), because an unqualifiedUnable to output ...on stdout would otherwise describe a run that succeeded and exits 0 — the exact misreading that broke #1394. The leading text is kept verbatim so existing searches still match.Verification
masterfor all 237Testing/**/*.iccprofiles that produce output — zero regressions. (The 15 that produce nothing fail atRead, a different path, on master too.)-Wall -Wextra -Wpedantic -Werrorgate 0 warnings, 0 errorsctest121/122 — the sole failure is the pre-existingspectral-tiff-previewenvironment skip (ModuleNotFoundError: No module named 'imagecodecs')detect_leaks=1<MemberTags>still parsesKnown limitation, stated rather than hidden
The skip comment records the loss in the emitted XML, but it is not retained if that XML is parsed back and re-serialized — the tag is then simply gone with no marker. This is inherent to Option A and still ahead of the JSON writer, which leaves no marker at all; the affected input is already corrupt.
Out of scope — filed separately
Two further pre-existing defects were found while validating and are not touched here:
CIccTagXmlArray::ToXmlwrites aStructSignatureattribute, butParseXmlrequires an<ArraySignature>element, so aprivateArraynever round-trips (Unable to find ArraySignature). Needs a call on which side is canonical.UnknownElementemits an unterminated start tag (... OutputChannels="0"FF5...), malformed onmastertoday forTesting/CalcTest/uio-CIccCalculatorFunc-CheckUnderflowOverflow-IccMpeCalc_cpp-Line4238.icc.