Skip to content

SR-8999: NSData contentsOf expects st_size to be the size of the file. #1732

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

Merged
merged 2 commits into from
Oct 23, 2018
Merged

SR-8999: NSData contentsOf expects st_size to be the size of the file. #1732

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5733313
SR-8999: NSData contentsOf expects st_size to be the size of the file.
spevans Oct 19, 2018
d583377
SR-8999: Ensure the FileHandle read methods call fatalError() on error
spevans Oct 20, 2018
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
135 changes: 64 additions & 71 deletions Foundation/FileHandle.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,102 +31,95 @@ open class FileHandle : NSObject, NSSecureCoding {
}

open var availableData: Data {
return _readDataOfLength(Int.max, untilEOF: false)
do {
let readResult = try _readDataOfLength(Int.max, untilEOF: false)
return readResult.toData()
} catch {
fatalError("\(error)")
}
}

open func readDataToEndOfFile() -> Data {
return readData(ofLength: Int.max)
}

open func readData(ofLength length: Int) -> Data {
return _readDataOfLength(length, untilEOF: true)
do {
let readResult = try _readDataOfLength(length, untilEOF: true)
return readResult.toData()
} catch {
fatalError("\(error)")
}
}

internal func _readDataOfLength(_ length: Int, untilEOF: Bool) -> Data {
internal func _readDataOfLength(_ length: Int, untilEOF: Bool, options: NSData.ReadingOptions = []) throws -> NSData.NSDataReadResult {
precondition(_fd >= 0, "Bad file descriptor")
if length == 0 && !untilEOF {
// Nothing requested, return empty response
return NSData.NSDataReadResult(bytes: nil, length: 0, deallocator: nil)
}

var statbuf = stat()
var dynamicBuffer: UnsafeMutableRawPointer? = nil
var total = 0
if fstat(_fd, &statbuf) < 0 {
fatalError("Unable to read file")
throw NSError(domain: NSPOSIXErrorDomain, code: Int(errno), userInfo: nil)
}
if statbuf.st_mode & S_IFMT != S_IFREG {
/* We get here on sockets, character special files, FIFOs ... */
var currentAllocationSize: size_t = 1024 * 8
dynamicBuffer = malloc(currentAllocationSize)
var remaining = length
while remaining > 0 {
let amountToRead = min(1024 * 8, remaining)
// Make sure there is always at least amountToRead bytes available in the buffer.
if (currentAllocationSize - total) < amountToRead {
currentAllocationSize *= 2
dynamicBuffer = _CFReallocf(dynamicBuffer!, currentAllocationSize)
if dynamicBuffer == nil {
fatalError("unable to allocate backing buffer")

let readBlockSize: Int
if statbuf.st_mode & S_IFMT == S_IFREG {
// TODO: Should files over a certain size always be mmap()'d?
Copy link
Contributor

Choose a reason for hiding this comment

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

Immediate answer: no, unless if options contains .alwaysMapped or .mappedIfSafe.

(I don't see .mappedIfSafe handled here at all? Are you just assuming mapping is always unsafe? This is allowed by the contract but we should have a follow-up.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.mappedIfSafe wasn't handled in the previous code so I haven't added it but I can do a separate PR for it.

if options.contains(.alwaysMapped) {
// Filesizes are often 64bit even on 32bit systems
let mapSize = min(length, Int(clamping: statbuf.st_size))
let data = mmap(nil, mapSize, PROT_READ, MAP_PRIVATE, _fd, 0)
// Swift does not currently expose MAP_FAILURE
if data != UnsafeMutableRawPointer(bitPattern: -1) {
return NSData.NSDataReadResult(bytes: data!, length: mapSize) { buffer, length in
munmap(buffer, length)
}
}
let amtRead = read(_fd, dynamicBuffer!.advanced(by: total), amountToRead)
if 0 > amtRead {
free(dynamicBuffer)
fatalError("read failure")
}
if 0 == amtRead {
break // EOF
}

total += amtRead
remaining -= amtRead

if total == length || !untilEOF {
break // We read everything the client asked for.
}
}

if statbuf.st_blksize > 0 {
readBlockSize = Int(clamping: statbuf.st_blksize)
} else {
readBlockSize = 1024 * 8
}
} else {
let offset = lseek(_fd, 0, SEEK_CUR)
if offset < 0 {
fatalError("Unable to fetch current file offset")
/* We get here on sockets, character special files, FIFOs ... */
readBlockSize = 1024 * 8
}
var currentAllocationSize = readBlockSize
var dynamicBuffer = malloc(currentAllocationSize)!
var total = 0

while total < length {
let remaining = length - total
let amountToRead = min(readBlockSize, remaining)
// Make sure there is always at least amountToRead bytes available in the buffer.
if (currentAllocationSize - total) < amountToRead {
currentAllocationSize *= 2
dynamicBuffer = _CFReallocf(dynamicBuffer, currentAllocationSize)
}
if off_t(statbuf.st_size) > offset {
var remaining = size_t(off_t(statbuf.st_size) - offset)
remaining = min(remaining, size_t(length))

dynamicBuffer = malloc(remaining)
if dynamicBuffer == nil {
fatalError("Malloc failure")
}

while remaining > 0 {
let count = read(_fd, dynamicBuffer!.advanced(by: total), remaining)
if count < 0 {
free(dynamicBuffer)
fatalError("Unable to read from fd")
}
if count == 0 {
break
}
total += count
remaining -= count
}
let amtRead = read(_fd, dynamicBuffer.advanced(by: total), amountToRead)
if amtRead < 0 {
free(dynamicBuffer)
throw NSError(domain: NSPOSIXErrorDomain, code: Int(errno), userInfo: nil)
}
total += amtRead
if amtRead == 0 || !untilEOF { // If there is nothing more to read or we shouldnt keep reading then exit
break
}
}

if length == Int.max && total > 0 {
dynamicBuffer = _CFReallocf(dynamicBuffer!, total)
}

if total == 0 {
free(dynamicBuffer)
return NSData.NSDataReadResult(bytes: nil, length: 0, deallocator: nil)
}
else if total > 0 {
let bytePtr = dynamicBuffer!.bindMemory(to: UInt8.self, capacity: total)
return Data(bytesNoCopy: bytePtr, count: total, deallocator: .free)
}
else {
assertionFailure("The total number of read bytes must not be negative")
free(dynamicBuffer)
dynamicBuffer = _CFReallocf(dynamicBuffer, total)
let bytePtr = dynamicBuffer.bindMemory(to: UInt8.self, capacity: total)
return NSData.NSDataReadResult(bytes: bytePtr, length: total) { buffer, length in
free(buffer)
}

return Data()
}

open func write(_ data: Data) {
Expand Down
101 changes: 16 additions & 85 deletions Foundation/NSData.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ open class NSData : NSObject, NSCopying, NSMutableCopying, NSSecureCoding {

if url.isFileURL {
let data = try NSData.readBytesFromFileWithExtendedAttributes(url.path, options: readOptionsMask)
readResult = NSData(bytesNoCopy: data.bytes, length: data.length, deallocator: data.deallocator)
readResult = data.toNSData()
} else {
let session = URLSession(configuration: URLSessionConfiguration.default)
let cond = NSCondition()
Expand Down Expand Up @@ -410,102 +410,33 @@ open class NSData : NSObject, NSCopying, NSMutableCopying, NSSecureCoding {

// MARK: - IO
internal struct NSDataReadResult {
var bytes: UnsafeMutableRawPointer
var bytes: UnsafeMutableRawPointer?
var length: Int
var deallocator: ((_ buffer: UnsafeMutableRawPointer, _ length: Int) -> Void)?
}

internal static func readBytesFromFileWithExtendedAttributes(_ path: String, options: ReadingOptions) throws -> NSDataReadResult {
let fd = _CFOpenFile(path, O_RDONLY)
if fd < 0 {
throw NSError(domain: NSPOSIXErrorDomain, code: Int(errno), userInfo: nil)
}
defer {
close(fd)
}
var deallocator: ((_ buffer: UnsafeMutableRawPointer, _ length: Int) -> Void)!

var info = stat()
let ret = withUnsafeMutablePointer(to: &info) { infoPointer -> Bool in
if fstat(fd, infoPointer) < 0 {
return false
func toNSData() -> NSData {
if bytes == nil {
return NSData()
}
return true
}

if !ret {
throw NSError(domain: NSPOSIXErrorDomain, code: Int(errno), userInfo: nil)
}

let length = Int(info.st_size)
if length == 0 && (info.st_mode & S_IFMT == S_IFREG) {
return try readZeroSizeFile(fd)
return NSData(bytesNoCopy: bytes!, length: length, deallocator: deallocator)
}

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

// Swift does not currently expose MAP_FAILURE
if data != UnsafeMutableRawPointer(bitPattern: -1) {
return NSDataReadResult(bytes: data!, length: length) { buffer, length in
munmap(buffer, length)
}
func toData() -> Data {
guard let bytes = bytes else {
return Data()
}

}

let data = malloc(length)!
var remaining = Int(info.st_size)
var total = 0
while remaining > 0 {
let amt = read(fd, data.advanced(by: total), remaining)
if amt < 0 {
break
}
remaining -= amt
total += amt
return Data(bytesNoCopy: bytes, count: length, deallocator: Data.Deallocator.custom(deallocator))
}
}

if remaining != 0 {
free(data)
internal static func readBytesFromFileWithExtendedAttributes(_ path: String, options: ReadingOptions) throws -> NSDataReadResult {
guard let handle = FileHandle(path: path, flags: O_RDONLY, createMode: 0) else {
throw NSError(domain: NSPOSIXErrorDomain, code: Int(errno), userInfo: nil)
}

return NSDataReadResult(bytes: data, length: length) { buffer, length in
free(buffer)
}
let result = try handle._readDataOfLength(Int.max, untilEOF: true)
return result
}

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)
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")
let maxLength = Int(PATH_MAX) + 1
Expand Down
12 changes: 12 additions & 0 deletions TestFoundation/TestNSData.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,7 @@ class TestNSData: LoopbackServerTest {
("test_openingNonExistentFile", test_openingNonExistentFile),
("test_contentsOfFile", test_contentsOfFile),
("test_contentsOfZeroFile", test_contentsOfZeroFile),
("test_wrongSizedFile", test_wrongSizedFile),
("test_contentsOfURL", test_contentsOfURL),
("test_basicReadWrite", test_basicReadWrite),
("test_bufferSizeCalculation", test_bufferSizeCalculation),
Expand Down Expand Up @@ -1494,6 +1495,17 @@ extension TestNSData {
#endif
}

func test_wrongSizedFile() {
#if os(Linux)
// Some files in /sys report a non-zero st_size often bigger than the contents
guard let data = NSData.init(contentsOfFile: "/sys/kernel/profiling") else {
XCTFail("Cant read /sys/kernel/profiling")
return
}
XCTAssert(data.length > 0)
#endif
}

func test_contentsOfURL() {
let urlString = "http://127.0.0.1:\(TestURLSession.serverPort)/country.txt"
let url = URL(string: urlString)!
Expand Down