PR #24414 opened by David Tolnay (dtolnay) URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/24414 Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/24414.patch
At the time that this code was introduced into FFmpeg in April 2022 by commit b9a25963f7232433c9370ac369fb668ac0d5cb53, it was correct as written. The latest release of lcms2 at the time was 2.13.1 in which `cmsDeleteContext(NULL)` was a no-op. Since lcms2 version 2.15, released Mar 2023, the same call is no longer a no-op and instead means destroy the global context. The new behavior is justifiable as being consistent with the rest of lcms2's API, where passing NULL for a cmsContext argument means operate on the global context. For example `cmsDupContext(NULL, ...)` constructs a context duplicated from the global context; `cmsPluginTHR(NULL, ...)` registers a plugin into the global context. The upstream commit that introduced the behavior change for `cmsDeleteContext` is: https://github.com/mm2/Little-CMS/commit/a9e4601ceb3a185d4f78cc0cfbd285cf0c399e9d In FFmpeg, the new behavior leads to a memory leak if `ff_icc_context_uninit` is ever called on an `FFIccContext` without `ff_icc_context_init`. Here is why that can happen, and here is how it leads to a memory leak. Naively, uninitializing something which was never initialized seems like it shouldn't happen, but in this case these contexts are *lazily* initialized and *unconditionally* deinitialized. `FFIccContext` is a member of `AVCodecInternal`: // libavcodec/internal.h:148-150 #if CONFIG_LCMS2 FFIccContext icc; /* used to read and write embedded ICC profiles */ #endif It is initialized conditionally by the snippet of `detect_colorspace` shown below, or by similary shaped code in libavcodec/encode.c. The `avci->icc.avctx` guards "has it already been initialized". The early returns beforehand are codepaths for which `icc` will not be initialized, and will remain all-zero as allocated by `av_mallocz` in `ff_decode_internal_alloc`. // libavcodec/decode.c:534-542 if (!(avctx->flags2 & AV_CODEC_FLAG2_ICC_PROFILES)) return 0; sd = av_frame_get_side_data(frame, AV_FRAME_DATA_ICC_PROFILE); if (!sd || !sd->size) return 0; if (!avci->icc.avctx) { ret = ff_icc_context_init(&avci->icc, avctx); Later, `avci->icc` is uninitialized unconditionally by `ff_codec_close`: // libavcodec/avcodec.c:447-480 if (avcodec_is_open(avctx)) { ... #if CONFIG_LCMS2 ff_icc_context_uninit(&avci->icc); #endif So every codec closed without `AV_CODEC_FLAGS2_ICC_PROFILES`, or without an ICC side-data packet, reaches `ff_icc_context_uninit` on a never-initialized context. As part of `cmsDeleteContext(NULL)` destroying lcms2's global context, it runs `cmsUnregisterPlugins()` and destroys `globalContext.MemPool`. Since the global context is shared by every user of lcms2 in the process, closing an FFmpeg codec silently tears down unrelated libraries' lcms2 state. The most visible symptom is that `_cmsRegisterMutexPlugin(NULL, NULL)` nulls out the mutex callbacks, after which `cmsCloseProfile()` leaks each profile's `UsrMutex` and profiles opened later are left without any locking at all. Relevant code in lcms2: - https://github.com/mm2/Little-CMS/blob/lcms2.19.1/src/cmsplugin.c#L967 - https://github.com/mm2/Little-CMS/blob/lcms2.19.1/src/cmsplugin.c#L629 - https://github.com/mm2/Little-CMS/blob/lcms2.19.1/src/cmsplugin.c#L792 - https://github.com/mm2/Little-CMS/blob/lcms2.19.1/src/cmserr.c#L649-L652 - https://github.com/mm2/Little-CMS/blob/lcms2.19.1/src/cmserr.c#L683-L685 The fix is to delete only contexts created by FFmpeg. Signed-off-by: David Tolnay <[email protected]> >From 1bb31782ea444017ec5b59a6cfd6c83f664ecac6 Mon Sep 17 00:00:00 2001 From: David Tolnay <[email protected]> Date: Mon, 7 Sep 2026 17:09:55 -0700 Subject: [PATCH] avcodec/fflcms2: fix memory leak after cmsDeleteContext(NULL) At the time that this code was introduced into FFmpeg in April 2022 by commit b9a25963f7232433c9370ac369fb668ac0d5cb53, it was correct as written. The latest release of lcms2 at the time was 2.13.1 in which `cmsDeleteContext(NULL)` was a no-op. Since lcms2 version 2.15, released Mar 2023, the same call is no longer a no-op and instead means destroy the global context. The new behavior is justifiable as being consistent with the rest of lcms2's API, where passing NULL for a cmsContext argument means operate on the global context. For example `cmsDupContext(NULL, ...)` constructs a context duplicated from the global context; `cmsPluginTHR(NULL, ...)` registers a plugin into the global context. The upstream commit that introduced the behavior change for `cmsDeleteContext` is: https://github.com/mm2/Little-CMS/commit/a9e4601ceb3a185d4f78cc0cfbd285cf0c399e9d In FFmpeg, the new behavior leads to a memory leak if `ff_icc_context_uninit` is ever called on an `FFIccContext` without `ff_icc_context_init`. Here is why that can happen, and here is how it leads to a memory leak. Naively, uninitializing something which was never initialized seems like it shouldn't happen, but in this case these contexts are *lazily* initialized and *unconditionally* deinitialized. `FFIccContext` is a member of `AVCodecInternal`: // libavcodec/internal.h:148-150 #if CONFIG_LCMS2 FFIccContext icc; /* used to read and write embedded ICC profiles */ #endif It is initialized conditionally by the snippet of `detect_colorspace` shown below, or by similary shaped code in libavcodec/encode.c. The `avci->icc.avctx` guards "has it already been initialized". The early returns beforehand are codepaths for which `icc` will not be initialized, and will remain all-zero as allocated by `av_mallocz` in `ff_decode_internal_alloc`. // libavcodec/decode.c:534-542 if (!(avctx->flags2 & AV_CODEC_FLAG2_ICC_PROFILES)) return 0; sd = av_frame_get_side_data(frame, AV_FRAME_DATA_ICC_PROFILE); if (!sd || !sd->size) return 0; if (!avci->icc.avctx) { ret = ff_icc_context_init(&avci->icc, avctx); Later, `avci->icc` is uninitialized unconditionally by `ff_codec_close`: // libavcodec/avcodec.c:447-480 if (avcodec_is_open(avctx)) { ... #if CONFIG_LCMS2 ff_icc_context_uninit(&avci->icc); #endif So every codec closed without `AV_CODEC_FLAGS2_ICC_PROFILES`, or without an ICC side-data packet, reaches `ff_icc_context_uninit` on a never-initialized context. As part of `cmsDeleteContext(NULL)` destroying lcms2's global context, it runs `cmsUnregisterPlugins()` and destroys `globalContext.MemPool`. Since the global context is shared by every user of lcms2 in the process, closing an FFmpeg codec silently tears down unrelated libraries' lcms2 state. The most visible symptom is that `_cmsRegisterMutexPlugin(NULL, NULL)` nulls out the mutex callbacks, after which `cmsCloseProfile()` leaks each profile's `UsrMutex` and profiles opened later are left without any locking at all. The fix is to delete only contexts created by FFmpeg. Signed-off-by: David Tolnay <[email protected]> --- libavcodec/fflcms2.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libavcodec/fflcms2.c b/libavcodec/fflcms2.c index 3c7f3dc07f..5e2215bce5 100644 --- a/libavcodec/fflcms2.c +++ b/libavcodec/fflcms2.c @@ -43,7 +43,8 @@ void ff_icc_context_uninit(FFIccContext *s) { for (int i = 0; i < FF_ARRAY_ELEMS(s->curves); i++) cmsFreeToneCurve(s->curves[i]); - cmsDeleteContext(s->ctx); + if (s->ctx) + cmsDeleteContext(s->ctx); memset(s, 0, sizeof(*s)); } -- 2.52.0 _______________________________________________ ffmpeg-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
