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

Revisit all @Transient usages in coroutines #4294

Merged
merged 1 commit into from
Dec 11, 2024
Merged

Revisit all @Transient usages in coroutines #4294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion kotlinx-coroutines-core/jvm/src/Exceptions.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,15 @@ public actual fun CancellationException(message: String?, cause: Throwable?) : C
internal actual class JobCancellationException public actual constructor(
message: String,
cause: Throwable?,
@JvmField @Transient internal actual val job: Job
job: Job
) : CancellationException(message), CopyableThrowable<JobCancellationException> {

@Transient
private val _job: Job? = job

// The safest option for transient -- return something that meanigfully reject any attemp to interact with the job
internal actual val job get() = _job ?: NonCancellable

init {
if (cause != null) initCause(cause)
}
Expand Down
4 changes: 4 additions & 0 deletions kotlinx-coroutines-core/jvm/src/flow/internal/FlowExceptions.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ package kotlinx.coroutines.flow.internal

import kotlinx.coroutines.*

/**
* Implementation note: `owner` is an internal marked that is used ONLY for identity checks by coroutines machinery,
* and it's never exposed, thus it's safe to have it both `@Transient` and non-nullable.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems possible to maliciously deserialize and use an AbortFlowException that would fail in ways that legal instances wouldn't fail. I'm not sure why not just mark owner as nullable: I don't think anything breaks if we have @JvmField @Transient val owner: Any? = owner where a constructor only accepts a non-nullable version. Why not do that instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought about that as well and decided not to do it: our general contract is that owner should be unique, which nullable type will immediately defeat -- and also will open the door for us to introduce new bugs (also, additionally cognitive load on handling nulls).

All of that, for the sake of the really malicious actor that probably can work around their ways anyway

Copy link
Collaborator

Choose a reason for hiding this comment

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

also will open the door for us to introduce new bugs

I don't see the risk if

a constructor only accepts a non-nullable version

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's harded to follow in the code itself, the concept of "potential malicious actor" starts poisoning the signatures and makes readers/operators authors/etc. to read through that

*/
internal actual class AbortFlowException actual constructor(
@JvmField @Transient actual val owner: Any
) : CancellationException("Flow was aborted, no more elements needed") {
Expand Down
6 changes: 5 additions & 1 deletion kotlinx-coroutines-core/jvm/src/internal/CoroutineExceptionHandlerImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,11 @@ internal actual fun propagateExceptionFinalResort(exception: Throwable) {
}

// This implementation doesn't store a stacktrace, which is good because a stacktrace doesn't make sense for this.
internal actual class DiagnosticCoroutineContextException actual constructor(@Transient private val context: CoroutineContext) : RuntimeException() {
internal actual class DiagnosticCoroutineContextException actual constructor(context: CoroutineContext) : RuntimeException() {

@Transient
private val context: CoroutineContext? = context

override fun getLocalizedMessage(): String {
return context.toString()
}
Expand Down