From f4e6078d3d6906002a8dd2b6a6bea9a5b02b6949 Mon Sep 17 00:00:00 2001 From: Yee Cheng Chin Date: Sat, 9 Sep 2023 01:48:33 -0700 Subject: [PATCH] Fix Vim crashing when querying serverlist when quitting 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 #1423 --- src/MacVim/MMBackend.m | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/MacVim/MMBackend.m b/src/MacVim/MMBackend.m index 7bec3d3cb9..cce7d8ca83 100644 --- a/src/MacVim/MMBackend.m +++ b/src/MacVim/MMBackend.m @@ -273,6 +273,7 @@ - (void)dealloc [outputQueue release]; outputQueue = nil; [drawData release]; drawData = nil; [connection release]; connection = nil; + [appProxy release]; appProxy = nil; [actionDict release]; actionDict = nil; [sysColorDict release]; sysColorDict = nil; [colorDict release]; colorDict = nil; @@ -1746,11 +1747,11 @@ - (NSArray *)serverList { NSArray *list = nil; - if ([self connection]) { - id proxy = [connection rootProxy]; - [proxy setProtocolForProxy:@protocol(MMAppProtocol)]; - + if ([self connection] && [connection isValid]) { @try { + id proxy = [connection rootProxy]; + [proxy setProtocolForProxy:@protocol(MMAppProtocol)]; + list = [proxy serverList]; } @catch (NSException *ex) { @@ -2535,6 +2536,14 @@ - (void)connectionDidDie:(NSNotification * UNUSED)notification ASLogNotice(@"Main connection was lost before process had a chance " "to terminate; preserving swap files."); + + // Just release the connection, in case some autocmd's end up triggering + // IPC calls during shutdown. If other code use isValid checks this is a + // little unnecessary, but it just helps prevent issues with code that use + // the connection or proxy without checking for validity first. + [connection release]; connection = nil; + [appProxy release]; appProxy = nil; + getout_preserve_modified(1); }