Skip to content
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

[Windows] CFThreadSpecific implementation allows access to deallocated value #2764

Merged
merged 1 commit into from Apr 20, 2020
Merged

[Windows] CFThreadSpecific implementation allows access to deallocated value #2764

merged 1 commit into from Apr 20, 2020

Conversation

lxbndr
Copy link
Contributor

@lxbndr lxbndr commented Apr 11, 2020

Unlike pthread-based implementation, TLS/FLS on Windows doesn't return NULL when reading value after destructor call.

To avoid that we have to nullify value in destructor callback.
The implementation needs to store key along with user data to perform proper cleanup.

This fixes crashes in NotificationQueue and OperationQueue on Windows.

Also introduced small delay in helper func of TestNotificationQueue to avoid false positive/false negative results in test_defaultQueue and test_notificationQueueLifecycle.

@lxbndr
Copy link
Contributor Author

lxbndr commented Apr 11, 2020

CC @compnerd @spevans @millenomi

compnerd
compnerd reviewed Apr 11, 2020
View reviewed changes
CoreFoundation/Base.subproj/CFPlatform.c Outdated
@@ -1416,8 +1434,15 @@ void _CFThreadSpecificSet(_CFThreadSpecificKey key, CFTypeRef _Nullable value) {
#if TARGET_OS_WIN32
if (value != NULL) {
swift_retain((void *)value);

_CFThreadSpecificData *data = malloc(sizeof(_CFThreadSpecificData));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can fail, please ensure that data is valid or abort.

CoreFoundation/Base.subproj/CFPlatform.c Outdated

FlsSetValue(key, data);
} else {
FlsSetValue(key, NULL);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This may cause a leak, and worse a crash - you may have had an entry and you lost it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, this is definitely a leak. Thanks!

I wonder how does it work for Swift value we store here? We are not doing swift_release for previously stored object when overwriting it. But it not leaks, as far as I see.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't resolved. The problem is that you are now injecting a value that you are de-referencing rather than previously which was passed to free or swift_release which accept and ignore NULL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, by bad :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@lxbndr
Copy link
Contributor Author

lxbndr commented Apr 12, 2020

  • done cleanup of old value
  • HALT on malloc fail
    @compnerd

@lxbndr
Copy link
Contributor Author

lxbndr commented Apr 12, 2020 •
edited

As for my question about missing swift_release on value overwrite/clean. Looks like value actually leaks when stored with Thread Specific API.
But there is (almost) no coincidence of this leak, because of specific usage:

  • NotificationQueue uses ThreadSpecific as singleton storage. It not overwrites value once it stored.
  • OperationQueue does manual release in several places, like here: OperationQueue.swift:305. Technically could be overrelease, but it balances Thread Specific retain.
    I guess there is still a danger zone, as Thread Specific destructor callback could try swift_release already released value. But this potential issue is out of scope of this patch.

compnerd
compnerd reviewed Apr 15, 2020
View reviewed changes
CoreFoundation/Base.subproj/CFPlatform.c Outdated
@@ -1415,9 +1433,23 @@ CFTypeRef _Nullable _CFThreadSpecificGet(_CFThreadSpecificKey key) {
void _CFThreadSpecificSet(_CFThreadSpecificKey key, CFTypeRef _Nullable value) {
#if TARGET_OS_WIN32
if (value != NULL) {
_CFThreadSpecificData *oldData = (_CFThreadSpecificData *)FlsGetValue(key);
if (oldData) {
free(oldData);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably should add a comment explaining that the value should not be freed here.

CoreFoundation/Base.subproj/CFPlatform.c Outdated

FlsSetValue(key, data);
} else {
FlsSetValue(key, NULL);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't resolved. The problem is that you are now injecting a value that you are de-referencing rather than previously which was passed to free or swift_release which accept and ignore NULL.

@lxbndr
[Windows] CFThreadSpecific implementation allows access to deallocate…
511f6c9 
…d value

Unlike pthread-based implementation, TLS/FLS on Windows doesn't
return NULL when reading value after destructor call.

To avoid that we have to nullify value in destructor callback.
The implementation needs to store key along with user
data to perform proper cleanup.
@lxbndr lxbndr requested a review from compnerd April 15, 2020 19:16
@compnerd
Copy link
Collaborator

@swift-ci please test Linux platform

@compnerd
Copy link
Collaborator

https://dev.azure.com/compnerd/swift-build/_build/results?buildId=30308&view=results

@compnerd
Copy link
Collaborator

@swift-ci please test Linux platform

@compnerd
Copy link
Collaborator

Please test with following PRs:
apple/swift#31144
@swift-ci please test Linux platform

@compnerd compnerd merged commit 791d57e into apple:master Apr 20, 2020
@lxbndr lxbndr deleted the win-thread-specific branch April 20, 2020 10:50
@lxbndr lxbndr mentioned this pull request May 7, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compnerd compnerd Awaiting requested review from compnerd

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@lxbndr @compnerd