Repository navigation
path: serialize iconv_open and iconv_close - #7404
Open
stig-codes wants to merge 1 commit into
Open
stig-codes wants to merge 1 commit into
stig-codes wants to merge 1 commit into
Conversation
Since macOS 14 the system iconv is FreeBSD's Citrus implementation, whose iconv_open takes one process-wide pthread rwlock twice and whose iconv_close takes it once (`ci_lock` in citrus/citrus_iconv.c). A thread that is waiting for a macOS pthread rwlock when a signal arrives can lose its wakeup: every waiter then blocks forever, and no thread holds the lock. We open and close a converter for every directory that we iterate and every reference name that we normalize whenever core.precomposeunicode is set, which git and libgit2 set on APFS. So a multithreaded program that handles any signal - a SIGCHLD handler for its child processes is enough - can hang forever in a reference lookup, a status, a diff, or anything else that iterates the working directory. Hold a process-wide mutex around iconv_open and iconv_close. Only one thread is then ever inside them, so iconv's lock never has a waiter and there is no wakeup to lose. The conversions themselves (iconv) do not take that lock and are not serialized.
This branch has not been deployed
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.
Fixes #7403.
On macOS 14 and later,
iconv_openandiconv_closetake a process-wide pthread rwlock that can lose a wakeup when a waiting thread takes a signal. Once that happens, every thread that opens or closes a converter blocks forever. We open and close a converter for every directory we iterate and every reference name we normalize whenevercore.precomposeunicodeis set, which is the default on APFS. So a multithreaded program that handles any signal can hang in a reference lookup, a status or a diff. #7403 has the details, the Apple source references and a reproducer.This change holds a process-wide
git_mutexaroundiconv_openandiconv_close. The mutex is created bygit_fs_path_iconv_global_init, whichgit_libgit2_initruns alongside the other util-layer inits; it is freed throughgit_runtime_shutdown_register. With only one thread ever inside those calls, iconv's own lock never has a waiter. The conversions themselves (iconv()) don't take that lock and stay unserialized.Notes for review:
git_fs_path_iconv_init_precomposenow setsmapto(iconv_t)-1before it takes the lock, so a failed lock leavesicin a stategit_fs_path_iconv_clearaccepts. A lock failure is reported asGIT_ERROR_THREAD, as inmwindow.c.mwindow.candopenssl.c. On POSIX,git_mutex_lockispthread_mutex_lock, which returns a positive errno, so a< 0check would never fire.GIT_I18N_ICONVcompile all of this out, including the init entry.tests/util/init.cexercises its init and shutdown.Testing, on macOS 26.5.1 with an Apple M3 Ultra:
GIT_USE_ICONVin place ofGIT_I18N_ICONV, went from 3 of 3 runs wedged to 3 of 3 kept going.util_testsandlibgit2_tests -xonlinepass. The util suite and thestatus::renames,refs,repo::init,iteratorandthreadssuites give identical results with and without the patch, on main and on v1.9.7.-DENABLE_WERROR=ON, with-DUSE_THREADS=OFF, with both together, and with-DUSE_I18N=OFF.