Fix: CIccXform::Create overloads leak/double-free pProfile via bOwnsProfile (#1308)#1347
Merged
xsscx merged 1 commit intoJun 14, 2026
Conversation
…rofile (InternationalColorConsortium#1308) Follow-up to InternationalColorConsortium#1304/InternationalColorConsortium#1305. CIccXform::Create owns pProfile only on success (SetParams hands it to the new xform); the tag-based overload returned NULL on its failure paths without freeing the owned profile, leaking it (InternationalColorConsortium#1308), while the no-tag overload (post-InternationalColorConsortium#1305) freed it unconditionally -- which double-freed a profile borrowed from a live xform at the MCS-link path. Adds a bool bOwnsProfile=true parameter to the three pointer-based Create overloads (CIccXform no-tag, CIccXform tag, CIccXformMpe). Every failure path now frees pProfile only when the caller owns it, so: - owned callers (the default) get a leak-free Create on all failure paths (closes the InternationalColorConsortium#1308 tag-overload leak), and - the two borrowed callers -- CheckPCSConnections' HToS injection and Begin's MCS-link conversion -- pass bOwnsProfile=false, so a failed Create no longer deletes a profile still owned by a live xform. Also removes the now-redundant delete in the reference-based Create overload: the pointer Create it delegates to already owns and frees the copy on failure, so the extra delete had become a double-free after InternationalColorConsortium#1305. Inline comments at each overload and call site document the ownership contract and when to pass bOwnsProfile=false. Success paths are unchanged (ownership is still transferred via SetParams and released by the borrowed callers' ShareProfile()). Verified with AddressSanitizer/LeakSanitizer: the tag-overload AddXform path leaks 608 B pre-fix and is clean post-fix; valid conversions and the CheckPCSConnections/MCS-link chains run without leak, double-free, or UAF. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
colourbill-ctrl
requested review from
ChrisCoxArt,
maxderhak and
xsscx
as code owners
colourbill-ctrl
added a commit
that referenced
this pull request
Jun 14, 2026
…1308/#1348) The CI tool-test job and ICCDEV_TEST_ENV both export ASAN_OPTIONS=...detect_leaks=0 to silence unrelated leaks in the smoke-tested tools. This LSan-based leak regression inherited that and therefore PASSED even on pre-fix master where the profile leaks 608 bytes -- a false green that defeats the test's purpose. Launch the test through `cmake -E env ASAN_OPTIONS=detect_leaks=1` (the same override mechanism the script tests use) so leak detection is forced back on for this test's own process. Set ONLY detect_leaks=1: appending halt_on_error=0 makes the runtime print the leak but exit 0 (ctest would still pass), and a ':' option separator is mis-parsed and silently disables the check -- both verified. Verified on a clean build: under the CI env (detect_leaks=0 exported) the test now FAILS with the 608-byte leak on pre-fix master and PASSES with the #1347 fix applied. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
Resolves #1308 (follow-up to #1304/#1305).
CIccXform::Createtakes ownership ofpProfileonly on success (SetParamshands it to the new xform). Two defects flowed from that:Create(CIccProfile*, CIccTag*, …)overload returnedNULLon every failure path without freeing the owned profile — soAddXform(CIccProfile*, CIccTag*)leaked it (the call site even comments "CIccXform::Create has already deleted the profile", which was aspirational for this overload).pProfileunconditionally on failure. At the MCS-link path inBegin()it is called with a profile borrowed from a live xform, so a failedCreatefreed a profile that xform still owned → double-free/UAF.Fix (Option B — ownership parameter)
Adds
bool bOwnsProfile = trueto the three pointer-basedCreateoverloads (CIccXformno-tag,CIccXformtag,CIccXformMpe). Each failure path freespProfileonly when the caller owns it:Createon all failure paths — closes the Leak: sibling CIccXform::Create / CIccXformMpe::Create overloads leak pProfile on failure paths (follow-up to #1304) #1308 tag-overload leak.CheckPCSConnections' HToS injection andBegin's MCS-link conversion — passbOwnsProfile=false, so a failedCreateno longer deletes a profile a live xform still owns.Also removes the now-redundant
deletein the reference-basedCreateoverload: the pointerCreateit delegates to already owns and frees the copy on failure, so that extra delete had become a double-free after #1305.Inline comments at each overload and call site document the ownership contract and when to pass
false. Success paths are unchanged (ownership still transfers viaSetParams, released by the borrowed callers'ShareProfile()).CIccXformMpe::Createcurrently has no callers; it is updated for family consistency so any future caller is leak-safe.Verification (AddressSanitizer / LeakSanitizer, clang-18)
AddXformpath (profile + NULL tag): 608 B leaked pre-fix, clean post-fix.CIccPcsXformleak (fixed separately by Fix: leak of CIccPcsXform in CIccCmm::CheckPCSConnections on Connect failure (#1337) #1338), confirming this change introduces no new heap error.Notes
IccProfLib/), no fork-gate split needed..github/+Build/Cmake). Merge this fix first; the test PR goes green once this lands.🤖 Generated with Claude Code