Skip to content

Fix a memory leak in DispatchData.withUnsafeBytes #850

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

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Oct 29, 2024

DispatchData.withUnsafeBytes created a new dispatch_data_t by calling dispatch_data_create_map. I assume that the intention was that this memory was freed when data is destroyed, based on the presence of _fixLifetime(data) but data was just a plain dispatch_data_t C struct, that doesn’t have any cleanup operations associated with it when destroyed.

To fix the leak, wrap the dispatch_data_t in a DispatchData, which takes over the ownership of the dispatch_data_t and releases it when data gets destroyed.

Alternatively, _fixLifetime could have been replaced by _swift_dispatch_release(unsafeBitCast(data, to: dispatch_object_t.self)) but I think using DispatchData is the cleaner solution.


For future reference, a minimal reproducer for this issue was

import Dispatch
import Foundation

func run() {
  let queue = DispatchQueue(label: "jsonrpc-queue", qos: .userInitiated)

  let receiveIO = DispatchIO(
    type: .stream,
    fileDescriptor: FileHandle.standardInput.fileDescriptor,
    queue: queue,
    cleanupHandler: { _ in }
  )

  receiveIO.setLimit(lowWater: 1)
  receiveIO.setLimit(highWater: Int.max)

  receiveIO.read(offset: 0, length: Int.max, queue: queue) { done, data, errorCode in
    guard let data, !data.isEmpty else {
      return
    }

    data.withUnsafeBytes { (pointer: UnsafePointer<UInt8>) in }
  }
}

try await Task.sleep(for: .seconds(2))
print("start")
run()
try await Task.sleep(for: .seconds(10))

Then run it as

seq 1 100000000 | .build/debug/repro

and attach heaptrack to it during the initial 2s waiting period by running

heaptrack --pid -$(pidof repro)

Heaptrack will list multiple GB worth of leaks without this fix.

`DispatchData.withUnsafeBytes` created a new `dispatch_data_t` by calling `dispatch_data_create_map`. I assume that the intention was that this memory was freed when `data` is destroyed, based on the presence of `_fixLifetime(data)` but `data` was just a plain `dispatch_data_t` C struct, that doesn’t have any cleanup operations associated with it when destroyed.

To fix the leak, wrap the `dispatch_data_t` in a `DispatchData`, which takes over the ownership of the `dispatch_data_t` and releases it when `data` gets destroyed.

Alternatively, `_fixLifetime` could have been replaced by `_swift_dispatch_release(unsafeBitCast(data, to: dispatch_object_t.self))` but I think using `DispatchData` is the cleaner solution.
@ahoppen
Copy link
Member Author

ahoppen commented Oct 29, 2024

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Oct 29, 2024

@swift-ci please test

@ahoppen
Copy link
Member Author

ahoppen commented Oct 29, 2024

@swift-ci please test

@azharudd
Copy link

@swift-ci Please test Linux platform

@shahmishal
Copy link
Member

@swift-ci please test

@ahoppen ahoppen merged commit 0d67182 into swiftlang:main Oct 30, 2024
2 checks passed
@ahoppen ahoppen deleted the data-leak branch October 30, 2024 21:11
@jessezamora
Copy link

Will this be included in 6.1? Or does is need to be cherry picked into the release/6.1 branch now?

@ahoppen
Copy link
Member Author

ahoppen commented Oct 30, 2024

6.1 has not branched yet, so this will be in Swift 6.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants