SR-7112: Fixed access to memory out of range

[tid-kijyun]

Fixed the code accessing outside the range of the character string.
When running the test in Xcode, Guard Malloc (libgmalloc) detected it and caused a runtime error.

[ikesyo]

It seems that this is a fix for https://bugs.swift.org/browse/SR-7112, right?

[tid-kijyun]

Exactly! This PR is a solution to that issue.

[spevans]

@swift-ci please test

[spevans]

@swift-ci please test

[parkera]

I'm trying to trace this through from the original bug report but I'm having trouble figuring out why we end up out of bounds here in the first place. The string is [null, null null] -- but the argument values in that function are range 0, length 0. Does that mean CFStringGetCStringPtr returned an invalid pointer in the first place?

[parkera]

If this is a recent failure, it may be related to @milseman 's string changes...

[spevans]

@parkera When I had a look at this issue it seemed to be triggered with 16 (ASCII) character strings, I wonder if its related to a short string optimisation? It can be triggered by running the TestFoundation tests under Xcode, so it might be the malloc checker looking at overflows of 16byte allocations - I guess on linux the minimum malloc size is > 16 so its not triggered in the tests.

[milseman]

Note that small strings landed last week, but that bug report is from a month ago.

[parkera]

Is it possible to re-create this failure without the JSON bit in particular? I tried a bit on Darwin to get it to happen but couldn't - which makes me wonder about the GetCString call being incorrectly implemented for the Swift-string backed NSString.

[tid-kijyun]

This is reproduced with this code. This problem occurs when giving an ASCII string of multiples of 16.
As you know 16 bytes is the same as the minimum block size of macOS's malloc.
Note: I have not yet investigated how Swift does String's memory layout.

_ = ("1234567890123456" as NSString).data(using: String.Encoding.utf8.rawValue)

