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.

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

Jump to bottom

[SR-5363] Update Foundation.Data API for UnsafeRawBufferPointer. #3839

Closed
atrick opened this issue Jul 3, 2017 · 2 comments
Closed

[SR-5363] Update Foundation.Data API for UnsafeRawBufferPointer. #3839

atrick opened this issue Jul 3, 2017 · 2 comments
Labels
bug Foundation Standard Library

Comments

@atrick
Copy link
Member

atrick commented Jul 3, 2017

Previous ID SR-5363
Radar rdar://28201288
Original Reporter @atrick
Type Bug
Status Closed
Resolution Duplicate
Additional Detail from JIRA
Votes 1
Component/s Foundation, Standard Library
Labels Bug
Assignee None
Priority Medium

md5: 47a2b35ea104f50c0451e7d810cc42b6

cloned to:

  • SR-9720 Foundation Data API: add Data.copyBytes methods taking UnsafeRawBufferPointer.

Issue Description:

Data's current API's that interoperate with unsafe pointers are awkward, and misleading. They should be updated now that we have UnsafeRawBufferPointer (SE-0183).

Prototype:
atrick/swift@796b08f

commit 796b08fa5635a7d61cbb6bf0718178e0ce326e4f
Author: Andrew Trick <atrick@apple.com>
Date:   Wed Sep 7 20:07:21 2016

    Add Foundation Data interop with UnsafeRawBufferPointer.
    
    Adds these initializers to construct a data object from a slice of raw memory:
      public init(buffer: UnsafeMutableRawBufferPointer)
      public init(bufferNoCopy buffer: UnsafeMutableRawBufferPointer, deallocator: Deallocator)
    
    Deprecates these initializers because they are redundant:
      public init(bytes: UnsafeRawPointer, count: Int)
      public init(bytesNoCopy bytes: UnsafeMutableRawPointer, count: Int, deallocator: Deallocator)
    
    Adds these closure applications that expose Data's underlying raw memory:
      public func withUnsafeBytes<ResultType>(
        _ body: (UnsafeRawBufferPointer) throws -> ResultType
        ) rethrows -> ResultType
      public mutating func withUnsafeMutableBytes<ResultType>(
        _ body: (UnsafeMutableRawBufferPointer) throws -> ResultType
        ) rethrows -> ResultType
    
    As explained in the comments, the existing closure taking method,
    Data.withUnsafeBytes<ResultType, ContentType>, is unsafe for Data initialized
    with bytesNoCopy because it changes the memory's semantic state behind the caller's back.
    
    Adds these methods to copy bytes from data into a buffer:
      public func copyBytes(to buffer: UnsafeMutableRawBufferPointer, count: Int)
      public func copyBytes(to buffer: UnsafeMutableRawBufferPointer, from range: Range<Index>)
    
    Adds this method to copy bytes into data from a buffer:
      public mutating func append(_ buffer: UnsafeRawBufferPointer)

I also suggest that we deprecate this existing API:

  public func withUnsafeBytes<ResultType, ContentType>

And reintroduce it as:

  public func withUnsafePointer<ResultType, ContentType>

It’s only purpose is to make it more convenient to call a C API passing a non-void pointer into Data.

data.withUnsafePointer {
  strlen($0)
}

If the C api takes void* and length, it would actually be safer to use the newly proposed:

data.withUnsafeBytes {
  memcpy(dest, $0.baseAddress, $0.length)
}

because that doesn’t require knowledge of the type of values in memory.

9/9/16, 9:35 PM Andrew Trick:
As mentioned on the review thread, we should also deprecate these:

    public func copyBytes(to pointer: UnsafeMutablePointer<UInt8>, count: Int)
    public func copyBytes(to pointer: UnsafeMutablePointer<UInt8>, from range: Range<Index>)

The reason for deprecating them is that the current versions allow buffer overflow. They do not actually bind memory, so that isn’t the problem.

9/9/16, 9:45 PM Andrew Trick:
This is the method that unnecessarily binds memory to UInt8:

    public func enumerateBytes(_ block: (_ buffer: UnsafeBufferPointer<UInt8>, _ byteIndex: Index, _ stop: inout Bool) -> Void)

We should deprecate it and add a safer overload:

    public func enumerateBytes(_ block: (_ buffer: UnsafeRawBufferPointer, _ byteIndex: Index, _ stop: inout Bool) -> Void)
@atrick
Copy link
Member Author

atrick commented Aug 15, 2018

I want to clarify the most important change proposed in this bug:

I also suggest that we deprecate this existing API:
public func withUnsafeBytes<ResultType, ContentType>

And reintroduce it as:
public func withUnsafePointer<ResultType, ContentType>

Deprecating the existing `withUnsafeBytes` in favor of one that passes
an UnsafeRawBufferPointer to its closure is the most important thing
to change. However, it should not simply be renamed to
`withUnsafePointer`. It should also take a range argument and pass a
buffer pointer to the closure like this:

func withUnsafePointer<Result, ContentType>(
  in range: Range<Int>,
  apply: (UnsafeBufferPointer<ContentType>) throws -> Result
) rethrows -> Result

Additionally, even in its new form, this API is incompatibile with
`bytesNoCopy`. The problem is that `Data` has no knowledge of the
types stored in its memory. It simply references "raw" bytes. If the
client passes existing memory into Data via `bytesNoCopy` then that
memory could presumably be accessed by client code as any type. It
would be quite natural for the user to use `Data.withUnsafePointer` as
an effective pointer cast, but doing so would result in undefined
behavior. For example:

import Foundation

let pointer = UnsafeMutablePointer<Int>.allocate(capacity: 1)
let data = Data(bytesNoCopy: pointer, count: 1, deallocator: .none)
pointer[0] = 3
let val = data.withUnsafePointer {
 (uptr: UnsafeBufferPointer<UInt>) in
 return uptr[0]
}
pointer[0] = 42
print(val) // might print "3" or "42".

If we do still want to provide a `withUnsafePointer` API, then we
should also verify that the Data object has ownership of its memory
and was not constructed using `bytesNoCopy`. Then calling
`withUnsafePointer` could bind Data's memory to the requested
type. That would remain the memory's type until the next call to
`withUnsafePointer`.

@atrick
Copy link
Member Author

atrick commented May 26, 2019

The following bug is more up-to-date:

https://bugs.swift.org/browse/SR-9720

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@shahmishal shahmishal transferred this issue from apple/swift May 5, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Foundation Standard Library
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@atrick