Skip to content

[stdlib] Avoid materializing strong references in potential dangling unmanaged opaque functions #70945

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 5 commits into from
Jan 17, 2024

Conversation

Azoy
Copy link
Contributor

@Azoy Azoy commented Jan 16, 2024

The current implementations of toOpaque and fromOpaque accidentally/incorrectly gets hold of temporary strong references to Instance which will emit retain/releases from the compiler in certain configurations. These functions should not induce any ARC traffic whatsoever. Bit cast self to and from the pointer value instead to avoid this.

Resolves: rdar://120849792

@Azoy Azoy requested a review from a team as a code owner January 16, 2024 23:20
Azoy and others added 2 commits January 16, 2024 15:57
Co-authored-by: Karoy Lorentey <[email protected]>
Co-authored-by: Karoy Lorentey <[email protected]>
Co-authored-by: Karoy Lorentey <[email protected]>
@Azoy
Copy link
Contributor Author

Azoy commented Jan 17, 2024

@swift-ci please test

@Azoy
Copy link
Contributor Author

Azoy commented Jan 17, 2024

@swift-ci please test

Co-authored-by: Guillaume Lessard <[email protected]>

fix test
@Azoy Azoy force-pushed the fix-unmanaged-opaque branch from 14caaab to f0a74ec Compare January 17, 2024 01:14
@Azoy
Copy link
Contributor Author

Azoy commented Jan 17, 2024

@swift-ci please test

@stephentyrone
Copy link
Contributor

Is init(_private: Instance) now unused? Can you add a comment that it is needed for ABI?

// pointer. This test's expectEqual is kind of bogus, we're just checking that
// it doesn't crash.

// Create a dangling pointer, usually to unmapped memory.
Copy link
Contributor

Choose a reason for hiding this comment

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

usually 😂

@Azoy
Copy link
Contributor Author

Azoy commented Jan 17, 2024

That initializer is still used in the functions that go from strong ref -> unmanaged.

@Azoy Azoy merged commit 72ad6c5 into swiftlang:main Jan 17, 2024
@Azoy Azoy deleted the fix-unmanaged-opaque branch January 17, 2024 17:30
Azoy added a commit to Azoy/swift that referenced this pull request Jan 17, 2024
[stdlib] Avoid materializing strong references in potential dangling unmanaged opaque functions
stephentyrone pushed a commit that referenced this pull request Jan 18, 2024
[stdlib] Avoid materializing strong references in potential dangling unmanaged opaque functions
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