Close Keep-Alive connections following a parser error (#232)

Sources/KituraNet/HTTP/IncomingHTTPSocketProcessor.swift

@@ -49,11 +49,14 @@ public class IncomingHTTPSocketProcessor: IncomingSocketProcessor {
  ///HTTP Parser
  private let httpParser: HTTPParser
 
+ /// Indicates whether the HTTP parser has encountered a parsing error
+ private var parserErrored = false
+
  /// Controls the number of requests that may be sent on this connection.
  private(set) var keepAliveState: KeepAliveState
 
  /// Should this socket actually be kept alive?
- var isKeepAlive: Bool { return clientRequestedKeepAlive && keepAliveState.keepAlive() }
+ var isKeepAlive: Bool { return clientRequestedKeepAlive && keepAliveState.keepAlive() && !parserErrored }
 
  let socket: Socket
 
@@ -199,6 +202,9 @@ public class IncomingHTTPSocketProcessor: IncomingSocketProcessor {
  Log.error("Failed to parse a request. \(parsingStatus.error!)")
  let response = HTTPServerResponse(processor: self, request: nil)
  response.statusCode = .badRequest
+ // We must avoid any further attempts to process data from this client
+ // after a parser error has occurred. (see Kitura-net#228)
+ parserErrored = true
  do {
  try response.end()
  }

Sources/KituraNet/IncomingSocketHandler.swift

@@ -192,8 +192,10 @@ public class IncomingSocketHandler {
  public func handleBufferedReadData() {
  #if os(OSX) || os(iOS) || os(tvOS) || os(watchOS) || GCD_ASYNCH
  if socket.socketfd != Socket.SOCKET_INVALID_DESCRIPTOR {
- socketReaderQueue.sync() { [unowned self] in
- _ = self.handleBufferedReadDataHelper()
+ socketReaderQueue.sync() { [weak self] in
+ if let strongSelf = self {
+ _ = strongSelf.handleBufferedReadDataHelper()
+ }
  }
  }
  #endif

Tests/KituraNetTests/ClientE2ETests.swift

@@ -134,17 +134,13 @@ class ClientE2ETests: KituraNetTest {
 
  private func doPipelineTest(expecting expectedResponse: String, totalRequests: Int, writer: (Socket) throws -> Void) {
  do {
- let server: HTTPServer = try startServer(TestServerDelegate(), port: 0, useSSL: false)
+ let server: HTTPServer
+ let serverPort: Int
+ (server, serverPort) = try startEphemeralServer(ClientE2ETests.TestServerDelegate(), useSSL: false)
  defer {
  server.stop()
  }
 
- guard let serverPort = server.port else {
- XCTFail("Server port was not initialized")
- return
- }
- XCTAssertTrue(serverPort != 0, "Ephemeral server port not set")
-
  // Send pipelined requests to the server
  let clientSocket = try Socket.create()
  try clientSocket.connect(to: "localhost", port: Int32(serverPort))

Tests/KituraNetTests/KituraNetTest.swift

@@ -22,6 +22,10 @@ import Foundation
 import Dispatch
 import SSLService
 
+struct KituraNetTestError: Swift.Error {
+ let message: String
+}
+
 class KituraNetTest: XCTestCase {
 
  static let useSSLDefault = true
@@ -69,6 +73,19 @@ class KituraNetTest: XCTestCase {
  return server
  }
 
+ /// Convenience function for starting an HTTPServer on an ephemeral port,
+ /// returning the a tuple containing the server and the port it is listening on.
+ func startEphemeralServer(_ delegate: ServerDelegate?, useSSL: Bool = useSSLDefault, allowPortReuse: Bool = portReuseDefault) throws -> (server: HTTPServer, port: Int) {
+ let server = try startServer(delegate, port: 0, useSSL: useSSL, allowPortReuse: allowPortReuse)
+ guard let serverPort = server.port else {
+ throw KituraNetTestError(message: "Server port was not initialized")
+ }
+ guard serverPort != 0 else {
+ throw KituraNetTestError(message: "Ephemeral server port not set (was zero)")
+ }
+ return (server, serverPort)
+ }
+
  func performServerTest(_ delegate: ServerDelegate?, port: Int = portDefault, useSSL: Bool = useSSLDefault, allowPortReuse: Bool = portReuseDefault,
  line: Int = #line, asyncTasks: (XCTestExpectation) -> Void...) {
 

Tests/KituraNetTests/RegressionTests.swift

@@ -28,6 +28,10 @@ class RegressionTests: KituraNetTest {
  static var allTests : [(String, (RegressionTests) -> () throws -> Void)] {
  return [
  ("testIssue1143", testIssue1143),
+ ("testServersCollidingOnPort", testServersCollidingOnPort),
+ ("testServersSharingPort", testServersSharingPort),
+ ("testBadRequest", testBadRequest),
+ ("testBadRequestFollowingGoodRequest", testBadRequestFollowingGoodRequest),
  ]
  }
 
@@ -59,17 +63,13 @@ class RegressionTests: KituraNetTest {
  /// connections.
  func testIssue1143() {
  do {
- let server: HTTPServer = try startServer(nil, port: 0, useSSL: true)
+ let server: HTTPServer
+ let serverPort: Int
+ (server, serverPort) = try startEphemeralServer(ClientE2ETests.TestServerDelegate(), useSSL: true)
  defer {
  server.stop()
  }
 
- guard let serverPort = server.port else {
- XCTFail("Server port was not initialized")
- return
- }
- XCTAssertTrue(serverPort != 0, "Ephemeral server port not set")
-
  // Queue a server stop operation in 1 second, in case the test hangs (socket listener blocks)
  let recoveryOperation = DispatchWorkItem {
  server.stop()
@@ -152,17 +152,13 @@ class RegressionTests: KituraNetTest {
  /// Tests that attempting to start a second HTTPServer on the same port fails.
  func testServersCollidingOnPort() {
  do {
- let server: HTTPServer = try startServer(nil, port: 0, useSSL: false)
+ let server: HTTPServer
+ let serverPort: Int
+ (server, serverPort) = try startEphemeralServer(ClientE2ETests.TestServerDelegate(), useSSL: false)
  defer {
  server.stop()
  }
 
- guard let serverPort = server.port else {
- XCTFail("Server port was not initialized")
- return
- }
- XCTAssertTrue(serverPort != 0, "Ephemeral server port not set")
-
  do {
  let collidingServer: HTTPServer = try startServer(nil, port: serverPort, useSSL: false)
  defer {
@@ -206,5 +202,163 @@ class RegressionTests: KituraNetTest {
  XCTFail("Error: \(error)")
  }
  }
+
+ /// Tests that sending a bad request results in a `400/Bad Request` response
+ /// from the server with `Connection: Close` header.
+ func testBadRequest() {
+ let requestBuffer = "GET / HTTP/1.1\r\nFOO\r\n\r\n"
+ do {
+ let server: HTTPServer
+ let serverPort: Int
+ (server, serverPort) = try startEphemeralServer(ClientE2ETests.TestServerDelegate(), useSSL: false)
+ defer {
+ server.stop()
+ }
+
+ // Send request to the server
+ let clientSocket = try Socket.create()
+ try clientSocket.connect(to: "localhost", port: Int32(serverPort))
+ defer {
+ clientSocket.close()
+ }
+ try clientSocket.write(from: requestBuffer)
+
+ // Queue a recovery task to close our socket so that the test cannot wait forever
+ // waiting for responses from the server
+ let recoveryTask = DispatchWorkItem {
+ XCTFail("Timed out waiting for responses from server")
+ clientSocket.close()
+ }
+ let timeout = DispatchTime.now() + .seconds(1)
+ DispatchQueue.global().asyncAfter(deadline: timeout, execute: recoveryTask)
+
+ // Read responses from the server
+ let buffer = NSMutableData(capacity: 2000)!
+ var read = 0
+ var bufferPosition = 0
+ let response = ClientResponse()
+ while true {
+ XCTAssert(read == buffer.length, "Bytes read does not equal buffer length")
+ let status = response.parse(buffer, from: bufferPosition)
+ bufferPosition = buffer.length - status.bytesLeft
+ if status.state == .messageComplete {
+ break
+ }
+ read += try clientSocket.read(into: buffer)
+ }
+ // Check that the response indicates failure
+ validate400Response(response: response, responseNumber: 1)
+ // We completed reading the responses, cancel the recovery task
+ recoveryTask.cancel()
+ XCTAssert(bufferPosition == buffer.length, "Unparsed bytes remaining after final response")
+
+ } catch {
+ XCTFail("Error: \(error)")
+ }
+ }
+
+ /// Tests that sending a good request followed by garbage on a Keep-Alive
+ /// connection results in a `200/OK` response, followed by a `400/Bad Request`
+ /// response with `Connection: Close` header.
+ /// This is to verify the fix introduced in Kitura-net PR #229, where a malformed
+ /// request sent during a Keep-Alive session could cause the server to crash.
+ func testBadRequestFollowingGoodRequest() {
+ let requestBuffer = "GET / HTTP/1.1\r\n\r\nFOO\r\n"
+ let totalRequests = 2
+ do {
+ let server: HTTPServer
+ let serverPort: Int
+ (server, serverPort) = try startEphemeralServer(ClientE2ETests.TestServerDelegate(), useSSL: false)
+ defer {
+ server.stop()
+ }
+
+ // Send requests to the server
+ let clientSocket = try Socket.create()
+ try clientSocket.connect(to: "localhost", port: Int32(serverPort))
+ defer {
+ clientSocket.close()
+ }
+ try clientSocket.write(from: requestBuffer)
+
+ // Queue a recovery task to close our socket so that the test cannot wait forever
+ // waiting for responses from the server
+ let recoveryTask = DispatchWorkItem {
+ XCTFail("Timed out waiting for responses from server")
+ clientSocket.close()
+ }
+ let timeout = DispatchTime.now() + .seconds(1)
+ DispatchQueue.global().asyncAfter(deadline: timeout, execute: recoveryTask)
+
+ // Read responses from the server
+ let buffer = NSMutableData(capacity: 2000)!
+ var read = 0
+ var bufferPosition = 0
+ var responsesToRead = totalRequests
+ while responsesToRead > 0 {
+ responsesToRead -= 1
+ let response = ClientResponse()
+ while true {
+ XCTAssert(read == buffer.length, "Bytes read does not equal buffer length")
+ let status = response.parse(buffer, from: bufferPosition)
+ bufferPosition = buffer.length - status.bytesLeft
+ if status.state == .messageComplete {
+ break
+ }
+ read += try clientSocket.read(into: buffer)
+ }
+ let responseNumber = totalRequests - responsesToRead
+ switch responseNumber {
+ case 1:
+ validate200Response(response: response, responseNumber: responseNumber)
+ case 2:
+ validate400Response(response: response, responseNumber: responseNumber)
+ default:
+ XCTFail("Unexpected responseNumber \(responseNumber)")
+ }
+ // Check that the response indicates success
+
+ }
+ // We completed reading the responses, cancel the recovery task
+ recoveryTask.cancel()
+ XCTAssert(bufferPosition == buffer.length, "Unparsed bytes remaining after final response")
+
+ } catch {
+ XCTFail("Error: \(error)")
+ }
+ }
+
+ /// Checks that the provided ClientResponse represents an HTTP `200 OK`
+ /// response from the server with an appropriate `Connection: Keep-Alive`
+ /// header.
+ private func validate200Response(response: ClientResponse, responseNumber: Int) {
+ XCTAssert(response.httpStatusCode == .OK, "Response \(responseNumber) was not 200/OK, was \(response.httpStatusCode)")
+ guard let connectionHeader = response.headers["Connection"] else {
+ XCTFail("Response did not contain a 'Connection' header")
+ return
+ }
+ guard connectionHeader.count == 1 else {
+ XCTFail("Connection header did not have a single value: \(connectionHeader)")
+ return
+ }
+ let connectionValue = connectionHeader[0]
+ XCTAssert(connectionValue == "Keep-Alive", "Response 'Connection' header should be 'Keep-Alive', but was \(connectionValue)")
+ }
+
+ /// Checks that the provided ClientResponse represents an HTTP `400 Bad Request`
+ /// response from the server with an appropriate `Connection: Close` header.
+ private func validate400Response(response: ClientResponse, responseNumber: Int) {
+ XCTAssert(response.httpStatusCode == .badRequest, "Response was not 400/Bad Request, was \(response.httpStatusCode)")
+ guard let connectionHeader = response.headers["Connection"] else {
+ XCTFail("Response did not contain a 'Connection' header")
+ return
+ }
+ guard connectionHeader.count == 1 else {
+ XCTFail("Connection header did not have a single value: \(connectionHeader)")
+ return
+ }
+ let connectionValue = connectionHeader[0]
+ XCTAssert(connectionValue == "Close", "Response 'Connection' header should be 'Close', but was \(connectionValue)")
+ }
 
 }