Skip to content

Commit 664d24f

Browse files
authored
Avoid unsafeBitCast() in the new interop target. (#1384)
This PR uses `Unmanaged` to avoid calling `unsafeBitCast()` in the new `_TestingInterop` target on platforms that can use `Atomic`. This improves type safety (yay!) ### Checklist: - [x] Code and documentation should follow the style of the [Style Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md). - [x] If public symbols are renamed or modified, DocC references should be updated.
1 parent b1789fd commit 664d24f

File tree

2 files changed

+56
-10
lines changed

2 files changed

+56
-10
lines changed

Package.swift

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,17 @@ let package = Package(
105105
)
106106
)
107107

108+
#if DEBUG
109+
// Build _TestingInterop for debugging/testing purposes only. It is
110+
// important that clients do not link to this product/target.
111+
result += [
112+
.library(
113+
name: "_TestingInterop_DO_NOT_USE",
114+
targets: ["_TestingInterop_DO_NOT_USE"]
115+
)
116+
]
117+
#endif
118+
108119
return result
109120
}(),
110121

@@ -209,6 +220,16 @@ let package = Package(
209220
cxxSettings: .packageSettings,
210221
swiftSettings: .packageSettings + .enableLibraryEvolution()
211222
),
223+
.target(
224+
// Build _TestingInterop for debugging/testing purposes only. It is
225+
// important that clients do not link to this product/target.
226+
name: "_TestingInterop_DO_NOT_USE",
227+
dependencies: ["_TestingInternals",],
228+
path: "Sources/_TestingInterop",
229+
exclude: ["CMakeLists.txt"],
230+
cxxSettings: .packageSettings,
231+
swiftSettings: .packageSettings
232+
),
212233

213234
// Cross-import overlays (not supported by Swift Package Manager)
214235
.target(

Sources/_TestingInterop/FallbackEventHandler.swift

Lines changed: 35 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,13 @@
99
//
1010

1111
#if !SWT_NO_INTEROP
12-
#if SWT_TARGET_OS_APPLE && !SWT_NO_OS_UNFAIR_LOCK
12+
#if SWT_TARGET_OS_APPLE && !SWT_NO_OS_UNFAIR_LOCK && !hasFeature(Embedded)
1313
private import _TestingInternals
1414
#else
1515
private import Synchronization
1616
#endif
1717

18-
#if SWT_TARGET_OS_APPLE && !SWT_NO_OS_UNFAIR_LOCK
18+
#if SWT_TARGET_OS_APPLE && !SWT_NO_OS_UNFAIR_LOCK && !hasFeature(Embedded)
1919
/// The installed event handler.
2020
private nonisolated(unsafe) let _fallbackEventHandler = {
2121
let result = ManagedBuffer<FallbackEventHandler?, os_unfair_lock>.create(
@@ -26,8 +26,17 @@ private nonisolated(unsafe) let _fallbackEventHandler = {
2626
return result
2727
}()
2828
#else
29+
/// `Atomic`-compatible storage for ``FallbackEventHandler``.
30+
private final class _FallbackEventHandlerStorage: Sendable, RawRepresentable {
31+
let rawValue: FallbackEventHandler
32+
33+
init(rawValue: FallbackEventHandler) {
34+
self.rawValue = rawValue
35+
}
36+
}
37+
2938
/// The installed event handler.
30-
private nonisolated(unsafe) let _fallbackEventHandler = Atomic<UnsafeRawPointer?>(nil)
39+
private let _fallbackEventHandler = Atomic<Unmanaged<_FallbackEventHandlerStorage>?>(nil)
3140
#endif
3241

3342
/// A type describing a fallback event handler that testing API can invoke as an
@@ -58,7 +67,7 @@ package typealias FallbackEventHandler = @Sendable @convention(c) (
5867
@_cdecl("_swift_testing_getFallbackEventHandler")
5968
@usableFromInline
6069
package func _swift_testing_getFallbackEventHandler() -> FallbackEventHandler? {
61-
#if SWT_TARGET_OS_APPLE && !SWT_NO_OS_UNFAIR_LOCK
70+
#if SWT_TARGET_OS_APPLE && !SWT_NO_OS_UNFAIR_LOCK && !hasFeature(Embedded)
6271
return _fallbackEventHandler.withUnsafeMutablePointers { fallbackEventHandler, lock in
6372
os_unfair_lock_lock(lock)
6473
defer {
@@ -67,8 +76,14 @@ package func _swift_testing_getFallbackEventHandler() -> FallbackEventHandler? {
6776
return fallbackEventHandler.pointee
6877
}
6978
#else
70-
return _fallbackEventHandler.load(ordering: .sequentiallyConsistent).flatMap { fallbackEventHandler in
71-
unsafeBitCast(fallbackEventHandler, to: FallbackEventHandler?.self)
79+
// If we had a setter, this load would present a race condition because
80+
// another thread could store a new value in between the load and the call to
81+
// `takeUnretainedValue()`, resulting in a use-after-free on this thread. We
82+
// would need a full lock in order to avoid that problem. However, because we
83+
// instead have a one-time installation function, we can be sure that the
84+
// loaded value (if non-nil) will never be replaced with another value.
85+
return _fallbackEventHandler.load(ordering: .sequentiallyConsistent).map { fallbackEventHandler in
86+
fallbackEventHandler.takeUnretainedValue().rawValue
7287
}
7388
#endif
7489
}
@@ -86,8 +101,10 @@ package func _swift_testing_getFallbackEventHandler() -> FallbackEventHandler? {
86101
@_cdecl("_swift_testing_installFallbackEventHandler")
87102
@usableFromInline
88103
package func _swift_testing_installFallbackEventHandler(_ handler: FallbackEventHandler) -> CBool {
89-
#if SWT_TARGET_OS_APPLE && !SWT_NO_OS_UNFAIR_LOCK
90-
return _fallbackEventHandler.withUnsafeMutablePointers { fallbackEventHandler, lock in
104+
var result = false
105+
106+
#if SWT_TARGET_OS_APPLE && !SWT_NO_OS_UNFAIR_LOCK && !hasFeature(Embedded)
107+
result = _fallbackEventHandler.withUnsafeMutablePointers { fallbackEventHandler, lock in
91108
os_unfair_lock_lock(lock)
92109
defer {
93110
os_unfair_lock_unlock(lock)
@@ -99,8 +116,16 @@ package func _swift_testing_installFallbackEventHandler(_ handler: FallbackEvent
99116
return true
100117
}
101118
#else
102-
let handler = unsafeBitCast(handler, to: UnsafeRawPointer.self)
103-
return _fallbackEventHandler.compareExchange(expected: nil, desired: handler, ordering: .sequentiallyConsistent).exchanged
119+
let handler = Unmanaged.passRetained(_FallbackEventHandlerStorage(rawValue: handler))
120+
defer {
121+
if !result {
122+
handler.release()
123+
}
124+
}
125+
126+
result = _fallbackEventHandler.compareExchange(expected: nil, desired: handler, ordering: .sequentiallyConsistent).exchanged
104127
#endif
128+
129+
return result
105130
}
106131
#endif

0 commit comments

Comments
 (0)