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

Fix: Object::from_ref no longer forgetting to increment the strong count #5858

Merged
merged 3 commits into from
Oct 24, 2024

Conversation

MikaelMayer
Copy link
Member

@MikaelMayer MikaelMayer commented Oct 24, 2024

Fixes #5851
This PR adds the following safety checks

  • If the strong count is not updated, we will panic instead of silently returning a Rc pointer that would cause dangling pointers if dropped. Adding these checks helped me reproduce the soundness issue all the time locally, but obviously did not fix it. I'm keeping it there just in case.
  • Itmakes the strong count increment opaque to the compiler, so that it should prevent optimizations that would skip the strong count from increasing. It also makes it opaque at the call site. This did not prevent the compiler from optimizing it away unfortunately in at least one experiment, but I'll keep it just in case it's was another lurking issue.
  • Added #[inline(never)] to the function from_ref did fix the soundness issue.
  • I tried the following but it did not do anything, so not implementing: Instead of incrementing the strong count from the pointer obtained by &T, it increments the strong count from the pointer obtained by the extracted Rc<T> by doing it the normal way

An alternative to avoid all this hassle would be to pass self: &Rc<Self> instead of self: &Self, which is what the branch https://github.com/dafny-lang/dafny/tree/fix-object-safety-rust does, but &Rc does not make traits object-safe, which means we can't yet use them in the code. I tried also passing a second arguments so that we directly have a Rc<> and don't need to recover it from the &T, but this: Rc<Self> again makes the trait not object-safe, and this: Rc<dyn Any> causes an issue because we can't create this argument when what we have is only the trait, and trait upcast to Any is currently unsound in Rust. This PR is the best workaround I found so far.

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

…unt.

Fixes #5851
I haven't been able to produce consistently any program that could exhibit the issue this PR is solving, the only hint I had that this happened was the report of #5851, which I reasoned manually must come from this function that is being skipped. By making it opaque to the compiler, we prevent optimizations that would skip the strong count from increasing.
Alternative to this would be to pass &Rc<Self> instead of &Self, which is what https://github.com/dafny-lang/dafny/tree/fix-object-safety-rust does, but &Rc does not make traits object-safe, which means we can't yet use them in the code.
I tried also passing a second arguments so that we directly have a `Rc<>` and don't need to recover it from the &T, but `this: Rc<Self>` again makes the trait not object-safe, and `this: Rc<dyn Any>` causes an issue because we can't create this argument when what we have is only the trait, and trait upcast to Any is currently unsound in Rust.
This PR is the best workaround I found so far.
@MikaelMayer MikaelMayer merged commit fc90edc into master Oct 24, 2024
22 checks passed
@MikaelMayer MikaelMayer deleted the fix-5851-increment-strong-count-opaque branch October 24, 2024 18:36
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.

Dafny to Rust : Object::from_ref is unsound
2 participants