Skip to content

Foundation: FileHandle.swift: memory leak in _readDataOfLength #1155

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

Jump to bottom
Merged
merged 1 commit into from
Aug 11, 2017
Merged

Foundation: FileHandle.swift: memory leak in _readDataOfLength #1155

merged 1 commit into from
Aug 11, 2017

Conversation

mattrajca
Copy link
Contributor

dynamicBuffer will never be freed if total > 0. Fix this by giving the underlying Data a "free" destructor. This is covered by existing tests.

@ianpartridge
Copy link
Contributor

The line in question was added in 188a4e1 so tagging @phausler.

parkera
parkera reviewed Aug 3, 2017
View reviewed changes
Foundation/FileHandle.swift
@@ -109,7 +109,7 @@ open class FileHandle : NSObject, NSSecureCoding {

Copy link
Contributor

Choose a reason for hiding this comment

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

For clarity, can we change this:

if (0 == total) { } if (total > 0) { } to an if-else?

Just so we make sure we can't exit this function without either a free or data creation with .free.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The earlier code paths ensure that the total will never be negative. Regardless, I updated this to cover all cases and added an assertion in the unreachable case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@parkera Please review the updated commit.

@mattrajca
Foundation: FileHandle.swift: memory leak in _readDataOfLength
adcbaf3 
`dynamicBuffer` will never be freed if `total > 0`. Fix this by
giving the underlying Data a "free" destructor. This is covered
by existing tests.
@ianpartridge
Copy link
Contributor

@swift-ci please test and merge

1 similar comment
@alblue
Copy link
Contributor

alblue commented Aug 8, 2017

@swift-ci please test and merge

@mattrajca
Copy link
Contributor Author

@ianpartridge @alblue Hmm, seems like there were some server errors: Server returned HTTP response code: 405, message: 'Method Not Allowed' for URL: https://api.github.com/repos/apple/swift-corelibs-foundation/pulls/1155/merge

@ianpartridge ianpartridge reopened this Aug 10, 2017
@ianpartridge
Copy link
Contributor

@swift-ci please test and merge

1 similar comment
@ianpartridge
Copy link
Contributor

@swift-ci please test and merge

@swift-ci swift-ci merged commit cab40e7 into swiftlang:master Aug 11, 2017
@swift-ci swift-ci mentioned this pull request Aug 12, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parkera parkera parkera approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mattrajca @ianpartridge @alblue @parkera @swift-ci