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.

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

Conversation

qwwdfsad
Copy link
Collaborator

@qwwdfsad qwwdfsad commented Dec 7, 2024

Fixes #4293

@qwwdfsad qwwdfsad requested a review from dkhalanskyjb December 7, 2024 18:20
@qwwdfsad
Copy link
Collaborator Author

qwwdfsad commented Dec 7, 2024

@qwwdfsad
Copy link
Collaborator Author

qwwdfsad commented Dec 7, 2024

In general, I cannot say that "trust me, this is safe" is, well, actually safe.

The problem with serialization is that it's impossible to reason about it in the bigger picture. We have our invariants, they are inherently local to the execution, and serializing-deserializing some of the things basically erases these invariants.
Even with manual readResolve, it still does not seem to be fixable semantically.

TL;DR Java serialization is bad, (see also: https://www.youtube.com/watch?v=fbqAyRJoQO0 by Java Architects), in other news -- water is wet

Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb left a comment

Choose a reason for hiding this comment

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

I have one minor remark about the contract of AbortFlowException, but I don't think it's a realistic concern; it's mostly a question of purity that can be ignored.

@@ -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

@qwwdfsad qwwdfsad merged commit 9f6c80b into develop Dec 11, 2024
1 check passed
@qwwdfsad qwwdfsad deleted the fixup-transient branch December 11, 2024 13:13
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.

2 participants