Foundation: fix invalid free

[compnerd]

The memory in the String is not-being copied, and thus is more of a
view into the memory, not an owning reference. Do not free this memory
after the destruction of the object.

[compnerd]

@swift-ci please test Linux platform

[millenomi]

cc @phausler

[compnerd]

Windows CI:

• Facebook: https://dev.azure.com/compnerd/swift-build/_build/results?buildId=26295&view=results
• Google: https://dev.azure.com/compnerd/swift-build/_build/results?buildId=26294&view=results

[spevans]

This makes a copy since it ends up calling:

public convenience init?(bytesNoCopy bytes: UnsafeMutableRawPointer, length len: Int, encoding: UInt, freeWhenDone freeBuffer: Bool) /* "NoCopy" is a hint */ {
        // just copy for now since the internal storage will be a copy anyhow
        self.init(bytes: bytes, length: len, encoding: encoding)
        if freeBuffer { // don't take the hint
            free(bytes)
        }
    }

So I think it will leak. If you are going to pass false I think you will need to call ptr.deallocate() after creating the string.

[compnerd]

Ugh, I missed the UnsafeMutableRawPointer.allocate on L601. You are right, this is a leak. So, there something going on here thats triggering a ton of failures/heap corruption on Windows.

[spevans]

Maybe there is an issue estimating the buffer size to use and its under allocating. I rewrote this code recently so I may have introduced a bug. You could try allocating capacity * 3 or something just to make sure the buffer is definitely big enough and see if you still see the issue.

[compnerd]

Just tested it locally, over-estimating the capacity does not seem to help

[spevans]

Maybe this is a simpler fix:

@ -597,12 +597,14 @@ open class NSData : NSObject, NSCopying, NSMutableCopying, NSSecureCoding {
         let dataLength = self.length
         if dataLength == 0 { return "" }
 
-        let capacity = estimateBase64Size(length: dataLength)
+        let capacity = estimateBase64Size(length: dataLength) + 1
         let ptr = UnsafeMutableRawPointer.allocate(byteCount: capacity, alignment: 1)
         let buffer = UnsafeMutableRawBufferPointer(start: ptr, count: capacity)
         let length = NSData.base64EncodeBytes(self, options: options, buffer: buffer)
-
-        return String(bytesNoCopy: ptr, length: length, encoding: .ascii, freeWhenDone: true)!
+        buffer[length] = 0
+        let string = String(validatingUTF8: ptr.assumingMemoryBound(to: CChar.self))!
+        ptr.deallocate()
+        return string
     }
 
     /// Creates a Base64, UTF-8 encoded Data from the data object using the given options.

[compnerd]

Updated but using defer for the ptr.deallocate().

[spevans]

I wonder if this is ultimately a bug in the function NSString.init?(bytes: UnsafeRawPointer, length len: Int, encoding: UInt) which is being called by the String(bytesNoCopy.. call as I cant see anything wrong with the original code.

[compnerd]

This reminds me of an old issue that never really got resolved - there is a difference between SwiftFoundation and Foundation where lengths between CFString and NSData are different due to the trailing null-terminator. This would fit that scenario - the backing here is NSData rather than CFString.

I think that is a separate issue to iron out and we should still merge this though.

[spevans]

I agree that the issue is probably in the NSData -> String conversion, Im having a look at it now. Could we leave this unmerged for a bit as its the only current reproducer of the bug and would be good to see if it can be fixed properly.

[spevans]

I found an issue with my fix so feel free to merge, I will see If I can find a longer timer solution.

[compnerd]

@swift-ci please test Linux platform

[compnerd]

@swift-ci please test Linux platform