Fix: signed atoi() -> unsigned wrap across IccTagXml ParseXml handlers (#1346)#1349
Merged
xsscx merged 1 commit intoJun 14, 2026
Conversation
InternationalColorConsortium#1346) Sweep of the pattern reported as InternationalColorConsortium#1342/InternationalColorConsortium#1343: XML attributes parsed with atoi() (signed int) were stored into unsigned icUIntNN members/locals, so a negative attribute wrapped to a large unsigned value. Where the conversion was implicit, UBSan's implicit-integer-sign-change flagged it (that is how the two reported sites surfaced); where an explicit (icUIntNN) cast was already present the same wrap happened silently, invisible to the sanitizer. Adds a single file-local helper, icXmlAttrToUInt(), that parses the value and floors any negative (invalid) input to 0, and routes all 19 affected sites through it (11 implicit "Class A" sites + the audit-only "Class B" sites that the fuzzer can never reach). 8- and 16-bit targets keep their explicit width cast, which both narrows the value and suppresses implicit-integer-truncation, so the only behavioral change is that a negative attribute now yields a well-defined 0 instead of a wrapped maximum. The two SeamlessIndicator sites (image tags) are intentionally left to InternationalColorConsortium#1344, which clamps them in the same spirit; the Class C sites (enum / float / int locals with their own bounds checks) are out of scope and unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
colourbill-ctrl
requested review from
ChrisCoxArt,
maxderhak and
xsscx
as code owners
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
2 participants
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.
Summary
Fixes the tracking issue #1346 — the pervasive
atoi()(signed) → unsignedicUIntNNpattern inIccXML/IccLibXML/IccTagXml.cppthat #1342/#1343 were two instances of. A negative XML attribute wrapped to a large unsigned value: caught by UBSan'simplicit-integer-sign-changewhere the conversion was implicit (Class A), and silently where an explicit(icUIntNN)cast already suppressed the check (Class B, invisible to the fuzzer).Change
Adds one file-local helper and routes all 19 affected sites through it:
cicpFields(ColorPrimaries/TransferCharacteristics/MatrixCoefficients/VideoFullRangeFlag),SparseMatrixArray(outputChannels, rows, cols ×2),MultiProcessElement(input/outputChannels),GamutBoundaryDesc(PCS/Device channels). 8/16-bit targets keep their explicit width cast, which also suppressesimplicit-integer-truncation.steps,m_reserved2/m_reserved3,m_nReserved2,nGridGranularity. Same wrap bug, no sanitizer signal — fixed for consistency since no fuzzer will ever resurface them.SeamlessIndicatorimage-tag sites are left to Fix: signed->unsigned SeamlessIndicator conversion in image ParseXml (#1342, #1343) #1344 (same clamp, in flight); Class C (enum/float/bounds-checkedintlocals) is unchanged.Only behavioral change: a negative attribute now yields a well-defined
0instead of a wrapped maximum.Verification (clang-18,
-DENABLE_ASAN=ON -DENABLE_UBSAN=ON -DENABLE_INTEGER_SANITIZER=ON)cicpFields ColorPrimaries="-1"→ pre-fix:IccTagXml.cpp:1172: runtime error: implicit conversion from type 'int' of value -1 ... changed the value to 255; post-fix: clean, profile parses and saves.ColorPrimaries="9"parses unchanged (value passes through; helper only floors negatives).The deterministic regression (script + PoC + CMake registration) lands in a companion test PR, since it touches the fork-gated CI/test paths.
🤖 Generated with Claude Code