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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix stacked borrows when dropping #81

Merged
merged 1 commit into from
Jul 7, 2022
Merged

Conversation

TimDiekmann
Copy link
Contributor

馃専 What is the purpose of this PR?

While adding a compatibility layer in error-stack for eyre, miri has detected a bug we also encountered when implementing error-stack.

error: Undefined Behavior: trying to reborrow <226876> for Unique permission at alloc95485[0x18], but that tag does not exist in the borrow stack for this location
   --> .cargo/registry/src/github.com-1ecc6299db9ec823/eyre-0.6.8/src/error.rs:521:20
    |
521 |     let unerased = mem::transmute::<Box<ErrorImpl<()>>, Box<ErrorImpl<E>>>(e);
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |                    |
    |                    trying to reborrow <226876> for Unique permission at alloc95485[0x18], but that tag does not exist in the borrow stack for this location
    |                    this error occurs as part of a reborrow at alloc95485[0x0..0x30]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information

馃敆 Related links

馃攳 What does this change?

  • Replacing the mem::transmute when dropping with a pointer cast

@TimDiekmann TimDiekmann changed the title Fix stacked borrows when dropping Error Fix stacked borrows when dropping Jul 7, 2022
@yaahc
Copy link
Collaborator

yaahc commented Jul 7, 2022

Wow, is this all that's needed to satisfy miri? I was under the impression from #59 that it would be more involved.

@yaahc yaahc merged commit dba5231 into eyre-rs:master Jul 7, 2022
@yaahc
Copy link
Collaborator

yaahc commented Jul 7, 2022

I'll make sure to cut a new release later today, thank you for the fix!

@TimDiekmann
Copy link
Contributor Author

I cannot guarantee, that every test is passing using miri because some tests use features, which are not available with miri and I didn't investigate further. However, simple tests (create a Report, wrap it a few times, drop it) are now passing.

If you need help setting up miri for CI feel free to ping me! 馃檪

@yaahc
Copy link
Collaborator

yaahc commented Jul 7, 2022

If you need help setting up miri for CI feel free to ping me! 馃檪

I mean, if you wanted to do that please do so! I haven't been great about making time for maint on eyre and appreciate all the help I can get.

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.

Allow use under miri (aka: fix UB / update Error impl to contain some of anyhow's bugfixes since Jan)
2 participants