fix(#1980): reject a non-physical encoding white-point luminance - #1984
Merged
Conversation
CIccDefaultEncProfileConverter::ConvertFromParams reads the colour-space white-point luminance out of the encoding parameters and derives the whole viewing-conditions block from it: it scales the illuminant and surround XYZ written into the spectralViewingConditions tag, defaults the ambient luminance La and the viewing surround Lsw, and becomes the CAM Yb parameter and the divisor of the surround ratio. ICC.2:2023 12.2.3.2.6 defines 'wlum' as a luminance in cd/m2 and admits no zero exception. The value is profile-supplied, so a crafted profile could make it zero, negative or non-finite. #1817 guarded the surround-ratio division against a zero, but every other consumer in the block still read the same value, and the CAM adapting-luminance guard added for #1950 contains only the last of those consequences. Validate the luminance once, where it is read, and reject the parameters when it is not finite and strictly positive. The read is hoisted above the viewing-conditions tag allocation so the rejection path has nothing to release but the profile itself; placing the check after that allocation leaks the tag on all four reject inputs (96 bytes each under LSan). This deliberately changes the #1817 contract: a zero white-point luminance previously converted with icEncConvertOk, having resolved the guarded ratio to Dark surround, and is now icEncConvertBadParams. The surround-ratio guard is kept - its Lsw term is still load-bearing, since Lsw is a separate profile-supplied value with no such precondition - and its comment now records which of its terms the new check makes redundant. Extend the existing regression to a table of cases rather than replacing its input: zero (the #1817 value), negative, NaN and infinite are asserted rejected, and a plain positive luminance plus an absent member - the common case, which defaults to 100 cd/m2 - are asserted still accepted, so the guard cannot silently narrow the accepted range. All four reject cases fail against master and both controls pass.
colourbill-ctrl
requested review from
ChrisCoxArt,
dwtza,
maxderhak and
xsscx
as code owners
August 5, 2026 17:59
Member
|
@colourbill-ctrl Yes, Thank You. Excellent Findings!
|
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 #1980.
Developed from the QA branch
ci-qa-issue-1980(@xsscx,7c8dcb71), which is where thespec anchor and the shape of the check come from. The code here is written fresh, and
reviewing that branch turned up two things worth correcting before this lands — those are
below rather than folded in silently.
The defect
CIccDefaultEncProfileConverter::ConvertFromParamsreads the colour-space white-pointluminance out of the encoding parameters and then derives the entire viewing-conditions
block from it:
spectralViewingConditionsTagIccEncoding.cpp:418-420La:423La):427-437Lsw:441Ybparameter:449:466ICC.2:2023 12.2.3.2.6 defines
wlumas a luminance in cd/m², and supplies no exceptionadmitting zero. The value is profile-supplied, so a crafted profile controls it.
#1817 guarded only the last row of that table. Every other consumer still read the same
value, and the CAM adapting-luminance guard added for #1950 contains only the
Laconsequence — after the bad value has already been written into the tag. So a negative
luminance still propagated a negative illuminant and surround XYZ into a profile that the
converter then reported as successfully built.
This validates the luminance once, where it is read, and rejects the parameters when it is
not finite and strictly positive.
Two corrections to the QA branch
1. The check's placement leaks the viewing-conditions tag
On the branch the check sits after
pCond = CIccTag::Create(icSigSpectralViewingConditionsType)but before
pIcc->AttachTag(icSigSpectralViewingConditionsTag, pCond), sodelete pIcconthe reject path never reaches it. Built at that exact placement and run under ASAN with
ASAN_OPTIONS=detect_leaks=1:96 bytes per reject input, once for each of the four. Worth noting that CI's own ctest runs
with
detect_leaks=0, so this would not have surfaced there.Here the read and the check are hoisted above that allocation instead, so the reject path
has nothing to release but the profile itself. Same build, same test, this placement: clean.
2. The test change dropped #1817's coverage
The branch swaps the test input from
0.0fto-1.0fand flips the expectation. Thatremoves zero — the value #1817 was actually about — from the suite at the same moment its
contract changes, so nothing would have recorded the change.
The regression is extended into a table of cases instead of having its input replaced:
icEncConvertBadParamsicEncConvertBadParamsicEncConvertBadParamsicEncConvertBadParams100.0ficEncConvertOk, profile producedicEncConvertOk, profile producedThe last two are controls: the guard must not narrow the accepted range, and the absent
member — overwhelmingly the common case, defaulting to 100 cd/m² — has to keep working.
Behaviour change
This changes the #1817 contract deliberately, and reviewers should be looking at it:
a zero white-point luminance previously converted with
icEncConvertOk, having resolvedthe guarded ratio to Dark surround. It is now
icEncConvertBadParams.The surround-ratio guard from #1817 is kept rather than removed. Its
Lwterms are nowredundant, but its
Lswterm is still load-bearing —Lswis a separate profile-suppliedvalue with its own default and no equivalent precondition, so a non-finite
Lswstillresolves the ratio to
0.0fand falls through to Dark surround. The comment there nowrecords which of its terms this check makes redundant, so the next reader is not misled by
a rationale that no longer holds.
Verification
spectral-tiff-preview, a local missingimagecodecspython module, unrelateddetect_leaks=1)-Wall -Wextra -Wpedantic -Werror, both changed TUspreflight-safety-checks.shcodeqlnot installed locally)isfiniteguard survives optimisationScope
The check is deliberately confined to
wlum. While reviewing the surrounding function Ifound a separate, pre-existing memory-safety defect that this PR does not touch:
IccEncoding.cpp:237takespLumMtxfrompParams->FindElemOfType(icSigCeptLumaChromaMatrixMbr, ...)— a borrowed pointer, exactly as
pWhitePt,pSurroundandpSegCurveare treated in thesame function — and then
deletes it at:259while it is still in the struct's elementlist.
icConvertEncodingProfiledeletespParamsat:666, whose destructor frees theelements again. A profile carrying a
ceptLumaChromaMatrixMbrof 9 or more values reachesit. That is a double free on profile-controlled input and wants its own issue and its own
regression rather than being folded in here; happy to file it separately if you would like,
@xsscx.