fix(#1932): reject a spectral PCS header whose channel count and range disagree - #1942
Merged
Merged
Conversation
…e disagree CIccPcsXform::Connect() sizes the PCS steps it pushes from the profile header's spectralRange, while the pixel buffers those steps run in are sized from the channel count carried in the spectralPCS signature. Nothing checked that the two agreed. The reproducer's embedded v5 sub-profile declares "rs" + 36 channels alongside spectralRange.steps == 184. CIccApplyCmm::InitPixel() therefore allocates 144 bytes, while pushRef2Xyz() -> pushRad2Cie() -> pushMatrix(3, 184, observer) builds a 3x184 observer matrix. CIccSparseMatrix::MultiplyVector() walks that matrix -- well formed, every column index in range for 184 columns, IsValid() passes -- against the 36-float pixel, and reads past its end on the 37th column: an ASan heap-buffer-overflow, 4-byte read 0 bytes after a 144-byte region (CWE-125). Nothing in the sparse matrix is wrong; the caller handed it a vector shorter than the matrix it was built for. CIccProfile::Validate() already reports the same disagreement as a critical error, but neither iccApplyProfiles nor CIccCmm calls Validate(), so Connect() now makes the check itself and returns icCmmStatInvalidProfile before pushing any step. There is no way to tell which of the two numbers the producer meant, so rejection is the only correct outcome. The scoping mirrors the validator's exactly: the sparse-matrix PCS is excluded, because there the channel count is the length of the encoded matrix blob the pixel carries rather than a sample count, and is independent of the ranges by design. Also considered and rejected: a general invariant that a step chain's first step may not read more channels than the xform declares as its source. pushXYZNormalize() legitimately applies a scratch CIccPcsXform with m_nSrcSamples == 0 and an 81-channel step, so that guard would have broken a working path. New CTest iccdev.spectral-pcs-range-consistency drives Connect() through stub xforms, so its assertions are on returned status and need no sanitizer. It pins the rejections, the deliberate sparse-matrix exclusion, the conformant shapes, and a non-spectral connection. Red-green: removing the guard fails 8 of its assertions while every control still passes.
colourbill-ctrl
requested review from
ChrisCoxArt,
dwtza,
maxderhak and
xsscx
as code owners
August 1, 2026 02:38
xsscx
approved these changes
Aug 1, 2026
xsscx
left a comment
Member
There was a problem hiding this comment.
2026-08-01 10:19:24 UTC
- Fuzzed Overnight
This was referenced Aug 2, 2026
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 #1932.
@xsscx — reproduced your PoC exactly, then found the defect is not where the stack trace
points. Evidence first, because the sparse matrix is innocent.
The sparse matrix is well formed
Instrumenting
CIccSparseMatrix::MultiplyVectorat the moment of the over-read:Monotonic row starts, all within
m_nMaxEntries, every column index ascending and< 184.IsValid()returns true on it. Nothing about the matrix is wrong, andhardening
MultiplyVectorwould be hardening the wrong object — it is handed a bareconst icFloatNumber *and cannot know how long it is.ASan names the buffer that actually overflowed:
144 bytes = 36 floats. That is the CMM pixel buffer, and a 3x184 matrix is being run
over it.
Root cause: the header states its sample count twice, and the two disagree
Your reproducer is a TIFF whose embedded ICC carries an
ICC5/ICCptag, and thatv5 sub-profile is the payload (
-embedded 10003selects it,+10000= "use V5sub-profile if present"). Its header says:
spectralPCS0x72730024—"rs"+ 36 channelsspectralRangeBoth numbers describe the same quantity, and the two halves of the CMM read different ones:
GetNumSrcSamples()/GetNumDstSamples()report, and what
CIccApplyCmm::InitPixel()sizesm_Pixel/m_Pixel2from → 36;CIccPcsXform::Connect()sizes every spectral step it pushes fromspectralRange, sopushRef2Xyz()→pushRad2Cie()→pushMatrix(3, 184, observer)builds a 3x184observer matrix, which
CIccPcsStepMatrix::reduce()converts to the sparse matrix above.Dumping the chain makes the contradiction explicit — note the xform declares 36 source
samples while its first step consumes 184:
MultiplyVectorthen readspVector[36]on the 37th column. CWE-125.The check already exists — the apply path just never asks
CIccProfile::Validate()reports exactly this as a critical error, in thespectralPCSswitch ofCheckHeader():Neither
iccApplyProfilesnorCIccCmmcallsValidate(), so nothing enforced it. Thefix puts the same rule on the apply path.
Fix
One guard in
CIccPcsXform::Connect(), placed right afterm_nSrcSamples/m_nDstSamplesare taken from the neighbouring xforms and before the dispatch switch — the single
choke point every spectral route passes through. Returns
icCmmStatInvalidProfilebeforeany step is pushed.
Rejection is the only correct outcome: there is no way to tell which of the two numbers
the producer meant, and honouring either silently reinterprets the pixel data.
The scoping deliberately mirrors the validator's. The sparse-matrix PCS (
sm) isexcluded, because there the channel count is the length of the encoded matrix blob the
pixel carries rather than a sample count, so it is independent of the ranges by design —
CIccPcsStepSrcSparseMatrixis handed the two separately.iccdev.spectral-pcs-range-consistencypins that exclusion so a later tidy-up cannot quietly widen it.
ConnectFirst()/ConnectLast()need no equivalent: both only push colorimetric steps andnever read
spectralRange.Considered and rejected
I drafted a second, more general guard — "a step chain's first step may not read more
channels than the xform declares as its source". It is wrong:
pushXYZNormalize()legitimately applies a scratch
CIccPcsXformwithm_nSrcSamples == 0and an 81-channelstep, so the invariant would have broken a working path. Recording it here so it does not
get re-proposed.
Test
iccdev.spectral-pcs-range-consistency(.github/ci/regression/) drivesConnect()through stub xforms, so every assertion is on returned status and needs no sanitizer. No
fixture and no I/O, so a corpus change cannot silently disable it. It covers:
rs/ts/es), both directionsof the mismatch;
steps x stepsproduct;Red-green: with the guard removed, 8 assertions fail and every control still passes
— so the failures are attributable to the guard, not to a broken harness.
Verification
IccSparseMatrix.cpp:312, matching the issuestatus 3: Invalid profile, no report.iccin the tree: 255 inTesting/, 24 under.github/ci/iccdev-ci-regression), g++ 15.2.0, Release + LTO,-Wall -Wextra -Wpedantic -WerrorASAN_OPTIONS=detect_leaks=1detect_leaks=0, so a leak would otherwise pass silently)Not caused by this change:
iccdev.hybrid-pipelinefails underctest -j4and passesstandalone (358 s), and its container failure ("Unable to save profile as
ICC/MultSpectralRGB.icc") reproduces identically on pristine master05b5d54fin thesame image — a mounted-volume write issue, not a regression. Same family as the known
tool-coverageparallel flake.On the corpus files and CB tooling you posted in #1931 — I have not harvested those yet;
they look directly useful for the
IccMpeXml.cppatoifamily and I will pick them up there.