search

zyantific / zydis-rs

Zydis Rust Bindings
MIT License
83 stars 14 forks source link

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

Closed williballenthin closed 2 years ago

williballenthin commented 2 years ago

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 commented 2 years ago

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

williballenthin commented 2 years ago

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

th0rex commented 2 years ago

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!

williballenthin commented 2 years ago

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 commented 2 years ago

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 :)