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

Add Insufficient covariance check makes self_cell unsound #1818

Merged
merged 1 commit into from
Nov 11, 2023

Conversation

Voultapher
Copy link
Contributor

No description provided.

@Voultapher
Copy link
Contributor Author

@steffahn thoughts?

Copy link
Member

@Shnatsel Shnatsel left a comment

Choose a reason for hiding this comment

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

Looks good, I'm happy to merge as soon as @steffahn approves. Thanks!

@steffahn
Copy link

There's a discussion to be had, whether a fixed 0.10.3 version (by means of re-exporting version 1.* will also be published, in which case the patched = [">= 1.0.2"] would no longer be entirely accurate. Does RustSec support something like “everything <= 0.10.2, as well as 1.0.0 <= x <= 1.0.1 is affected, but 0.10.3, and >= 1.0.2 are patched” in the first place?

@tarcieri
Copy link
Member

Yes, that would look like:

affected = ["<= 0.10.2", ">1.0.0, <= 1.0.1"]
patched = ["0.10.3", ">= 1.0.2"]

@steffahn
Copy link

The description is okay, though it reads a bit as if the covariance check used to not work at all. Maybe the sentence

This allowed users to mark a dependent as covariant even though it allows interior-mutability.

could become

This allowed users to mark a dependent as covariant even though its type was not covariant but invariant, for certain invariant types involving trait object lifetimes.

also removing the mention of “interior-mutability” which is hard to understand in this context, anyways.

@steffahn
Copy link

steffahn commented Nov 10, 2023

Yes, that would look like:

affected = ["<= 0.10.2", ">1.0.0, <= 1.0.1"]
patched = ["0.10.3", ">= 1.0.2"]

Does ">1.0.0, <= 1.0.1" exactly correspond to “1.0.0 <= x <= 1.0.1”, i.e including version 1.0.0 still as “affected”? Or should that be

affected = ["<= 0.10.2", ">= 1.0.0, <= 1.0.1"]
patched = ["0.10.3", ">= 1.0.2"]

Perhaps also, to mention fewer distinct boundaries:

affected = ["< 0.10.3", ">= 1.0.0, < 1.0.2"]
patched = [">= 0.10.3, < 1.0.0", ">= 1.0.2"]

Edit: (Also see my next comment below…) affected is not a thing, so it looks like

patched = [">= 0.10.3, < 1.0.0", ">= 1.0.2"]

@tarcieri
Copy link
Member

The latter looks good, yeah

@steffahn
Copy link

Looks like there was a confusion in the advice here. An affected key does not exists, and would be redundant anyways. An unaffected key would exist for earlier versions that are not affected yet. I don’t believe any earlier version didn’t have the issue. @Voultapher you just need to remove the affected line, it seems ^^

@steffahn
Copy link

@Shnatsel, this should be good to merge now. The 0.10.3 version question was resolved (it’s already published), and this advisory’s information updated accordingly.

@Shnatsel Shnatsel merged commit 0c128ba into rustsec:main Nov 11, 2023
1 check passed
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