Skip to content

don't hold references to unowned DispatchData objects (SR-3628) #200

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
Jan 20, 2017

Conversation

weissi
Copy link
Contributor

@weissi weissi commented Jan 16, 2017

In DispatchIO.read, the dispatch_data_t object received from dispatch_io_read will be released once the block passed to dispatch_io_read returns.

In the Swift overlay (src/swift/IO.swift) the code marks that data object correctly as "borrowedData"

public func read(offset: off_t, length: Int, queue: DispatchQueue, ioHandler: @escaping (_ done: Bool, _ data: DispatchData?, _ error: Int32) -> Void) {
	dispatch_io_read(self.__wrapped, offset, length, queue.__wrapped) { (done: Bool, data: dispatch_data_t?, error: Int32) in
		ioHandler(done, data.flatMap { DispatchData(borrowedData: $0) }, error)
	}
}

that however leads to use-after-frees if the Swift DispatchData is kept around for longer.

The change I made will retain the underlying dispatch_data_t if it's "borrowed" which I believe is correct and fixes SR-3628.

@dgrove-oss
Copy link
Contributor

borrowedData was added to Data to compensate for memory leaks that I think were related to iteration. I will go back and look at that PR / SR to see how this proposed change interacts with that.

@dgrove-oss
Copy link
Contributor

dgrove-oss commented Jan 16, 2017

ok. This change makes sense to me.

When I "fixed" the over-release problem of SR-2656 by introducing borrowedData: in #175 I didn't account for when the __DispatchData that was borrowing the dispatch_data_t outlived the "real" owner. I was too focused on trying to avoid the overhead of an "extra" retain/release operation. But, it really can't be avoided (as shown by the test case for this bug).

@@ -188,21 +188,20 @@ extension DispatchSource : DispatchSourceProcess,

internal class __DispatchData : DispatchObject {
internal let __wrapped:dispatch_data_t
internal let __owned:Bool

final internal override func wrapped() -> dispatch_object_t {
return unsafeBitCast(__wrapped, to: dispatch_object_t.self)
}

internal init(data:dispatch_data_t, owned:Bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

that owned tag should really be transferOwnership: Bool but the patch LGTM and I should have realized when we reviewed the earlier patch :/

@MadCoder
Copy link
Contributor

@swift-ci please test

@weissi
Copy link
Contributor Author

weissi commented Jan 17, 2017

build seems healthy to me but then failed because of

Build timed out (after 166 minutes). Marking the build as failed.

@weissi
Copy link
Contributor Author

weissi commented Jan 17, 2017

@swift-ci please test

@weissi
Copy link
Contributor Author

weissi commented Jan 17, 2017

@MadCoder are there any special permissions required to '@swift-ci please this'?

@MadCoder MadCoder merged commit 44c2291 into swiftlang:master Jan 20, 2017
@MadCoder
Copy link
Contributor

@dgrove-oss I'm not 100% sure, do we need this for another branch than master? we want that for whatever the next swift 3 update will be.

@dgrove-oss
Copy link
Contributor

We should get it into the swift-3.1-branch. I don't think it is worth putting into the swift-3.0-branch.
@weissi can you easily do the PR to the swift-3.1 branch or would you like me to take care of it?

@weissi
Copy link
Contributor Author

weissi commented Jan 20, 2017

@dgrove-oss sure, on it!

@weissi
Copy link
Contributor Author

weissi commented Jan 20, 2017

@dgrove-oss #201

das pushed a commit that referenced this pull request Feb 21, 2017
don't hold references to unowned DispatchData objects (SR-3628)

Signed-off-by: Daniel A. Steffen <dsteffen@apple.com>
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