Skip to content

Fix for SR-6849 and SR-6962 leak in NSData and NSMutableData #1455

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
Feb 23, 2018

Conversation

mamabusi
Copy link
Contributor

Test case:

import Foundation

for _ in 0..<10 {
        let data = Data(base64Encoded: "UE9TVCAvIEhUVFAvMS4wDQpDb250ZW50LUxlbmd0aDoNCg0K")!
}

Valgrind memcheck tool output before the fix (as reported in SR-6849):

==7078== LEAK SUMMARY:
==7078==    definitely lost: 360 bytes in 10 blocks

Valgrind memcheck tool output after the fix:

==20265== LEAK SUMMARY:
==20265==    definitely lost: 0 bytes in 0 blocks
==20265==    indirectly lost: 0 bytes in 0 blocks
==20265==      possibly lost: 0 bytes in 0 blocks

Summary:
This is a regression from the df3ec55 commit for the High Sierra changes.

Previously, PR: #380 had a fix for NSData leak where __CFDataGetInfoBit for __kCFDontDeallocate (then equivalent of __CFDataDontDeallocate before PR for High Sierra changes) was set on an ‘if’ condition in _CFDataInit method and it was in turn checked in the __CFDataDeallocate method of CFData during deallocation.

However, with the HighSierra PR 1338 changes now, the value for __CFDataDontDeallocate is hardcoded to 'true'
__CFDataSetDontDeallocate(memory, true);
With this, data is never deallocated and hence the leak.

@@ -69,11 +69,10 @@ private final class _NSDataDeallocator {

private let __kCFMutable: CFOptionFlags = 0x01
private let __kCFGrowable: CFOptionFlags = 0x02
private let __kCFMutableVarietyMask: CFOptionFlags = 0x03
Copy link
Contributor Author

@mamabusi mamabusi Feb 23, 2018

Choose a reason for hiding this comment

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

__kCFMutableVarietyMask is not used anymore.

private let __kCFBytesInline: CFOptionFlags = 0x04
private let __kCFUseAllocator: CFOptionFlags = 0x08
private let __kCFDontDeallocate: CFOptionFlags = 0x10
private let __kCFAllocatesCollectable: CFOptionFlags = 0x20
Copy link
Contributor Author

Choose a reason for hiding this comment

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

__kCFAllocatesCollectable is not used anymore.


private let __kCFBytesInline: CFOptionFlags = 2
private let __kCFUseAllocator: CFOptionFlags = 3
private let __kCFDontDeallocate: CFOptionFlags = 4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed for bitmasks to integers on CoreFoundation's CFData.c

@spevans
Copy link
Contributor

spevans commented Feb 23, 2018

@swift-ci please test

@mamabusi
Copy link
Contributor Author

@djones6 tested this fix for SR-6962 and observes the leak is not happening.

@parkera parkera self-requested a review February 23, 2018 18:12
@parkera
Copy link
Contributor

parkera commented Feb 23, 2018

Thank you @mamabusi. I have a feeling this is one of those changes that looks simple but took forever to track down.

@parkera
Copy link
Contributor

parkera commented Feb 23, 2018

Approved for 4.1 branch as well.

@spevans
Copy link
Contributor

spevans commented Feb 23, 2018

@swift-ci please test and merge

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