Fix buffer overflow in _NSCFString.getBytes #5287
Conversation
|
@swift-ci test |
|
This caused a new test failure: This failure doesn't occur on the base |
|
@swift-ci please test |
Sources/Foundation/NSCFString.swift
Outdated
| if let buffer = buffer { | ||
| for (idx, character) in encodingView.enumerated() { | ||
| if idx >= maxBufLen { break } | ||
| if encoding == CFStringEncoding(kCFStringEncodingASCII) && !Unicode.ASCII.isASCII(character) { break } |
There was a problem hiding this comment.
This line fixes the unit test failure. The issue was that this fix exposes the bug mentioned above on line 154:
// TODO: Don't treat many encodings like they are UTF8
Currently if the requested encoding is UTF8/ISOLatin1/MacRoman/ASCII/non-lossy ASCII we just encode UTF-8. Previously, we incorrectly reported the convertedLength as the UTF-8 length, but this fix updated that to correctly report the UTF-16 length processed. In plist encoding, when encoding a string the CoreFoundation code attempts getting ASCII bytes and if that fails it gets UTF-16 bytes encoded instead. Previously, it believed that the ASCII conversion failed because the reported UTF-8 length mismatched the known UTF-16 length. Now that the return value is correctly reporting the converted length, it began "passing" and thinking that it could encode the non-ASCII string as ASCII instead of falling over to UTF-16.
As a stopgap, this fixes the ASCII encoding by failing if we hit a non-ASCII character. We also need to do this same behavior (as the comment suggests) for the other non-UTF8 encodings here (and/or actually encode them properly 🙂). That's a larger change that I'm proposing we do as a followup since this is effectively already broken for the other encodings and I don't think it makes it worse (just perhaps more discoverable) and the other encodings are likely far less common than ASCII.
|
@swift-ci please test |
|
@swift-ci please test Windows platform |
1 similar comment
|
@swift-ci please test Windows platform |
Catfish-Man
left a comment
Catfish-Man
left a comment
There was a problem hiding this comment.
LGTM, thanks for catching this
* Fix buffer overflow in _NSCFString.getBytes * Fix unit test failure * Fix test_dateParseAndFormatWithJapaneseCalendar
3 participants
_NSCFString.getBytesdoes not currently read/respect the provided max buffer length when writing bytes out to the provided buffer. When an insufficiently sized buffer is provided, a buffer overflow occurs. This appeared as a symptom inplutilwhere reading a plist with a long unicode string would cause a buffer overflow since the UTF16 count was small enough for plutil to use a stack buffer instead of a dynamically-sized heap buffer, but the UTF8 count was long enough that writing the bytes overflows the stack buffer. This PR resolves the issue by breaking during byte iteration when we reach the max buffer length.