Fix Vim crashing when querying serverlist when quitting #1427
Conversation
This file contains 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
Currently, when quitting MacVim using Cmd-Q, if an autocmd queries serverlist() during shutdown (e.g. VimLeavePre), there's a potential that Vim will crash and then stuck in a spinloop and never gets killed by the parent process. The reason is somehwat complicated. MMAppController tells Vim to quit but has a hard-coded timer before terminating the connection. If Vim takes too long to respond somehow, it will see a "connectionDidDie" message where it will be forced to quit. The serverlist() IPC API call isn't properly guarding against an invalid connection and if an autocmd triggers that call during this time, it will throw an exception and crash. Usually if Vim crashes, it should terminate cleanly, but couple things cause this to not work properly: - Vim's signal handler `deathtrap` tries to exit cleanly when a signal is detected, and it tries to call all deferred functions (added by :defer in Vimscript). The list of functions are allocated on the stack rather than the heap. - The ObjC exception is thrown inside a CFRunLoop (which is what called connectionDidDie) and CFRunLoop silently handles the exception before re-throwing it which triggers the actual abort signal to be caught by Vim's signal handler, but at this time, the deferred functions data structure is messed up as the stack is already gone since the first exception unwound it. This leads to a bogus memory state and lead to an infinite loop in `invoke_all_defer`. MacVim also doesn't have a solid mechanism to shut down zombie processes right now (it depends on Vim cleaning up after itself), so after MacVim quits, the now-orphaned Vim process stuck consuming 100% CPU. The fix is to simply guard against this and make sure we clean up the connection properly when we detected it died, and to be more defensive and make sure the serverlist call properly guard against invalid states and exceptions. Not tackling the other issues for now. There are some unfortunate interactions here with an unwound exception causing invoke_all_defer() to not work, but we just need to make sure to guard potential places with try/catch blocks, as invoke_all_defer() is still useful. Also, proper zombie process killing will be done at a later time, as we will soon tackle removing Distributed Objects/NSConnection and revamp the entire IPC stack anyway. Fix macvim-dev#1423
ychin
force-pushed
the
fix-nsconnection-shutdown-issues
branch
from
dee254e
to
f4e6078
Compare
ychin
added a commit
that referenced
this pull request
Sep 12, 2023
Updated to Vim 9.0.1897 Special Notes ==================== As some of you may have read, Bram Moolenaar, the creator of Vim, has [passed away](https://groups.google.com/g/vim_announce/c/tWahca9zkt4) recently. He has worked tirelessly on Vim for more than 30 years and this release is dedicated to him. If you would like, you could pay your respects at [this discussion thread](vim/vim#12737). The Vim project has transitioned to new maintainers, and MacVim will continue to be supported as long as Vim is around. Features ==================== More flexible Python integration -------------------- MacVim now allows you to use Python runtime (via `pythonthreedll`, used for Python plugins) of any version at or above 3.9. Previously you had to use the exact same version that was used to build MacVim (Python 3.11). The Python detection logic is also updated to always just find the latest version of Homebrew Python instead of a fixed one, and it will also now locate the default macOS / Xcode Python provided by the Xcode Command Line Tools if that is the only Python available. This should hopefully make configuring Python for MacVim a lot more seamless. See `:h python3-stable-abi`. Vim v9.0.1776 / #1428. New Vim features -------------------- - New built-in support for [EditorConfig](https://editorconfig.org/) via an optional package. Use `packadd editorconfig` to activate it. See vim/vim#12902. - `g<End>` now goes to the first non-blank char. v9.0.1753 - API changes - `undotree()` now takes a bufnr v9.0.1686 - `printf()` now takes positional arguments v9.0.1704 - `virtcol()` now takes winid v9.0.1728 - quickfix items can now have user data v9.0.1688 - Miscellaneous security fixes. Security Fixes ==================== - Fixed insecure usages of interprocess communication in MacVim (CVE-2023-41036) Fixes ==================== - Fixed MacVim to correctly set up the runtime folder in the app bundle. As a corollary, `xxd` is now bundled with MacVim like most other Vim distributions, and MacVim.app now provides man page for the CLI vim commands if the user wants to associate man pages with the `mvim` comamnd (see `:h macvim-PATH`). #1430 - Fixed Vim occasionally crashing and/or hung when autocmd calls `serverlist()` on exit. #1427 Scripting ==================== - Scripting languages versions: - Python now supports 3.9 or above. Compatibility ==================== Requires macOS 10.9 or above. (10.9 - 10.12 requires downloading a separate legacy build) Script interfaces have compatibility with these versions: - Lua 5.4 - Perl 5.30 - Python2 2.7 - Python3 3.9 or above - Ruby 3.2
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.
Currently, when quitting MacVim using Cmd-Q, if an autocmd queries serverlist() during shutdown (e.g. VimLeavePre), there's a potential that Vim will crash and then stuck in a spinloop and never gets killed by the parent process.
The reason is somehwat complicated. MMAppController tells Vim to quit but has a hard-coded timer before terminating the connection. If Vim takes too long to respond somehow, it will see a "connectionDidDie" message where it will be forced to quit. The serverlist() IPC API call isn't properly guarding against an invalid connection and if an autocmd triggers that call during this time, it will throw an exception and crash.
Usually if Vim crashes, it should terminate cleanly, but couple things cause this to not work properly:
deathtraptries to exit cleanly when a signal is detected, and it tries to call all deferred functions (added by :defer in Vimscript). The list of functions are allocated on the stack rather than the heap.invoke_all_defer.MacVim also doesn't have a solid mechanism to shut down zombie processes right now (it depends on Vim cleaning up after itself), so after MacVim quits, the now-orphaned Vim process stuck consuming 100% CPU.
The fix is to simply guard against this and make sure we clean up the connection properly when we detected it died, and to be more defensive and make sure the serverlist call properly guard against invalid states and exceptions.
Not tackling the other issues for now. There are some unfortunate interactions here with an unwound exception causing invoke_all_defer() to not work, but we just need to make sure to guard potential places with try/catch blocks, as invoke_all_defer() is still useful. Also, proper zombie process killing will be done at a later time, as we will soon tackle removing Distributed Objects/NSConnection and revamp the entire IPC stack anyway.
Fix #1423