From 8ad012ae32955c151dd65a79dbcde7c04dadecf3 Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Thu, 31 Aug 2023 18:08:09 +0200 Subject: [PATCH 1/4] Mark BufferedChannelIterator.continuation @BeningDataRace to address potential UB and OoTA on K/N Fixes #3834 --- .../common/src/channels/BufferedChannel.kt | 1 + .../common/src/internal/Concurrent.common.kt | 12 ++++++++++++ .../native/src/internal/Concurrent.kt | 2 ++ 3 files changed, 15 insertions(+) diff --git a/kotlinx-coroutines-core/common/src/channels/BufferedChannel.kt b/kotlinx-coroutines-core/common/src/channels/BufferedChannel.kt index 08cda7e9e8..a54284271d 100644 --- a/kotlinx-coroutines-core/common/src/channels/BufferedChannel.kt +++ b/kotlinx-coroutines-core/common/src/channels/BufferedChannel.kt @@ -1583,6 +1583,7 @@ internal open class BufferedChannel( * continuation. The [tryResumeHasNext] and [tryResumeHasNextOnClosedChannel] * function resume this continuation when the [hasNext] invocation should complete. */ + @BenignDataRace // See its uses for the explanation of the race private var continuation: CancellableContinuationImpl? = null // `hasNext()` is just a special receive operation. diff --git a/kotlinx-coroutines-core/common/src/internal/Concurrent.common.kt b/kotlinx-coroutines-core/common/src/internal/Concurrent.common.kt index 848a42c867..c06fcaf7b2 100644 --- a/kotlinx-coroutines-core/common/src/internal/Concurrent.common.kt +++ b/kotlinx-coroutines-core/common/src/internal/Concurrent.common.kt @@ -12,3 +12,15 @@ internal expect class ReentrantLock() { internal expect inline fun ReentrantLock.withLock(action: () -> T): T internal expect fun identitySet(expectedSize: Int): MutableSet + +/** + * Annotation indicating that the marked property is the subject of benign data race. + * LLVM does not support this notion, so on K/N platforms we alias it into `@Volatile` to prevent potential OoTA. + * + * The purpose of this annotation is not to save an extra-volatile on JVM platform, but rather to explicitly emphasize + * that data-race is benign. + */ +@OptionalExpectation +@Target(AnnotationTarget.FIELD) +@OptIn(ExperimentalMultiplatform::class) +internal expect annotation class BenignDataRace() diff --git a/kotlinx-coroutines-core/native/src/internal/Concurrent.kt b/kotlinx-coroutines-core/native/src/internal/Concurrent.kt index 17975e2e7f..f46326bcda 100644 --- a/kotlinx-coroutines-core/native/src/internal/Concurrent.kt +++ b/kotlinx-coroutines-core/native/src/internal/Concurrent.kt @@ -29,3 +29,5 @@ internal open class SuppressSupportingThrowableImpl : Throwable() { } } +@Suppress("ACTUAL_WITHOUT_EXPECT") // This suppress can be removed in 2.0: KT-59355 +internal actual typealias BenignDataRace = kotlin.concurrent.Volatile From 3e5acfdc4e16f1c50daa409f64e1de465916062e Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Thu, 31 Aug 2023 19:06:04 +0200 Subject: [PATCH 2/4] Explain benign data-race on SelectImplementation.clauses and mark it with @BenignDataRace Fixes #3843 --- kotlinx-coroutines-core/common/src/selects/Select.kt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/kotlinx-coroutines-core/common/src/selects/Select.kt b/kotlinx-coroutines-core/common/src/selects/Select.kt index 3ac3cb6f27..1f91515f0d 100644 --- a/kotlinx-coroutines-core/common/src/selects/Select.kt +++ b/kotlinx-coroutines-core/common/src/selects/Select.kt @@ -372,7 +372,12 @@ internal open class SelectImplementation( /** * List of clauses waiting on this `select` instance. + * + * This property is the subject to bening data-race: concurrent cancellation might null-out this property + * while [trySelect] operation reads it and iterates over its content. + * A logical race is resolved by the consensus on [state] property. */ + @BenignDataRace private var clauses: MutableList? = ArrayList(2) /** From 6d91be880fef4131412e3eecfafa55f746f9a774 Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Thu, 31 Aug 2023 19:23:35 +0200 Subject: [PATCH 3/4] Explain benign data-race on SelectImplementation.internalResult and mark it with @BenignDataRace Fixes #3843 --- kotlinx-coroutines-core/common/src/selects/Select.kt | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/kotlinx-coroutines-core/common/src/selects/Select.kt b/kotlinx-coroutines-core/common/src/selects/Select.kt index 1f91515f0d..4bbb314a67 100644 --- a/kotlinx-coroutines-core/common/src/selects/Select.kt +++ b/kotlinx-coroutines-core/common/src/selects/Select.kt @@ -413,6 +413,7 @@ internal open class SelectImplementation( * or [DisposableHandle] instance when the clause is completed during registration ([inRegistrationPhase] is `false`). * Yet, this optimization is omitted for code simplicity. */ + @BenignDataRace // See its cleanup phase for the explanation private var internalResult: Any? = NO_RESULT /** @@ -626,9 +627,11 @@ internal open class SelectImplementation( // try to resume the continuation. this.internalResult = internalResult if (cont.tryResume(onCancellation)) return TRY_SELECT_SUCCESSFUL - // If the resumption failed, we need to clean - // the [result] field to avoid memory leaks. - this.internalResult = null + /* + * If the resumption failed, we need to clean the [result] field to avoid memory leaks. + * This write is benignly races with the very same write in cancellation invoke() handler + */ + this.internalResult = NO_RESULT return TRY_SELECT_CANCELLED } } From be2aa9f07da315d25993be74d7a174c9c897efc6 Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Fri, 20 Oct 2023 18:14:23 +0200 Subject: [PATCH 4/4] ~move benign race explanation to declaration site --- .../common/src/channels/BufferedChannel.kt | 6 +++++- .../common/src/selects/Select.kt | 14 ++++++++------ 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/kotlinx-coroutines-core/common/src/channels/BufferedChannel.kt b/kotlinx-coroutines-core/common/src/channels/BufferedChannel.kt index a54284271d..9224ae8fce 100644 --- a/kotlinx-coroutines-core/common/src/channels/BufferedChannel.kt +++ b/kotlinx-coroutines-core/common/src/channels/BufferedChannel.kt @@ -1582,8 +1582,12 @@ internal open class BufferedChannel( * When [hasNext] suspends, this field stores the corresponding * continuation. The [tryResumeHasNext] and [tryResumeHasNextOnClosedChannel] * function resume this continuation when the [hasNext] invocation should complete. + * + * This property is the subject to bening data race: + * It is nulled-out on both completion and cancellation paths that + * could happen concurrently. */ - @BenignDataRace // See its uses for the explanation of the race + @BenignDataRace private var continuation: CancellableContinuationImpl? = null // `hasNext()` is just a special receive operation. diff --git a/kotlinx-coroutines-core/common/src/selects/Select.kt b/kotlinx-coroutines-core/common/src/selects/Select.kt index 4bbb314a67..79acdbd1c2 100644 --- a/kotlinx-coroutines-core/common/src/selects/Select.kt +++ b/kotlinx-coroutines-core/common/src/selects/Select.kt @@ -373,7 +373,7 @@ internal open class SelectImplementation( /** * List of clauses waiting on this `select` instance. * - * This property is the subject to bening data-race: concurrent cancellation might null-out this property + * This property is the subject to bening data race: concurrent cancellation might null-out this property * while [trySelect] operation reads it and iterates over its content. * A logical race is resolved by the consensus on [state] property. */ @@ -412,8 +412,13 @@ internal open class SelectImplementation( * one that stores either result when the clause is successfully registered ([inRegistrationPhase] is `true`), * or [DisposableHandle] instance when the clause is completed during registration ([inRegistrationPhase] is `false`). * Yet, this optimization is omitted for code simplicity. + * + * This property is the subject to benign data race: + * [Cleanup][cleanup] procedure can be invoked both as part of the completion sequence + * and as a cancellation handler triggered by an external cancellation. + * In both scenarios, [NO_RESULT] is written to this property via race. */ - @BenignDataRace // See its cleanup phase for the explanation + @BenignDataRace private var internalResult: Any? = NO_RESULT /** @@ -627,10 +632,7 @@ internal open class SelectImplementation( // try to resume the continuation. this.internalResult = internalResult if (cont.tryResume(onCancellation)) return TRY_SELECT_SUCCESSFUL - /* - * If the resumption failed, we need to clean the [result] field to avoid memory leaks. - * This write is benignly races with the very same write in cancellation invoke() handler - */ + // If the resumption failed, we need to clean the [result] field to avoid memory leaks. this.internalResult = NO_RESULT return TRY_SELECT_CANCELLED }