Merge pull request #2098 from ahoppen/swift-format-crashing

Don’t crash SourceKit-LSP if swift-format crashes while we write the source file to stdin

Author: ahoppen

Sources/LanguageServerProtocolJSONRPC/DisableSigpipe.swift

@@ -25,12 +25,23 @@ private let globallyIgnoredSIGPIPE: Bool = {
   _ = signal(SIGPIPE, SIG_IGN)
   return true
 }()
+#endif
 
-internal func globallyDisableSigpipe() {
+/// We receive a `SIGPIPE` if we write to a pipe that points to a crashed process. This in particular happens if the
+/// target of a `JSONRPCConnection` has crashed and we try to send it a message or if swift-format crashes and we try
+/// to send the source file to it.
+///
+/// On Darwin, `DispatchIO` ignores `SIGPIPE` for the pipes handled by it and swift-tools-support-core offers
+/// `LocalFileOutputByteStream.disableSigpipe`, but that features is not available on Linux.
+///
+/// Instead, globally ignore `SIGPIPE` on Linux to prevent us from crashing if the `JSONRPCConnection`'s target crashes.
+///
+/// On Darwin platforms and on Windows this is a no-op.
+package func globallyDisableSigpipeIfNeeded() {
+  #if !canImport(Darwin) && !os(Windows)
   let haveWeIgnoredSIGPIEThisIsHereToTriggerIgnoringIt = globallyIgnoredSIGPIPE
   guard haveWeIgnoredSIGPIEThisIsHereToTriggerIgnoringIt else {
     fatalError("globallyIgnoredSIGPIPE should always be true")
   }
+  #endif
 }
-
-#endif

Sources/LanguageServerProtocolJSONRPC/JSONRPCConnection.swift

@@ -124,13 +124,7 @@ public final class JSONRPCConnection: Connection {
     self.inputMirrorFile = inputMirrorFile
     self.outputMirrorFile = outputMirrorFile
     self.receiveHandler = nil
-    #if os(Linux) || os(Android)
-    // We receive a `SIGPIPE` if we write to a pipe that points to a crashed process. This in particular happens if the
-    // target of a `JSONRPCConnection` has crashed and we try to send it a message.
-    // On Darwin, `DispatchIO` ignores `SIGPIPE` for the pipes handled by it, but that features is not available on Linux.
-    // Instead, globally ignore `SIGPIPE` on Linux to prevent us from crashing if the `JSONRPCConnection`'s target crashes.
-    globallyDisableSigpipe()
-    #endif
+    globallyDisableSigpipeIfNeeded()
     state = .created
     self.messageRegistry = messageRegistry
 

Sources/SourceKitLSP/Swift/DocumentFormatting.swift

@@ -13,6 +13,7 @@
 import Foundation
 package import LanguageServerProtocol
 import LanguageServerProtocolExtensions
+import LanguageServerProtocolJSONRPC
 import SKLogging
 import SKUtilities
 import SwiftExtensions
@@ -21,7 +22,9 @@ import SwiftSyntax
 import TSCExtensions
 
 import struct TSCBasic.AbsolutePath
+import class TSCBasic.LocalFileOutputByteStream
 import class TSCBasic.Process
+import protocol TSCBasic.WritableByteStream
 
 fileprivate extension String {
   init?(bytes: [UInt8], encoding: Encoding) {
@@ -190,11 +193,44 @@ extension SwiftLanguageService {
       ]
     }
     let process = TSCBasic.Process(arguments: args)
-    let writeStream = try process.launch()
+    let writeStream: any WritableByteStream
+    do {
+      writeStream = try process.launch()
+    } catch {
+      throw ResponseError.unknown("Launching swift-format failed: \(error)")
+    }
+    #if canImport(Darwin)
+    // On Darwin, we can disable SIGPIPE for a single pipe. This is not available on all platforms, in which case we
+    // resort to disabling SIGPIPE globally to avoid crashing SourceKit-LSP with SIGPIPE if swift-format crashes before
+    // we could send all data to its stdin.
+    if let byteStream = writeStream as? LocalFileOutputByteStream {
+      orLog("Disable SIGPIPE for swift-format stdin") {
+        try byteStream.disableSigpipe()
+      }
+    } else {
+      logger.fault("Expected write stream to process to be a LocalFileOutputByteStream")
+    }
+    #else
+    globallyDisableSigpipeIfNeeded()
+    #endif
 
-    // Send the file to format to swift-format's stdin. That way we don't have to write it to a file.
-    writeStream.send(snapshot.text)
-    try writeStream.close()
+    do {
+      // Send the file to format to swift-format's stdin. That way we don't have to write it to a file.
+      //
+      // If we are on Windows, `writeStream` is not a swift-tools-support-core type but a `FileHandle`. In that case,
+      // call the throwing `write(contentsOf:)` method on it so that we can catch a `ERROR_BROKEN_PIPE` error. The
+      // `send` method that we use on all other platforms ends up calling the non-throwing `FileHandle.write(_:)`, which
+      // calls `write(contentsOf:)` using `try!` and thus crashes SourceKit-LSP if the pipe to swift-format is closed,
+      // eg. because swift-format has crashed.
+      if let fileHandle = writeStream as? FileHandle {
+        try fileHandle.write(contentsOf: Data(snapshot.text.utf8))
+      } else {
+        writeStream.send(snapshot.text.utf8)
+      }
+      try writeStream.close()
+    } catch {
+      throw ResponseError.unknown("Writing to swift-format stdin failed: \(error)")
+    }
 
     let result = try await withTimeout(.seconds(60)) {
       try await process.waitUntilExitStoppingProcessOnTaskCancellation()

Tests/SourceKitLSPTests/FormattingTests.swift

@@ -14,8 +14,12 @@ import LanguageServerProtocol
 import SKLogging
 import SKTestSupport
 import SourceKitLSP
+import SwiftExtensions
+import ToolchainRegistry
 import XCTest
 
+import class TSCBasic.Process
+
 final class FormattingTests: XCTestCase {
   func testFormatting() async throws {
     try await SkipUnless.swiftFormatSupportsDashToIndicateReadingFromStdin()
@@ -306,4 +310,57 @@ final class FormattingTests: XCTestCase {
       ]
     )
   }
+
+  func testSwiftFormatCrashing() async throws {
+    try await withTestScratchDir { scratchDir in
+      let toolchain = try await unwrap(ToolchainRegistry.forTesting.default)
+
+      let crashingSwiftFilePath = scratchDir.appendingPathComponent("crashing-executable.swift")
+      let crashingExecutablePath = scratchDir.appendingPathComponent("crashing-executable")
+      try await "fatalError()".writeWithRetry(to: crashingSwiftFilePath)
+      var compilerArguments = try [
+        crashingSwiftFilePath.filePath,
+        "-o",
+        crashingExecutablePath.filePath,
+      ]
+      if let defaultSDKPath {
+        compilerArguments += ["-sdk", defaultSDKPath]
+      }
+      try await Process.checkNonZeroExit(arguments: [XCTUnwrap(toolchain.swiftc?.filePath)] + compilerArguments)
+
+      let toolchainRegistry = ToolchainRegistry(toolchains: [
+        Toolchain(
+          identifier: "\(toolchain.identifier)-crashing-swift-format",
+          displayName: "\(toolchain.identifier) with crashing swift-format",
+          path: toolchain.path,
+          clang: toolchain.clang,
+          swift: toolchain.swift,
+          swiftc: toolchain.swiftc,
+          swiftFormat: crashingExecutablePath,
+          clangd: toolchain.clangd,
+          sourcekitd: toolchain.sourcekitd,
+          sourceKitClientPlugin: toolchain.sourceKitClientPlugin,
+          sourceKitServicePlugin: toolchain.sourceKitServicePlugin,
+          libIndexStore: toolchain.libIndexStore
+        )
+      ])
+      let testClient = try await TestSourceKitLSPClient(toolchainRegistry: toolchainRegistry)
+      let uri = DocumentURI(for: .swift)
+      testClient.openDocument(
+        // Generate a large source file to increase the chance of the executable we substitute for swift-format
+        // crashing before the entire input file is sent to it.
+        String(repeating: "func foo() {}\n", count: 10_000),
+        uri: uri
+      )
+      await assertThrowsError(
+        try await testClient.send(
+          DocumentFormattingRequest(
+            textDocument: TextDocumentIdentifier(uri),
+            options: FormattingOptions(tabSize: 2, insertSpaces: true)
+          )
+        ),
+        expectedMessage: #/Running swift-format failed|Writing to swift-format stdin failed/#
+      )
+    }
+  }
 }