Skip to content

Commit

Permalink
Fix Vim crashing when querying serverlist when quitting
Browse files Browse the repository at this point in the history
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
  • Loading branch information
ychin committed Sep 9, 2023
1 parent a26996d commit f4e6078
Showing 1 changed file with 13 additions and 4 deletions.
17 changes: 13 additions & 4 deletions src/MacVim/MMBackend.m
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
}

Expand Down

0 comments on commit f4e6078

Please sign in to comment.