Skip to content

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.


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