Fix memory leak of shaper buffer in ParseCube() when loading .cube files#586
Merged
Conversation
ParseCube() allocates a temporary 'shapers' buffer to stage the LUT_1D_SIZE tone-curve samples before handing them to cmsBuildTabulatedToneCurveFloat() and cmsStageAllocToneCurves(). Both of those copy the sample data into their own storage (AllocateToneCurveStruct() duplicates the segment's SampledPoints via _cmsDupMem, and cmsStageAllocToneCurves() duplicates each tone curve via cmsDupToneCurve), so 'shapers' is never referenced again after cmsFreeToneCurveTriple(curves) runs -- but it was never freed either. This leaks 3 * shaper_size * sizeof(cmsFloat32Number) bytes on every single successful load of a .cube file that carries a LUT_1D_SIZE (1D shaper) entry, e.g. via cmsCreateDeviceLinkFromCubeFile(THR)(). The sibling 3D CLUT path a few lines below already frees its equivalent scratch buffer (_cmsFree(cube->ContextID, lut_table)); this brings the shaper path in line with it. Verified independently: built the library from this source with -fsanitize=address,undefined and reproduced the leak with a minimal LUT_1D_SIZE-only .cube file loaded via cmsCreateDeviceLinkFromCubeFile(); LeakSanitizer reported exactly the 'shapers' allocation at cmscgats.c (48 bytes for shaper_size=4). After adding the _cmsFree() call the same repro is clean (no leak, no use-after-free/heap-corruption from freeing the buffer after its data has been copied out). Also ran the existing testbed/testcms2.c regression suite before/after with no behavioral change.
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.
Summary
ParseCube()(src/cmscgats.c) allocates a temporaryshapersbuffer tostage the
LUT_1D_SIZEtone-curve samples read from a.cubefile beforehanding them off to
cmsBuildTabulatedToneCurveFloat()andcmsStageAllocToneCurves():Both consumers copy the sample data into their own storage rather than
retaining a pointer into
shapers:AllocateToneCurveStruct()duplicates the segment'sSampledPointsvia_cmsDupMem().cmsStageAllocToneCurves()duplicates each tone curve viacmsDupToneCurve().So
shapersis never referenced again aftercmsFreeToneCurveTriple(curves)runs on the line right below — but it is never freed. This leaks
3 * shaper_size * sizeof(cmsFloat32Number)bytes on every successfulload of a
.cubefile that carries aLUT_1D_SIZEentry, e.g. viacmsCreateDeviceLinkFromCubeFile(THR)().The sibling 3D-CLUT branch a few lines below already frees its equivalent
scratch buffer:
This PR brings the shaper path in line with it by adding the missing
_cmsFree(cube->ContextID, shapers);call.Verification
Built the library standalone from this source tree with
-fsanitize=address,undefinedand reproduced the leak with a minimalLUT_1D_SIZE-only.cubefile loaded through the publiccmsCreateDeviceLinkFromCubeFile()API:After adding the
_cmsFree()call, the identical repro is clean (no leakreported, and no use-after-free/heap-corruption from freeing the buffer
after its contents have already been copied out elsewhere — confirmed by
AddressSanitizer instrumentation on the same run). Also exercised a
combined
LUT_1D_SIZE+LUT_3D_SIZEfile through an actualcmsCreateTransform()/cmsDoTransform()round trip with nocorrectness regressions, and ran the existing
testbed/testcms2.cregression suite before/after with identical output.
This is unrelated to #585 (which fixes a separate
Extra-channelmis-indexing bug in
cmspack.c's planar-premul unroll path).