From 6617eb0d3afcb12170594968df01ca63afb58ac5 Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Thu, 12 Jul 2018 12:26:00 +0100 Subject: [PATCH] Move syscalls to a port of Posix enum. (#24) Motivation: Right now we make a bunch of naked syscalls that don't correctly handle reporting errno or catching EINTR. We can't use the NIO Posix enum to fix this for us because it's internal, and because it doesn't have the function we need. Instead, we create a temporary port of those functions. Ideally we'd reconcile these at some time in the future. Modifications: Add a port of the Posix enum and its core functions. Improve wrapErrorIsNullReturnCall to be more useful. Adopt the posix functions. Result: Better safety in the face of EINTR. Better reporting of errnos. --- Sources/NIOOpenSSL/PosixPort.swift | 112 ++++++++++++++++++ Sources/NIOOpenSSL/SSLCertificate.swift | 4 +- Sources/NIOOpenSSL/SSLContext.swift | 4 +- Sources/NIOOpenSSL/SSLPKCS12Bundle.swift | 11 +- Sources/NIOOpenSSL/SSLPrivateKey.swift | 4 +- .../NIOOpenSSLTests/SSLCertificateTest.swift | 8 +- .../NIOOpenSSLTests/SSLPKCS12BundleTest.swift | 4 +- .../NIOOpenSSLTests/SSLPrivateKeyTests.swift | 16 +-- 8 files changed, 134 insertions(+), 29 deletions(-) create mode 100644 Sources/NIOOpenSSL/PosixPort.swift diff --git a/Sources/NIOOpenSSL/PosixPort.swift b/Sources/NIOOpenSSL/PosixPort.swift new file mode 100644 index 00000000..a181c643 --- /dev/null +++ b/Sources/NIOOpenSSL/PosixPort.swift @@ -0,0 +1,112 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the SwiftNIO open source project +// +// Copyright (c) 2017-2018 Apple Inc. and the SwiftNIO project authors +// Licensed under Apache License v2.0 +// +// See LICENSE.txt for license information +// See CONTRIBUTORS.txt for the list of SwiftNIO project authors +// +// SPDX-License-Identifier: Apache-2.0 +// +//===----------------------------------------------------------------------===// + +// This file contains a version of the SwiftNIO Posix enum. This is necessary +// because SwiftNIO's version is internal. Our version exists for the same reason: +// to ensure errno is captured correctly when doing syscalls, and that no ARC traffic +// can happen inbetween that *could* change the errno value before we were able to +// read it. +// +// The code is an exact port from SwiftNIO, so if that version ever becomes public we +// can lift anything missing from there and move it over without change. +import NIO + +private let sysFopen: @convention(c) (UnsafePointer?, UnsafePointer?) -> UnsafeMutablePointer? = fopen +private let sysMlock: @convention(c) (UnsafeRawPointer?, size_t) -> CInt = mlock +private let sysMunlock: @convention(c) (UnsafeRawPointer?, size_t) -> CInt = munlock + +// Sadly, stat has different signatures with glibc and macOS libc. +#if os(Linux) || os(FreeBSD) || os(Android) +private let sysStat: @convention(c) (UnsafePointer, UnsafeMutablePointer) -> CInt = stat(_:_:) +#elseif os(macOS) || os(iOS) || os(watchOS) || os(tvOS) +private let sysStat: @convention(c) (UnsafePointer?, UnsafeMutablePointer?) -> CInt = stat(_:_:) +#endif + + +// MARK:- Copied code from SwiftNIO +private func isBlacklistedErrno(_ code: CInt) -> Bool { + switch code { + case EFAULT, EBADF: + return true + default: + return false + } +} + +/* Sorry, we really try hard to not use underscored attributes. In this case however we seem to break the inlining threshold which makes a system call take twice the time, ie. we need this exception. */ +@inline(__always) +internal func wrapSyscall(where function: StaticString = #function, _ body: () throws -> T) throws -> T { + while true { + let res = try body() + if res == -1 { + let err = errno + if err == EINTR { + continue + } + assert(!isBlacklistedErrno(err), "blacklisted errno \(err) \(strerror(err)!)") + throw IOError(errnoCode: err, function: function) + } + return res + } +} + +/* Sorry, we really try hard to not use underscored attributes. In this case however we seem to break the inlining threshold which makes a system call take twice the time, ie. we need this exception. */ +@inline(__always) +internal func wrapErrorIsNullReturnCall(where function: StaticString = #function, _ body: () throws -> T?) throws -> T { + while true { + guard let res = try body() else { + let err = errno + if err == EINTR { + continue + } + assert(!isBlacklistedErrno(err), "blacklisted errno \(err) \(strerror(err)!)") + throw IOError(errnoCode: err, function: function) + } + return res + } +} + +// MARK:- Our functions +internal enum Posix { + @inline(never) + public static func fopen(file: UnsafePointer, mode: UnsafePointer) throws -> UnsafeMutablePointer { + return try wrapErrorIsNullReturnCall { + sysFopen(file, mode) + } + } + + @inline(never) + @discardableResult + public static func stat(path: UnsafePointer, buf: UnsafeMutablePointer) throws -> CInt { + return try wrapSyscall { + sysStat(path, buf) + } + } + + @inline(never) + @discardableResult + public static func mlock(addr: UnsafeRawPointer, len: size_t) throws -> CInt { + return try wrapSyscall { + sysMlock(addr, len) + } + } + + @inline(never) + @discardableResult + public static func munlock(addr: UnsafeRawPointer, len: size_t) throws -> CInt { + return try wrapSyscall { + sysMunlock(addr, len) + } + } +} diff --git a/Sources/NIOOpenSSL/SSLCertificate.swift b/Sources/NIOOpenSSL/SSLCertificate.swift index 2bea2288..8d0268ed 100644 --- a/Sources/NIOOpenSSL/SSLCertificate.swift +++ b/Sources/NIOOpenSSL/SSLCertificate.swift @@ -46,9 +46,7 @@ public class OpenSSLCertificate { /// /// Note that this method will only ever load the first certificate from a given file. public convenience init (file: String, format: OpenSSLSerializationFormats) throws { - guard let fileObject = fopen(file, "rb") else { - throw NIOOpenSSLError.noSuchFilesystemObject - } + let fileObject = try Posix.fopen(file: file, mode: "rb") defer { fclose(fileObject) } diff --git a/Sources/NIOOpenSSL/SSLContext.swift b/Sources/NIOOpenSSL/SSLContext.swift index 73ca3525..26a6c2ab 100644 --- a/Sources/NIOOpenSSL/SSLContext.swift +++ b/Sources/NIOOpenSSL/SSLContext.swift @@ -26,7 +26,9 @@ private enum FileSystemObject { static func pathType(path: String) -> FileSystemObject? { var statObj = stat() - guard stat(path, &statObj) == 0 else { + do { + try Posix.stat(path: path, buf: &statObj) + } catch { return nil } diff --git a/Sources/NIOOpenSSL/SSLPKCS12Bundle.swift b/Sources/NIOOpenSSL/SSLPKCS12Bundle.swift index fb852b15..39eb107a 100644 --- a/Sources/NIOOpenSSL/SSLPKCS12Bundle.swift +++ b/Sources/NIOOpenSSL/SSLPKCS12Bundle.swift @@ -116,9 +116,7 @@ public struct OpenSSLPKCS12Bundle { public init(file: String, passphrase: Bytes?) throws where Bytes.Element == UInt8 { guard openSSLIsInitialized else { fatalError("Failed to initialize OpenSSL") } - guard let fileObject = fopen(file, "rb") else { - throw NIOOpenSSLError.noSuchFilesystemObject - } + let fileObject = try Posix.fopen(file: file, mode: "rb") defer { fclose(fileObject) } @@ -179,13 +177,10 @@ internal extension Collection where Element == UInt8 { ptr.deallocate() } - if mlock(bufferPtr.baseAddress!, bufferPtr.count) != 0 { - throw IOError(errnoCode: errno, function: "mlock") - } + try Posix.mlock(addr: bufferPtr.baseAddress!, len: bufferPtr.count) defer { // If munlock fails take out the process. - let rc = munlock(bufferPtr.baseAddress!, bufferPtr.count) - precondition(rc == 0, "munlock failed with \(rc), errno \(errno)") + try! Posix.munlock(addr: bufferPtr.baseAddress!, len: bufferPtr.count) } let (_, nextIndex) = bufferPtr.initialize(from: self) diff --git a/Sources/NIOOpenSSL/SSLPrivateKey.swift b/Sources/NIOOpenSSL/SSLPrivateKey.swift index 90e438d9..f224a1a1 100644 --- a/Sources/NIOOpenSSL/SSLPrivateKey.swift +++ b/Sources/NIOOpenSSL/SSLPrivateKey.swift @@ -121,9 +121,7 @@ public class OpenSSLPrivateKey { /// A delegating initializer for `init(file:format:passphraseCallback)` and `init(file:format:)`. private convenience init(file: String, format: OpenSSLSerializationFormats, callbackManager: CallbackManagerProtocol?) throws { - guard let fileObject = fopen(file, "rb") else { - throw NIOOpenSSLError.noSuchFilesystemObject - } + let fileObject = try Posix.fopen(file: file, mode: "rb") defer { fclose(fileObject) } diff --git a/Tests/NIOOpenSSLTests/SSLCertificateTest.swift b/Tests/NIOOpenSSLTests/SSLCertificateTest.swift index f569368a..a8388c1d 100644 --- a/Tests/NIOOpenSSLTests/SSLCertificateTest.swift +++ b/Tests/NIOOpenSSLTests/SSLCertificateTest.swift @@ -251,8 +251,8 @@ class SSLCertificateTest: XCTestCase { do { _ = try OpenSSLCertificate(file: "/nonexistent/path", format: .pem) XCTFail("Did not throw") - } catch NIOOpenSSLError.noSuchFilesystemObject { - // ok + } catch let error as IOError { + XCTAssertEqual(error.errnoCode, ENOENT) } catch { XCTFail("Unexpected error: \(error)") } @@ -262,8 +262,8 @@ class SSLCertificateTest: XCTestCase { do { _ = try OpenSSLCertificate(file: "/nonexistent/path", format: .der) XCTFail("Did not throw") - } catch NIOOpenSSLError.noSuchFilesystemObject { - // ok + } catch let error as IOError { + XCTAssertEqual(error.errnoCode, ENOENT) } catch { XCTFail("Unexpected error: \(error)") } diff --git a/Tests/NIOOpenSSLTests/SSLPKCS12BundleTest.swift b/Tests/NIOOpenSSLTests/SSLPKCS12BundleTest.swift index 2ed18583..a7ffb825 100644 --- a/Tests/NIOOpenSSLTests/SSLPKCS12BundleTest.swift +++ b/Tests/NIOOpenSSLTests/SSLPKCS12BundleTest.swift @@ -498,8 +498,8 @@ class SSLPKCS12BundleTest: XCTestCase { do { _ = try OpenSSLPKCS12Bundle(file: "/nonexistent/path") XCTFail("Did not throw") - } catch NIOOpenSSLError.noSuchFilesystemObject { - // Ok + } catch let error as IOError { + XCTAssertEqual(error.errnoCode, ENOENT) } catch { XCTFail("Unexpected error: \(error)") } diff --git a/Tests/NIOOpenSSLTests/SSLPrivateKeyTests.swift b/Tests/NIOOpenSSLTests/SSLPrivateKeyTests.swift index 3c97d444..4f66be68 100644 --- a/Tests/NIOOpenSSLTests/SSLPrivateKeyTests.swift +++ b/Tests/NIOOpenSSLTests/SSLPrivateKeyTests.swift @@ -141,8 +141,8 @@ class SSLPrivateKeyTest: XCTestCase { do { _ = try OpenSSLPrivateKey(file: "/nonexistent/path", format: .pem) XCTFail("Did not throw") - } catch NIOOpenSSLError.noSuchFilesystemObject { - // ok + } catch let error as IOError { + XCTAssertEqual(error.errnoCode, ENOENT) } catch { XCTFail("Unexpected error: \(error)") } @@ -152,8 +152,8 @@ class SSLPrivateKeyTest: XCTestCase { do { _ = try OpenSSLPrivateKey(file: "/nonexistent/path", format: .der) XCTFail("Did not throw") - } catch NIOOpenSSLError.noSuchFilesystemObject { - // ok + } catch let error as IOError { + XCTAssertEqual(error.errnoCode, ENOENT) } catch { XCTFail("Unexpected error: \(error)") } @@ -165,8 +165,8 @@ class SSLPrivateKeyTest: XCTestCase { XCTFail("Should not be called") } XCTFail("Did not throw") - } catch NIOOpenSSLError.noSuchFilesystemObject { - // ok + } catch let error as IOError { + XCTAssertEqual(error.errnoCode, ENOENT) } catch { XCTFail("Unexpected error: \(error)") } @@ -178,8 +178,8 @@ class SSLPrivateKeyTest: XCTestCase { XCTFail("Should not be called") } XCTFail("Did not throw") - } catch NIOOpenSSLError.noSuchFilesystemObject { - // ok + } catch let error as IOError { + XCTAssertEqual(error.errnoCode, ENOENT) } catch { XCTFail("Unexpected error: \(error)") }