Skip to content

SR-4993: Read zero length files on Linux #1166

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.

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

Merged
merged 1 commit into from
Aug 29, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 36 additions & 1 deletion Foundation/NSData.swift
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ open class NSData : NSObject, NSCopying, NSMutableCopying, NSSecureCoding {
public convenience init(bytesNoCopy bytes: UnsafeMutableRawPointer, length: Int, deallocator: ((UnsafeMutableRawPointer, Int) -> Void)? = nil) {
self.init(bytes: bytes, length: length, copy: false, deallocator: deallocator)
}

public convenience init(contentsOfFile path: String, options readOptionsMask: ReadingOptions = []) throws {
let readResult = try NSData.readBytesFromFileWithExtendedAttributes(path, options: readOptionsMask)
self.init(bytes: readResult.bytes, length: readResult.length, copy: false, deallocator: readResult.deallocator)
Expand Down Expand Up @@ -380,7 +381,10 @@ open class NSData : NSObject, NSCopying, NSMutableCopying, NSSecureCoding {
}

let length = Int(info.st_size)

if length == 0 && (info.st_mode & S_IFMT == S_IFREG) {
return try readZeroSizeFile(fd)
}

if options.contains(.alwaysMapped) {
let data = mmap(nil, length, PROT_READ, MAP_PRIVATE, fd, 0)

Expand Down Expand Up @@ -414,6 +418,37 @@ open class NSData : NSObject, NSCopying, NSMutableCopying, NSSecureCoding {
free(buffer)
}
}

internal static func readZeroSizeFile(_ fd: Int32) throws -> NSDataReadResult {
let blockSize = 1024 * 1024 // 1MB
var data: UnsafeMutableRawPointer? = nil
var bytesRead = 0
var amt = 0

repeat {
data = realloc(data, bytesRead + blockSize)
Copy link
Contributor

Choose a reason for hiding this comment

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

This may return NULL, in which case the unwrap subsequently will fail. Is there any reason why we can't use a single byte array for reading and then have an NSMutableData to append the bytes into? That would solve the realloc calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was duplicating the usage of malloc() in the readBytesFromFileWithExtendedAttributes function above. Generally the pattern in Foundation seems to be not checking the return value of malloc()/realloc() and letting it eventually abort on a NULL deref. The same is true if bytesRead exceeds Int.max

amt = read(fd, data!.advanced(by: bytesRead), blockSize)

// Dont continue on EINTR or EAGAIN as the file position may not
// have changed, see read(2).
if amt < 0 {
free(data!)
throw NSError(domain: NSPOSIXErrorDomain, code: Int(errno), userInfo: nil)
}
bytesRead += amt
} while amt > 0

if bytesRead == 0 {
free(data!)
data = malloc(0)
} else {
data = realloc(data, bytesRead) // shrink down the allocated block.
}

return NSDataReadResult(bytes: data!, length: bytesRead) { buffer, length in
free(buffer)
}
}

internal func makeTemporaryFile(inDirectory dirPath: String) throws -> (Int32, String) {
let template = dirPath._nsObject.appendingPathComponent("tmp.XXXXXX")
Expand Down
43 changes: 43 additions & 0 deletions TestFoundation/TestNSData.swift
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ class TestNSData: XCTestCase {
("test_base64Data_medium", test_base64Data_medium),
("test_base64Data_small", test_base64Data_small),
("test_openingNonExistentFile", test_openingNonExistentFile),
("test_contentsOfFile", test_contentsOfFile),
("test_contentsOfZeroFile", test_contentsOfZeroFile),
("test_basicReadWrite", test_basicReadWrite),
("test_bufferSizeCalculation", test_bufferSizeCalculation),
// ("test_dataHash", test_dataHash), Disabled due to lack of brdiging in swift runtime -- infinite loops
Expand Down Expand Up @@ -908,6 +910,47 @@ extension TestNSData {
XCTAssertTrue(didCatchError)
}

func test_contentsOfFile() {
let testDir = testBundle().resourcePath
let filename = testDir!.appending("/NSStringTestData.txt")

let contents = NSData(contentsOfFile: filename)
XCTAssertNotNil(contents)
if let contents = contents {
let ptr = UnsafeMutableRawPointer(mutating: contents.bytes)
let str = String(bytesNoCopy: ptr, length: contents.length,
encoding: .ascii, freeWhenDone: false)
XCTAssertEqual(str, "swift-corelibs-foundation")
}
}

func test_contentsOfZeroFile() {
#if os(Linux)
guard FileManager.default.fileExists(atPath: "/proc/self") else {
return
}
let contents = NSData(contentsOfFile: "/proc/self/cmdline")
XCTAssertNotNil(contents)
if let contents = contents {
XCTAssertTrue(contents.length > 0)
let ptr = UnsafeMutableRawPointer(mutating: contents.bytes)
let str = String(bytesNoCopy: ptr, length: contents.length,
encoding: .ascii, freeWhenDone: false)
XCTAssertNotNil(str)
if let str = str {
XCTAssertTrue(str.hasSuffix("TestFoundation"))
}
}

do {
let maps = try String(contentsOfFile: "/proc/self/maps", encoding: .utf8)
XCTAssertTrue(maps.count > 0)
} catch {
XCTFail("Cannot read /proc/self/maps: \(String(describing: error))")
}
#endif
}

func test_basicReadWrite() {
let url = URL(fileURLWithPath: NSTemporaryDirectory(), isDirectory: true).appendingPathComponent("testfile")
let count = 1 << 24
Expand Down