Repository navigation
libvmaf/feature: avoid static close() shadowing close(2) (fixes Darwin LTO miscompile) - #1696
Open
StormBytePP wants to merge 1 commit into
Open
StormBytePP wants to merge 1 commit into
StormBytePP wants to merge 1 commit into
Conversation
Thirteen feature extractors define their close callback as
static int close(VmafFeatureExtractor *fex)
That name shadows close(2) from <unistd.h>. In C, a static function is
private to its translation unit. LTO toolchains do not always keep it
that way. When libvmaf is linked into an application with LTO together
with other code that calls close(fd), Apple's ld64 has bound those
close(fd) calls to one of these static functions. The file descriptor
is then used as a VmafFeatureExtractor pointer. The first thing every
one of these callbacks does is load fex->priv, which is at offset 0x38,
so the process dies with EXC_BAD_ACCESS at fd + 0x38 (0x3b for fd 3).
We hit this in a macOS arm64 build with Apple Clang and -flto that
statically links libvmaf with FFmpeg. The crash was in
libavformat's file_close() during avformat_close_input(). It goes away
without LTO, and the same build works on Linux and Windows. No LTO
mode is needed to reproduce it on its own; the only requirement is
that libvmaf's bitcode and a close(fd) caller end up in the same LTO
link. FFmpeg removed the same pattern from its parsers for the same
reason (584c41a4ac, FFV1 parser, and the APV parser after it).
This change renames the callbacks:
- close() -> close_fex() in ciede.c, float_adm.c, float_ansnr.c,
float_moment.c, float_motion.c, float_ms_ssim.c, float_psnr.c,
float_ssim.c, float_vif.c, integer_adm.c, integer_ssim.c and
integer_vif.c. integer_motion.c already uses this name.
- close() -> close_temporal() in speed.c. It sits next to
close_chroma() in the same file.
This is not an API or ABI change. The functions are static, nothing
outside their own file refers to them by name, and they are still
reached only through VmafFeatureExtractor.close. No public header,
exported symbol or struct layout changes, and runtime behaviour is
the same. The patch only removes the libc name from libvmaf's
internal symbols. After it, the library defines no symbol named
close.
Signed-off-by: David C. Manuelda <StormByte@gmail.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
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.
libvmaf/feature: rename static
close()callbacks toclose_fex()Summary
Thirteen feature extractors define their close callback as
This name is the same as POSIX
close(2). When libvmaf is statically linked with LTO on Apple platforms, the linker has bound other code'sclose(fd)calls to one of these static functions. The result is a silent wrong-call-target miscompile that crashes the process. This PR renames the callbacks so libvmaf no longer defines any symbol calledclose.This is not an API or ABI change. The renamed functions are
static. Nothing outside their own file refers to them by name, and they are reached only throughVmafFeatureExtractor.close. No public header, exported symbol, struct layout or runtime behaviour changes.What happens
We statically link libvmaf, FFmpeg and our own library into one image with Apple Clang and
-flto(macOS, arm64). With LTO, ld64 resolved theclose(fd)call in FFmpeg'slibavformat/file.c(file_close()) to one of libvmaf's internalclose(VmafFeatureExtractor *)functions instead of libSystem'sclose.The file descriptor is then treated as a
VmafFeatureExtractor *. The first thing each of these callbacks does is loadfex->priv, which is at offset0x38on 64-bit:nameinitextractflushclosemergeoptionsprivWith fd 3 this faults at
3 + 0x38 = 0x3b, which is exactly what we see:The crash happens on the normal path: open a file, decode successfully, then call
avformat_close_input().Why it only shows up on Darwin + LTO
static closestays private to its object file and the build works.close(fd)caller go through the same LTO link, and the call resolves to the wrong function.So this cannot be reproduced on Linux or Windows. It is still a serious bug, for three reasons:
intthat is used as a pointer.close(fd)in the same LTO image is at risk, not only libvmaf's own code.closethe linker chooses and on the fd value. Here it crashed, but with another layout it could read or free unrelated memory instead.Precedent: FFmpeg made the same change
FFmpeg had the same pattern in its parsers and renamed them for the same reason:
close→ renamed in FFmpeg commit584c41a4ac.static av_cold void close(AVCodecParserContext *)→apv_close, merged upstream (#25211)Once our APV fix was in, libvmaf's 13
static closefunctions were the only definitions of that name left in our link. Renaming them is what fixed the crash.The change
close()→close_fex()inciede.c,float_adm.c,float_ansnr.c,float_moment.c,float_motion.c,float_ms_ssim.c,float_psnr.c,float_ssim.c,float_vif.c,integer_adm.c,integer_ssim.candinteger_vif.c.integer_motion.calready usesclose_fex, so this follows an existing name in the tree.close()→close_temporal()inspeed.c, next to the existingclose_chroma()in the same file.cambi.calready usesclose_cambi.26 lines changed: each function definition and its
.close = ...initializer.Why fix it in libvmaf
We can't control every downstream toolchain, and libvmaf is commonly linked statically into larger media stacks such as FFmpeg builds. The rename costs nothing, matches names already used in the tree, and removes a whole class of symbol collision for every consumer.
Testing
nmon the built library shows noclosesymbol, onlyclose_fex/close_temporallocals.file_close()→avformat_close_input(). With this patch it passes, without any workaround in FFmpeg.