There is a problem reading the end of the character string pointed to by pointer ptr in this loop condition.
https://github.com/apple/swift-corelibs-foundation/blob/2ba2086405c864e0c8bdb223209cf9a87e74a1d2/CoreFoundation/String.subproj/CFStringEncodings.c#L639-L642
ptr is obtained using unsafeBitCast in NSString._fastCStringContents.
(Also I'm concerned that the argument nullTerminated is not used.)
https://github.com/apple/swift-corelibs-foundation/blob/2ba2086405c864e0c8bdb223209cf9a87e74a1d2/Foundation/NSString.swift#L298-L305
For reference, NSString.getCString sets the NULL terminator by itself.
https://github.com/apple/swift-corelibs-foundation/blob/2ba2086405c864e0c8bdb223209cf9a87e74a1d2/Foundation/NSString.swift#L863-L867
So at the moment I think there is a problem with accessing an address which is not known whether there is a NULL terminator for the pointer obtained by unsafeBitCast.

Do you think there is another cause?

[milseman]

Near as I can tell, corelibs-foundation's implementation of _fastCStringContents is unsound if it relies in nul-termination. I brought this up here: #1484 (comment)

[parkera]

Ok, I see now. You're right that CFStringCStringPtr must return a NULL-terminated string. In fact that _fastCStringContents function has an argument specifying if it needs to return a null terminated string or not. In these cases we must be passing yes but NSString is not meeting the promise by ignoring the argument value. On Darwin it can do it but as you point out, Swift string does not provide the right guarantee.

Therefore, unfortunately, the right fix to this bug is to do what you originally suggested and return NULL in those functions, unless there is some case where we can actually get a null-terminated string out of Swift.String.

[milseman]

@parkera I think Swift's String should always over-allocate to guarantee nul-termination for ASCII. Does the UTF-16 version, _fastCharacterContents also assume nul-termination?

These of course won't work for slices, nor for Strings containing nul. Would Swift need to remember whether its ever stored an internal nul character? Do you know why they don't just return both a pointer and a length? I could imagine many uses might follow up with a strlen call anyways, and often the length is known.

[parkera]

It looks like _fastCharacterContents is required to return a NULL terminated string.

In NSString-land it's not so strange that this doesn't return an out-argument for length. If you need it you just call the length method yourself on the string. No need for strlen -- the result of length is probably already at hand in O(1).

_fastCStringContents is only required to null terminate if the argument is true. There may be cases like slices which can implement this function if someone asks for the characters and passes false because they already have the length themselves for some reason.

[milseman]

I see, and are you careful to track whether the string happened to store a NUL character in the middle? Or do you just return the pointer, leaving the behavior surrounding an interior NUL up to the caller?

(Personally, I find the second option more tenable)

[tid-kijyun]

Therefore, unfortunately, the right fix to this bug is to do what you originally suggested and return NULL in those functions,

@parkera You are absolutely right. However, I think that we should carefully change _fastCStringContents after creating a new bug report on bugs.swift.org.
As discussed here: #1484(comment), Changing _fastCStringContents involves overhead. Perhaps memory allocation occurs and it must be managed by NSString. We also need to think about whether we need arguments for this method. (This method is currently used only in CFStringGetCString, and its argument is always passed as True.) There may be other things to think about...

My main point is that I would like to quickly improve the situation where the test abnormally ends for those who want to contribute using macOS/Xcode.
This fix is not a fundamental solution, but it can avoid this problem without changing the current behavior at all. There is no logical change since I just replaced the right side and the left side of the AND condition. There are no side effects. The code associated with the fix works whether or not NULL termination exists in the first place.

[parkera]

There is clearly a logical change in this code, otherwise it wouldn't fix the issue.

The root cause is that CFStringGetCStringPtr is behaving in a way that this code does not expect. This happens to be one place where we've detected it, but there are probably a bunch of others because we use this function all over the place in CF.

Right now we have a signal that shows the true problem. Changing this code seems fine, but it papers over the underlying issue and will give us a false sense of confidence that we've fixed the issue. We can take the time to dig to the bottom of why the test is failing and fix it once.

[parkera]

A cheap way to fix the _fastCStringContents function, at least in terms of how much source change is required, is simply to return NULL. We almost did that in the other PR that you've referenced. Then the follow-up bug is "improve performance for this call" instead of "there may be other mysterious out-of-bounds issues when using this call".

[tid-kijyun]

I understand. Ok, I've changed it so that it simply returns NULL if the argument is True. I was concerned about changing the behavior of the existing code, but it is better than leaving the root cause.

Note that in this case I think that it is correct to return NULL, as it is written in the document (although it is Darwin's CFStringGetCStringPtr) as follows:

This function either returns the requested pointer immediately, with no memory allocations and no copying, in constant time, or returns NULL.

- CFStringGetCStringPtr

Thanks

[milseman]

Can you file a bug against String to over-allocate by 1 code unit for its storage?

edit: hyperlinking

[tid-kijyun]

@milseman Do you mean there is no null terminator in Swift's internal string storage? Unfortunately, I can not report about it. I think that there is no problem even if it does not exist, because It is not necessary for Swift. Also I think that swift-corelibs-foundation should not depend on Swift's internal implementation whenever possible.

[milseman]

It's generally useful in the ASCII case for things like withCString, which can avoid allocating a copy. We could similarly do it for UTF-16 storage, if there's benefit.

[tid-kijyun]

The pointer obtained with withCString is a valid pointer only in the closure. So I think that we can not use it in this case.

[nevil]

Edit:
As discussed above, ptr is a c-string so it should be fine to dereference *ptr to read the nul byte when rangeLen reached 0...

Ignore my comment below :-)

---

I have not analyzed this crash very deeply so I'm sorry for the noise...

I'm just wondering if it is always safe to dereference ptr here?
https://github.com/apple/swift-corelibs-foundation/blob/2ba2086405c864e0c8bdb223209cf9a87e74a1d2/CoreFoundation/String.subproj/CFStringEncodings.c#L639-L642

When the crash happens for me, rangeLen and uninterestingTailLen are both 0.
So if we swap the order the illegal dereference doesn't happen:

                while (rangeLen > uninterestingTailLen && *ptr < 0x80) {

[tid-kijyun]

@nevil At first I was doing the same patch as you. However, I accepted and withdrew the opinion that the root cause should be fixed. Since there is a log, I will skip the details.

[parkera]

Let's get this merged!

[parkera]

@swift-ci please test and merge