Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SR-7608: Pipe() crashes when the underlying system call fails. #1697

Merged
merged 1 commit into from Oct 10, 2018
Merged

SR-7608: Pipe() crashes when the underlying system call fails. #1697

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
81 changes: 45 additions & 36 deletions Foundation/FileHandle.swift
View file
Open in desktop
@@ -1,10 +1,10 @@
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2014 - 2016 Apple Inc. and the Swift project authors
// Copyright (c) 2014 - 2016, 2018 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See http://swift.org/LICENSE.txt for license information
// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
// See https://swift.org/LICENSE.txt for license information
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
//

import CoreFoundation
Expand All @@ -16,9 +16,12 @@ import Glibc
#endif

open class FileHandle : NSObject, NSSecureCoding {
internal var _fd: Int32
internal var _closeOnDealloc: Bool
internal var _closed: Bool = false
private var _fd: Int32
private var _closeOnDealloc: Bool

open var fileDescriptor: Int32 {
return _fd
}

open var readabilityHandler: ((FileHandle) -> Void)? = {
(FileHandle) -> Void in NSUnimplemented()
Expand All @@ -40,10 +43,11 @@ open class FileHandle : NSObject, NSSecureCoding {
}

internal func _readDataOfLength(_ length: Int, untilEOF: Bool) -> Data {
precondition(_fd >= 0, "Bad file descriptor")
var statbuf = stat()
var dynamicBuffer: UnsafeMutableRawPointer? = nil
var total = 0
if _closed || fstat(_fd, &statbuf) < 0 {
if fstat(_fd, &statbuf) < 0 {
fatalError("Unable to read file")
}
if statbuf.st_mode & S_IFMT != S_IFREG {
Expand Down Expand Up @@ -126,6 +130,7 @@ open class FileHandle : NSObject, NSSecureCoding {
}

open func write(_ data: Data) {
guard _fd >= 0 else { return }
data.enumerateBytes() { (bytes, range, stop) in
do {
try NSData.write(toFileDescriptor: self._fd, path: nil, buf: UnsafeRawPointer(bytes.baseAddress!), length: bytes.count)
Expand All @@ -138,39 +143,48 @@ open class FileHandle : NSObject, NSSecureCoding {
// TODO: Error handling.

open var offsetInFile: UInt64 {
precondition(_fd >= 0, "Bad file descriptor")
return UInt64(lseek(_fd, 0, SEEK_CUR))
}

@discardableResult
open func seekToEndOfFile() -> UInt64 {
precondition(_fd >= 0, "Bad file descriptor")
return UInt64(lseek(_fd, 0, SEEK_END))
}

open func seek(toFileOffset offset: UInt64) {
precondition(_fd >= 0, "Bad file descriptor")
lseek(_fd, off_t(offset), SEEK_SET)
}

open func truncateFile(atOffset offset: UInt64) {
precondition(_fd >= 0, "Bad file descriptor")
if lseek(_fd, off_t(offset), SEEK_SET) < 0 { fatalError("lseek() failed.") }
if ftruncate(_fd, off_t(offset)) < 0 { fatalError("ftruncate() failed.") }
}

open func synchronizeFile() {
precondition(_fd >= 0, "Bad file descriptor")
fsync(_fd)
}

open func closeFile() {
if !_closed {
if _fd >= 0 {
close(_fd)
_closed = true
_fd = -1
}
}

public init(fileDescriptor fd: Int32, closeOnDealloc closeopt: Bool) {
_fd = fd
_closeOnDealloc = closeopt
}


public convenience init(fileDescriptor fd: Int32) {
self.init(fileDescriptor: fd, closeOnDealloc: false)
}

internal init?(path: String, flags: Int32, createMode: Int) {
_fd = _CFOpenFileWithMode(path, flags, mode_t(createMode))
_closeOnDealloc = true
Expand All @@ -181,8 +195,9 @@ open class FileHandle : NSObject, NSSecureCoding {
}

deinit {
if _fd >= 0 && _closeOnDealloc && !_closed {
if _fd >= 0 && _closeOnDealloc {
close(_fd)
_fd = -1
}
}

Expand Down Expand Up @@ -355,38 +370,32 @@ extension FileHandle {
}
}

extension FileHandle {
public convenience init(fileDescriptor fd: Int32) {
self.init(fileDescriptor: fd, closeOnDealloc: false)
}

open var fileDescriptor: Int32 {
return _fd
}
}

open class Pipe: NSObject {
open let fileHandleForReading: FileHandle
open let fileHandleForWriting: FileHandle
public let fileHandleForReading: FileHandle
public let fileHandleForWriting: FileHandle

public override init() {
/// the `pipe` system call creates two `fd` in a malloc'ed area
var fds = UnsafeMutablePointer<Int32>.allocate(capacity: 2)
defer {
free(fds)
fds.deallocate()
}
/// If the operating system prevents us from creating file handles, stop
guard pipe(fds) == 0 else { fatalError("Could not open pipe file handles") }

/// The handles below auto-close when the `NSFileHandle` is deallocated, so we
/// don't need to add a `deinit` to this class

/// Create the read handle from the first fd in `fds`
self.fileHandleForReading = FileHandle(fileDescriptor: fds.pointee, closeOnDealloc: true)

/// Advance `fds` by one to create the write handle from the second fd
self.fileHandleForWriting = FileHandle(fileDescriptor: fds.successor().pointee, closeOnDealloc: true)

let ret = pipe(fds)
switch (ret, errno) {
case (0, _):
self.fileHandleForReading = FileHandle(fileDescriptor: fds.pointee, closeOnDealloc: true)
self.fileHandleForWriting = FileHandle(fileDescriptor: fds.successor().pointee, closeOnDealloc: true)

case (-1, EMFILE), (-1, ENFILE):
// Unfortunately this initializer does not throw and isnt failable so this is only
// way of handling this situation.
self.fileHandleForReading = FileHandle(fileDescriptor: -1, closeOnDealloc: false)
self.fileHandleForWriting = FileHandle(fileDescriptor: -1, closeOnDealloc: false)

default:
fatalError("Error calling pipe(): \(errno)")
}
super.init()
}
}
24 changes: 3 additions & 21 deletions TestFoundation/TestFileHandle.swift
View file
Open in desktop
@@ -1,17 +1,16 @@
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2016 Apple Inc. and the Swift project authors
// Copyright (c) 2016, 2018 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See http://swift.org/LICENSE.txt for license information
// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
// See https://swift.org/LICENSE.txt for license information
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
//

class TestFileHandle : XCTestCase {
static var allTests : [(String, (TestFileHandle) -> () throws -> ())] {
return [
("test_constants", test_constants),
("test_pipe", test_pipe),
("test_nullDevice", test_nullDevice),
("test_truncateFile", test_truncateFile)
]
Expand All @@ -22,23 +21,6 @@ class TestFileHandle : XCTestCase {
"\(FileHandle.readCompletionNotification.rawValue) is not equal to NSFileHandleReadCompletionNotification")
}

func test_pipe() {
let pipe = Pipe()
let inputs = ["Hello", "world", "🐶"]

for input in inputs {
let inputData = input.data(using: .utf8)!

// write onto pipe
pipe.fileHandleForWriting.write(inputData)

let outputData = pipe.fileHandleForReading.availableData
let output = String(data: outputData, encoding: .utf8)

XCTAssertEqual(output, input)
}
}

func test_nullDevice() {
let fh = FileHandle.nullDevice

Expand Down
34 changes: 27 additions & 7 deletions TestFoundation/TestPipe.swift
View file
Open in desktop
@@ -1,21 +1,41 @@
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2014 - 2016 Apple Inc. and the Swift project authors
// Copyright (c) 2014 - 2016. 2018 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See http://swift.org/LICENSE.txt for license information
// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
// See https://swift.org/LICENSE.txt for license information
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
//

class TestPipe : XCTestCase {
class TestPipe: XCTestCase {

static var allTests: [(String, (TestPipe) -> () throws -> Void)] {
return [
("test_NSPipe", test_NSPipe)
("test_MaxPipes", test_MaxPipes),
("test_Pipe", test_Pipe),
]
}

func test_NSPipe() {

func test_MaxPipes() {
// Try and create enough pipes to exhaust the process's limits. 1024 is a reasonable
// hard limit for the test. This is reached when testing on Linux (at around 488 pipes)
// but not on macOS.

var pipes: [Pipe] = []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried about this, because it means that the test process cannot use pipes meaningfully anywhere else.

Can we move this code to a separate process we spawn?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the pipes should be closed when the array is cleared. test_MaxPipes is run before test_Pipe to test this very case.

let maxPipes = 1024
pipes.reserveCapacity(maxPipes)
for _ in 1...maxPipes {
let pipe = Pipe()
if pipe.fileHandleForReading.fileDescriptor == -1 {
XCTAssertEqual(pipe.fileHandleForReading.fileDescriptor, pipe.fileHandleForWriting.fileDescriptor)
break
}
pipes.append(pipe)
}
pipes = []
}

func test_Pipe() {
let aPipe = Pipe()
let text = "test-pipe"

Expand Down