Skip to content

Support internal null characters in NSString initializers [SR-2225] #496

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

Closed
wants to merge 2 commits into from
Closed

Support internal null characters in NSString initializers [SR-2225] #496

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
9aafb17
Update NSString initializers [SR-2225]
xwu Jul 31, 2016
61ca9cf
Add test for NSString initializers [SR-2225]
xwu Jul 31, 2016
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
17 changes: 6 additions & 11 deletions Foundation/NSString.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1221,10 +1221,7 @@ extension NSString {
}

public convenience init?(data: Data, encoding: UInt) {
guard let cf = data.withUnsafeBytes({ (bytes: UnsafePointer<UInt8>) -> CFString? in
return CFStringCreateWithBytes(kCFAllocatorDefault, bytes, data.count, CFStringConvertNSStringEncodingToEncoding(encoding), true)
}) else { return nil }

guard let cf = CFStringCreateFromExternalRepresentation(kCFAllocatorDefault, data._cfObject, CFStringConvertNSStringEncodingToEncoding(encoding)) else { return nil }
Copy link
Contributor

Choose a reason for hiding this comment

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

The story of what happens on Darwin here is fairly complicated, which is probably why this was different in the first place, but it boils down to this:

- initWithData:(NSData *)data encoding:(NSStringEncoding)encoding {
    return (id)CFStringCreateFromExternalRepresentation(kCFAllocatorDefault, (CFDataRef)data, CFStringConvertNSStringEncodingToEncoding(encoding));
}

So this change looks correct.

var str: String?
if String._conditionallyBridgeFromObject(cf._nsObject, result: &str) {
self.init(str!)
Expand All @@ -1234,8 +1231,8 @@ extension NSString {
}

public convenience init?(bytes: UnsafeRawPointer, length len: Int, encoding: UInt) {
let bytePtr = bytes.bindMemory(to: UInt8.self, capacity: len)
guard let cf = CFStringCreateWithBytes(kCFAllocatorDefault, bytePtr, len, CFStringConvertNSStringEncodingToEncoding(encoding), true) else {
let extRep = NSData(bytesNoCopy: UnsafeMutableRawPointer(mutating: bytes), length: len, freeWhenDone: false)._cfObject
Copy link
Contributor

@parkera parkera Aug 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

But this one should remain using the bytes function:

- initWithBytes:(const void *)bytes length:(NSUInteger)len encoding:(NSStringEncoding)encoding {
    return (id)CFStringCreateWithBytes(kCFAllocatorDefault, (const UInt8 *)bytes, len, CFStringConvertNSStringEncodingToEncoding(encoding), true);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's interesting. I'm not in front of a computer at the moment, but IIRC, testing on OS X showed that Apple Foundation NSString.init(bytes:length:encoding:) proceeded past an internal null character. I will check again later in the day and report back.

Copy link
Contributor

Choose a reason for hiding this comment

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

It may depend on exactly how you're calling it. Those are from the placeholder string, which is the result of calling alloc on NSString, but not on a subclass of NSString.

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'm not quite sure I understand what you mean by placeholder string. However, it's clear that Apple Foundation reads past the internal null character with NSString.init(bytes:length:encoding:), as demonstrated below:

echo -e "This is a string.\0This is another string." > /Users/xwu/Desktop/test.txt
let url = URL(fileURLWithPath: "/Users/xwu/Desktop/test.txt")
let data = try Data(contentsOf: url)
data.withUnsafeBytes { (bytes: UnsafePointer<UInt8>) in
  let string = NSString(bytes: bytes, length: data.count, encoding: String.Encoding.ascii.rawValue)
  // "This is a string.\0This is another string."
}

Not sure what it would take to achieve this result if we are to use CFStringCreateWithBytes in corelibs-foundation. Likewise with the original bug regarding init(contentsOf: URL, encoding: UInt): I'm not sure how to interpret the discrepancy if Apple Foundation is doing nothing more than calling through to CFStringCreateWithBytes.

guard let cf = CFStringCreateFromExternalRepresentation(kCFAllocatorDefault, extRep, CFStringConvertNSStringEncodingToEncoding(encoding)) else {
return nil
}
var str: String?
Expand Down Expand Up @@ -1265,12 +1262,10 @@ extension NSString {
return nil
}
}

public convenience init(contentsOf url: URL, encoding enc: UInt) throws {
let readResult = try NSData(contentsOf: url, options: [])

let bytePtr = readResult.bytes.bindMemory(to: UInt8.self, capacity: readResult.length)
guard let cf = CFStringCreateWithBytes(kCFAllocatorDefault, bytePtr, readResult.length, CFStringConvertNSStringEncodingToEncoding(enc), true) else {
guard let cf = CFStringCreateFromExternalRepresentation(kCFAllocatorDefault, readResult._cfObject, CFStringConvertNSStringEncodingToEncoding(enc)) else {
Copy link
Contributor

Choose a reason for hiding this comment

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

This actually calls through to initWithBytes:length:encoding:.

throw NSError(domain: NSCocoaErrorDomain, code: NSCocoaError.FileReadInapplicableStringEncodingError.rawValue, userInfo: [
"NSDebugDescription" : "Unable to create a string using the specified encoding."
])
Expand All @@ -1284,7 +1279,7 @@ extension NSString {
])
}
}

public convenience init(contentsOfFile path: String, encoding enc: UInt) throws {
try self.init(contentsOf: URL(fileURLWithPath: path), encoding: enc)
}
Expand Down
13 changes: 12 additions & 1 deletion TestFoundation/TestNSString.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ class TestNSString : XCTestCase {
("test_FromNullTerminatedCStringInUTF8", test_FromNullTerminatedCStringInUTF8 ),
// Swift3 updates broke the expectations of this test. disabling for now
// ("test_FromMalformedNullTerminatedCStringInUTF8", test_FromMalformedNullTerminatedCStringInUTF8 ),
("test_FromBytesOrDataWithInternalNullCharacter", test_FromBytesOrDataWithInternalNullCharacter ),
("test_uppercaseString", test_uppercaseString ),
("test_lowercaseString", test_lowercaseString ),
("test_capitalizedString", test_capitalizedString ),
Expand All @@ -69,7 +70,7 @@ class TestNSString : XCTestCase {
("test_FromContentOfFile",test_FromContentOfFile),
("test_swiftStringUTF16", test_swiftStringUTF16),
// This test takes forever on build servers; it has been seen up to 1852.084 seconds
// ("test_completePathIntoString", test_completePathIntoString),
// ("test_completePathIntoString", test_completePathIntoString),
("test_stringByTrimmingCharactersInSet", test_stringByTrimmingCharactersInSet),
("test_initializeWithFormat", test_initializeWithFormat),
("test_initializeWithFormat2", test_initializeWithFormat2),
Expand Down Expand Up @@ -271,6 +272,16 @@ class TestNSString : XCTestCase {
XCTAssertNil(string)
}

func test_FromBytesOrDataWithInternalNullCharacter() {
let bytes = mockASCIIStringBytes + [0x00] + mockASCIIStringBytes
let length = mockASCIIStringBytes.count * 2 + 1
let data = Data(bytes: bytes, count: length)
let string1 = NSString(bytes: bytes, length: length, encoding: String.Encoding.ascii.rawValue)
XCTAssertTrue(string1?.isEqual(to: mockASCIIString + "\0" + mockASCIIString) ?? false)
let string2 = NSString(data: data, encoding: String.Encoding.ascii.rawValue)
XCTAssertTrue(string2?.isEqual(to: mockASCIIString + "\0" + mockASCIIString) ?? false)
}

func test_FromContentsOfURL() {
guard let testFileURL = testBundle().urlForResource("NSStringTestData", withExtension: "txt") else {
XCTFail("URL for NSStringTestData.txt is nil")
Expand Down