Add CodeQL + CTest for the #1356/#1359 patterns; fix the latent siblings they surface (#1361)#1365
Merged
Merged
Conversation
…atterns (#1361) Two custom queries generalising the #1355/#1356/#1359 findings so the same classes of bug are caught statically: * signature-serialized-without-zero-guard.ql -- an ICC four-byte signature serialized via the icGetSig* text helpers does not round-trip when zero (icGetSig*(0) -> "NULL"; icGetSigVal("NULL") -> 0x4E554C4C). Flags a header field that the JSON serializer guards with `field ? helper(...) : ""` but a *ToXml* serializer emits unconditionally. Validated: flags cmmId, deviceClass and creator on master; clean once they are guarded. * version-blind-space-validation.ql -- a header colour space validated by the version-blind CIccInfo::IsValidSpace() directly in an `if` condition that never consults m_Header.version, so iccMAX-only spaces (ncXXXX) pass on a v2/v4 profile. Validated: flags the DeviceLink PCS check on master; clean once it is version-gated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…queries Close the issues the two new CodeQL queries flag, so both run clean: * IccProfileXml.cpp: guard the zero case for the remaining header signature fields the JSON serializer already guards -- PreferredCMMType (cmmId), ProfileDeviceClass (deviceClass) and ProfileCreator (creator). Like the data colour space / PCS fix in #1356, a zero value was serialized as the literal "NULL" and reparsed by icGetSigVal("NULL") to 0x4E554C4C, corrupting it on an iccToXml -> iccFromXml round-trip. Emit an empty element for zero instead. * IccProfile.cpp: version-gate the DeviceLink PCS colour-space check the same way #1359 gated the data colour space. IsValidSpace() is version-blind and accepts the iccMAX N-channel spaces (ncXXXX) for any version, so a v2/v4 DeviceLink could carry an ncXXXX PCS unflagged; it is now a critical error reporting the raw value. Verified: zero cmmId/deviceClass/creator round-trip as 0 (not 0x4E554C4C); a v4 DeviceLink with an ncXXXX PCS is now flagged; v5 and normal v2/v4 profiles are unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Generalises the #1358 data-colour-space round-trip test to the remaining header signature fields the #1361 fix guards. Zeroes cmmId / deviceClass / creator in a real profile, serialises to XML and parses back, and fails if any field is corrupted to 0x4E554C4C ("NULL") instead of round-tripping as zero. Companion to the static signature-serialized-without-zero-guard.ql query. Verified to FAIL on master (all three corrupted) and PASS with the writer guards. Plain functional test; skips cleanly when the tools/fixture are absent. Reuses the committed nodata-colourspace-1356.icc fixture. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
colourbill-ctrl
requested review from
ChrisCoxArt,
dwtza,
maxderhak and
xsscx
as code owners
github-actions
Bot
added
Testing
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.
Closes #1361. In-repo branch (re-opened from #1363 per maintainer request so the Script/CodeQL CI runs against it — fork PRs hit the
WORKFLOW_TRIGGER_HELPERunbound-variable limitation). Supersedes #1363.Adds the CodeQL/CTest coverage to "identify same/similar issues" from #1356, #1358 and #1359 — and fixes the real latent instances the new queries surface, so both queries run clean.
CodeQL queries (detection)
signature-serialized-without-zero-guard.ql— the #1356 round-trip class. An ICC four-byte signature serialized through theicGetSig*text helpers does not round-trip when zero:icGetSig*(0)emits the literal"NULL", andicGetSigVal("NULL")returns0x4E554C4C. Flags a header field the JSON serializer guards withfield ? helper(...) : ""but a*ToXml*serializer emits unconditionally. Understands both ternary andif (field != 0)guard idioms.version-blind-space-validation.ql— the #1359 class. A header colour space validated by the version-blindCIccInfo::IsValidSpace()directly in anifcondition that never consultsm_Header.version, so iccMAX-only spaces (ncXXXX) pass on a v2/v4 profile.Both validated against a locally-built CodeQL DB (CLI 2.25.6):
mastercmmId,deviceClass,creator)Fixes the queries surface
IccProfileXml.cpp— guard the zero case forPreferredCMMType(cmmId),ProfileDeviceClass(deviceClass),ProfileCreator(creator). Same"NULL"→0x4E554C4Ccorruption as Fix: XML serializer corrupts zero header colour-space signature on round-trip (#1355) #1356; now emit an empty element for zero.IccProfile.cpp— version-gate the DeviceLink PCS colour-space check exactly as Fix: version-aware data colour space validation; reject iccMAX-only spaces on v2/v4 (#1355) #1359 gated the data colour space. A v2/v4 DeviceLink with anncXXXXPCS was accepted unflagged; now a critical error reporting the raw value.CTest (dynamic)
iccdev-issue-1361-header-sig-roundtrip-regression.sh— generalises the #1358 round-trip test to cmmId/deviceClass/creator. Verified FAIL on master / PASS on this branch. Registered asiccdev.issue-1361-header-sig-roundtrip-regression.Note on scope
Bundles detection (the queries/test #1361 asked for) with the small fixes those detectors surface, so the committed queries are green. The two source fixes are each a direct analog of #1356 / #1359 and isolated in their own commit — happy to split if preferred.
🤖 Generated with Claude Code