[4.1][sourcekitd] Fix crash in onDocumentUpdateNotification #14353
Conversation
nathawes
changed the title
|
@swift-ci please test |
| // Update the main connection | ||
| if (MainConnection) | ||
| xpc_release(MainConnection); | ||
| MainConnection = xpc_connection_t(xpc_retain(peer)); |
There was a problem hiding this comment.
The generally safe retain/release pattern, in case the 2 objects (new and old) happen to be the same, is to retain the new one before releasing the old one:
temp_object = retain(new)
release(old)
old = temp_object
There was a problem hiding this comment.
Ok, I'll fix it up shortly!
| @@ -208,8 +208,15 @@ std::string sourcekitd::getRuntimeLibPath() { | |||
| return Path; | |||
| } | |||
|
|
|||
| static void releaseIfMainConnection(xpc_connection_t peer) { | |||
There was a problem hiding this comment.
Is this necessary ? Seems risky to set the global main connection to null. What is the benefit ?
There was a problem hiding this comment.
Nathan explained the reasoning for this.
nathawes
force-pushed
the
rdar36077481-crash-in-on-document-update-notification-4.1
branch
from
9dabcb5 to
fa15967
Compare
|
@swift-ci please test |
…g when cancelled from the other end If a client crashes and we tried to post notifications to the XPC connection, SourceKitService would go down too, as it never `xpc_retain`ed the connection. Resolves rdar://problem/36077481
|
@swift-ci please test |
|
Build failed |
|
Build failed |
nathawes
deleted the
rdar36077481-crash-in-on-document-update-notification-4.1
branch
CCC Info
Radar: rdar://problem/36077481
Explanation:
SourceKit didn't
xpc_retaina globally stored XPC connection reference,MainConnection, allowing it to be used in an invalid state. This happened if the client cancelled the connection or crashed while SourceKit was busy gathering semantic information for a document. SourceKit would use the invalid connection to try to notify the client when it finished and crash.Risk: Low. This is simply adding a retain call.
Scope of issue: One of the top SourceKit crashes
Origination: It looks like this issue has always existed.
Reviewed by: Argyrios Kyrtzidis
Testing: Cancelled the connection from the client with an outstanding semantic operation before and after the changes to verify the fix. Manually tested changes against Xcode 9.2GM and 9.3 beta 1 with basic editing, completions, navigation etc. in new projects.
PR for swift-4.1-branch: #14353
PR for master: #14355
PR for swift-5.0-branch: #14354