Skip to content
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

Revert "Foundation: fix missing string terminator" #2676

Merged
merged 1 commit into from
Feb 22, 2020
+14 −4
Merged

Revert "Foundation: fix missing string terminator" #2676

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions Docs/ReleaseNotes_Swift5.md
View file
Original file line number Diff line number Diff line change
@@ -156,3 +156,11 @@ A few notes:
* The exception-throwing API will be deprecated in a future release. It is now marked as `@available(…, deprecated: 100000, …)`, which matches what you would see if `API_TO_BE_DEPRECATED` was used in a Objective-C header.

* Subclassing `NSFileHandle` is strongly discouraged. Many of the new methods are `public` and cannot be overridden.

## `NSString(bytesNoCopy:length:encoding:freeWhenDone:)` deviations from reference implementation

On Windows, `NSString(bytesNoCopy:length:encoding:freeWhenDone:)` deviates from
the behaviour on Darwin and uses the buffer's `deallocate` routine rather than
`free`. This is done to ensure that the correct routine is invoked for
releasing the resources acquired through `UnsafeMutablePointer.allocate` or
`UnsafeMutableRawPointer.allocate`.
4 changes: 1 addition & 3 deletions Sources/Foundation/NSData.swift
View file
Original file line number Diff line number Diff line change
@@ -599,12 +599,10 @@ open class NSData : NSObject, NSCopying, NSMutableCopying, NSSecureCoding {

let capacity = estimateBase64Size(length: dataLength)
let ptr = UnsafeMutableRawPointer.allocate(byteCount: capacity, alignment: 1)
defer { ptr.deallocate() }
let buffer = UnsafeMutableRawBufferPointer(start: ptr, count: capacity)
let length = NSData.base64EncodeBytes(self, options: options, buffer: buffer)

let utf8buffer = UnsafeBufferPointer<UInt8>(start: ptr.assumingMemoryBound(to: UInt8.self), count: length)
return String(decoding: utf8buffer, as: UTF8.self)
return String(bytesNoCopy: ptr, length: length, encoding: .ascii, freeWhenDone: true)!
}

/// Creates a Base64, UTF-8 encoded Data from the data object using the given options.
6 changes: 5 additions & 1 deletion Sources/Foundation/NSString.swift
View file
Original file line number Diff line number Diff line change
@@ -1359,7 +1359,11 @@ extension NSString {
// 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)
#if os(Windows)
bytes.deallocate()
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this is documented to call free: https://developer.apple.com/documentation/foundation/nsstring/1413830-initwithbytesnocopy

I think we should replicate this comment and the caller should make sure that the memory is allocated using malloc. WDYT?

Copy link
Contributor

@millenomi millenomi Feb 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

So:

  • On Linux and Darwin, free() is ultimately compatible with the allocation done by CF, ObjC and Swift. This is sort of a non-issue on those platforms.
  • On other platforms, there can be a distinction.

This has two sides: a contract side and an implementation side.

On the contract side: in ObjC, the original API here takes a void * and the language does not mandate how you produce that pointer, only that it be valid. It makes sense here to specify precisely the deallocation strategy.

But. This is Swift. And in Swift, this type is UnsafeMutableRawPointer — a type that does have a language-provided default deallocation facility. And in this language, since there are far fewer guarantees wrt the memory layout of language constructs*, the vast majority of these pointers will be produced by the .allocate(byteCount:alignment:) method of that class or the equivalent bound variations in the non-Raw pointers.

My opinion here is that we can deviate from Darwin Foundation as follows:

  • .deallocate() will eventually call through to free() on Linux and Darwin. We are going to rely on this property, for now, or conditionality the change away from free() on these OSes, so long that it interoperates correctly with buffers allocated with .allocate(…).

  • On Windows and other OSes where it's not guaranteed that .allocate(…) use malloc, we mandate that freeWhenDone: true use .deallocate() instead of free(). This puts an onus on the caller to use .allocate(…) rather than use OS facilities directly. I think this is a reasonable requirement to make: callers that choose a nonstandard allocation strategy on other OSes are already mandated to use freeWhenDone: false with these pointers and handle deallocation themselves; malloc() just happens to not be the standard on those OSes.

The contract side is straightforward-ish — we're just extending the existing contract to go from 'specifically free()' to 'the system-preferred deallocation strategy, which on Linux and Darwin is free()'. The implementation side puts a burden on implementors on those OSes where this is not the case — which means that existing setups don't regress.

I think I am in favor of making this change conditional on the OS, and we should note it in the release notes (in Docs/ReleaseNotes*.md) as a well-known deviation from Darwin behavior.

(* that is, you "can't just" take a pointer to a Swift.Array like you can for an array in C, and have strong guarantees about the memory layout of that object unless you are working hand in hand with the compiler and the standard library implementations.)

Copy link
Contributor

Choose a reason for hiding this comment

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

@millenomi but what happens to all the code that was previously correctly matching malloc with freeWhenDone: true? That works today and will then break, correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

@weissi I actually have code that passes raw buffer originated from C module (which uses malloc internally) to String(bytesNoCopy:... with "freeWenDone: true". And it crashes with this patch. I understand motivation though.

@compnerd I think we need to add one more detail to the docs. Official documentation doesn't clarify when freeWhenDone takes effect. It is unclear that on Windows we are allowed to do manual cleanup right after String init. As user, I'd rather think that String(bytesNoCopy:.. will need specified buffer alive until deinit.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lxbndr Thanks for chiming in! I mean it is documented to free so I think it should free. If we think we need a Swift version of this, why not create a new String(bytesNoCopy:...deallocateWhenDone:) that would call deallocate?

I'm pretty strongly against just changing the contract.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lxbndr Are you saying that memory allocated with malloc but then passed as an UnsafeRawMutableBufferPointer then has deallocate() called on it crashes? Is this on windows? Because that sounds like another bug.

@weichsel Data has the following initialiser: init(bytesNoCopy bytes: UnsafeMutableRawPointer, count: Int, deallocator: Data.Deallocator) where the deallocator can be specified including a custom one passed with a closure so that probably offers a good way to do it although it needs deallocate() added into Data.Deallocator https://developer.apple.com/documentation/foundation/data/deallocator

Copy link
Contributor

Choose a reason for hiding this comment

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

@spevans Yes, this is on Windows. deallocateWhenDone:false together with manual free() works like a charm. Initial PR description now seems about a bit different case. It's about Swift buffers which can't be passed to free(). I think I need look closer on my case.

Copy link
Contributor

Choose a reason for hiding this comment

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

@weissi Feels like band-aid, I agree. Approach with custom deallocator like in Data looks way better.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lxbndr We're looking into a more comprehensive solution that will fix the issue for good, but a quick search on GitHub shows that using UMRP.alloc…(…) with freeWhenDone: is ubiquitous and prevalent.

#else
free(bytes)
#endif
}
}