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

Userdata not correctly passed to formatted callback when LTO enabled #29

Closed
williballenthin opened this issue Jan 14, 2022 · 5 comments
Closed
Assignees

Comments

@williballenthin
Copy link
Contributor

I believe there is probably a UB bug in the handling of userdata pointers passed from Formatter.format_instruction to formatter callbacks. This results in segfaults and unexpected unwrap failures in non-unsafe client code. So far, I've only been able to trigger this issue when optimizations and LTO are enabled.

The example repo https://github.com/williballenthin/zydis-rs-issue-29 contains minimal programs that demonstrate the issue. The issue is present when built in release mode (which has fat LTO enabled) but not in debug mode.

b.rs is the more minimal example and completely non-unsafe. Although Some(...) is passed to format_instruction, the formatter callback receives None as the userdata:

image

image

a.rs more closely represents the code in another project that led to this bug discovery. There's a bit more data provided in the userdata structure, and rather than receiving None, the callback ends up with some unexpected data and segfaults. (Note: there is a single line of unsafe in this binary; however, I believe its unrelated, as I think its a well defined and valid cast across Formatter representations.)

image
image

I suspect the behaviors shown by a.rs and b.rs both stem from the same bug.

Notably, these test cases only demonstrate the issue when fat LTO is enabled (see release profile in Cargo.toml). However, I'm not exactly sure what this means. Suspicions include:

  1. maybe the unsafe casting from user data to &mut dyn Any and back is not well defined, or
  2. there's a bug in optimizations enabled by LTO.

Given that LTO is fairly widely used and tested, I think (1) or similar is more likely than (2). Unfortunately, I have pretty much avoided unsafe Rust because I don't know the safety rules well enough, so I don't know how to triage further.

@williballenthin
Copy link
Contributor Author

I tried using miri to audit the unsafe code in zydis-rs, but it is unable to emulate into zydis-c and bails.

@williballenthin
Copy link
Contributor Author

@th0rex tagging you since I think you originally added this code in #2 and would be familiar with the userdata casting.

th0rex added a commit that referenced this issue Jan 15, 2022
The problem was that the `mut x` in the match arm is instantly out of
scope, and this lead to problems with lto builds. Move the match to the
outside and duplicate the function call, such that `x` stays in scope
for the whole duration of the call. See #29.
@th0rex
Copy link
Collaborator

th0rex commented Jan 15, 2022

Hi, thanks for the detailed bug report!

I believe I found and fixed the problem, could you test the latest master on your side? If it's working I'll do a release on crates.io.

Thanks again for filing the bug!

@th0rex th0rex self-assigned this Jan 15, 2022
@th0rex th0rex closed this as completed in c94eb56 Jan 15, 2022
@th0rex th0rex reopened this Jan 15, 2022
@williballenthin
Copy link
Contributor Author

With the changes on nightly, my test cases and external project work as expected. Thanks @th0rex!

As an aside, it took me many reads of the patch to understand where the bug was. This is why I avoid unsafe :-) I'm so used to rustc providing reliable warnings/errors that I never would have guessed about the scope issue, esp with no warnings or lint failures. Thanks again for digging in and quickly releasing a fix!

@th0rex
Copy link
Collaborator

th0rex commented Jan 18, 2022

Nice to hear that it works. For some reason I'm unable to do a release on crates.io, we'll have to wait for @athre0z to be back for that. But I hope building directly from git is good enough for now :)

@th0rex th0rex closed this as completed Jan 18, 2022
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

No branches or pull requests

2 participants