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

cargo miri test fails with Undefined Behavior: trying to retag ... #455

Closed
LunNova opened this issue Sep 24, 2023 · 2 comments
Closed

cargo miri test fails with Undefined Behavior: trying to retag ... #455

LunNova opened this issue Sep 24, 2023 · 2 comments
Labels
invalid This doesn't seem right

Comments

@LunNova
Copy link

LunNova commented Sep 24, 2023

Handlers are created with many different types, which share the same first field as the OpCodeHandler struct. They're cast to a *const pointer to this struct despite not being of that type, and stored in a map.

#[repr(C)]
pub(super) struct OpCodeHandler {
pub(super) has_modrm: bool,
}

Later during decoding the handler is cast back to its original type, and cargo miri test fails here with a stacked borrows Undefined Behavior error:

fn decode(self_ptr: *const OpCodeHandler, decoder: &mut Decoder<'_>, instruction: &mut Instruction) {
let this = unsafe { &*(self_ptr as *const Self) };

Related snippets:

// SAFETY:
// code: let this = unsafe { &*(self_ptr as *const Self) };
// The first arg (`self_ptr`) to decode() is always the handler itself, cast to a `*const OpCodeHandler`.
// All handlers are `#[repr(C)]` structs so the OpCodeHandler fields are always at the same offsets.

pub(super) fn get_null_handler() -> (OpCodeHandlerDecodeFn, &'static OpCodeHandler) {
// SAFETY: it's #[repr(C)] and the first part is an `OpCodeHandler`
(OpCodeHandler_Invalid::decode, unsafe { &*(&NULL_HANDLER as *const _ as *const OpCodeHandler) })
}

Running cargo miri test in src/rust produces:

lun@hisame ~/s/d/W/i/s/rust master ✔ ./rw $ cargo miri test                                                                                                                     rustup-dev-shell-env
Preparing a sysroot for Miri (target: x86_64-unknown-linux-gnu)... done
    Updating crates.io index
  Downloaded serde_json v1.0.107
  Downloaded 1 crate (146.5 KB) in 0.18s
   Compiling serde v1.0.188
   Compiling serde_json v1.0.107
   Compiling lazy_static v1.4.0
   Compiling itoa v1.0.9
   Compiling ryu v1.0.15
   Compiling iced-x86 v1.20.0 (/home/lun/sync/dev/W/iced/src/rust/iced-x86)
   Compiling bincode v1.3.3
    Finished test [unoptimized + debuginfo] target(s) in 6.22s
     Running unittests src/lib.rs (target/miri/x86_64-unknown-linux-gnu/debug/deps/iced_x86-e64cb9d7dd2c9529)

running 614 tests
test block_enc::enums::test_relockind_try_from_usize ... ok
test block_enc::enums::test_relockind_values ... ok
test block_enc::tests::br8_16::br8_bwd ... warning: integer-to-pointer cast
    --> iced-x86/src/decoder.rs:17:56
     |
17   |                 let result = $from_le(unsafe { ptr::read_unaligned(data_ptr as *const $mem_ty) }) as $ret_ty;
     |                                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^ integer-to-pointer cast
...
1233 |         mk_read_xx_fn_body! {self, u8, u8::from_le, usize}
     |         -------------------------------------------------- in this macro invocation
     |
     = help: This program is using integer-to-pointer casts or (equivalently) `ptr::from_exposed_addr`,
     = help: which means that Miri might miss pointer bugs in this program.
     = help: See https://doc.rust-lang.org/nightly/std/ptr/fn.from_exposed_addr.html for more details on that operation.
     = help: To ensure that Miri does not miss bugs in your program, use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead.
     = help: You can then pass the `-Zmiri-strict-provenance` flag to Miri, to ensure you are not relying on `from_exposed_addr` semantics.
     = help: Alternatively, the `-Zmiri-permissive-provenance` flag disables this warning.
     = note: BACKTRACE:
     = note: inside `decoder::Decoder::<'_>::read_u8` at iced-x86/src/decoder.rs:17:56: 17:82
note: inside `decoder::Decoder::<'_>::decode_out_ptr`
    --> iced-x86/src/decoder.rs:1403:11
     |
1403 |         let b = self.read_u8();
     |                 ^^^^^^^^^^^^^^
note: inside `decoder::Decoder::<'_>::decode`
    --> iced-x86/src/decoder.rs:1314:4
     |
1314 |             self.decode_out_ptr(instruction.as_mut_ptr());
     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `<decoder::DecoderIntoIter<'_> as std::iter::Iterator>::next`
    --> iced-x86/src/decoder.rs:2629:9
     |
2629 |             Some(self.decoder.decode())
     |                  ^^^^^^^^^^^^^^^^^^^^^
     = note: inside `<std::vec::Vec<instruction::Instruction> as std::vec::spec_from_iter_nested::SpecFromIterNested<instruction::Instruction, decoder::DecoderIntoIter<'_>>>::from_iter` at /home/lun/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/vec/spec_from_iter_nested.rs:26:32: 26:47
     = note: inside `<std::vec::Vec<instruction::Instruction> as std::vec::spec_from_iter::SpecFromIter<instruction::Instruction, decoder::DecoderIntoIter<'_>>>::from_iter` at /home/lun/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/vec/spec_from_iter.rs:33:9: 33:48
     = note: inside `<std::vec::Vec<instruction::Instruction> as std::iter::FromIterator<instruction::Instruction>>::from_iter::<decoder::DecoderIntoIter<'_>>` at /home/lun/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/vec/mod.rs:2747:9: 2747:76
     = note: inside `<decoder::DecoderIntoIter<'_> as std::iter::Iterator>::collect::<std::vec::Vec<instruction::Instruction>>` at /home/lun/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/iter/traits/iterator.rs:2053:9: 2053:38
note: inside `block_enc::tests::decode`
    --> iced-x86/src/block_enc/tests/mod.rs:34:2
     |
34   |     decoder.into_iter().collect()
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `block_enc::tests::encode_test`
    --> iced-x86/src/block_enc/tests/mod.rs:54:20
     |
54   |     let orig_instrs = decode(bitness, orig_rip, original_data, decoder_options);
     |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `block_enc::tests::br8_16::br8_bwd`
    --> iced-x86/src/block_enc/tests/br8_16.rs:157:2
     |
157  | /     encode_test(
158  | |         BITNESS,
159  | |         ORIG_RIP,
160  | |         &original_data,
...    |
166  | |         &expected_reloc_infos,
167  | |     );
     | |_____^
note: inside closure
    --> iced-x86/src/block_enc/tests/br8_16.rs:91:14
     |
90   | #[test]
     | ------- in this procedural macro expansion
91   | fn br8_bwd() {
     |              ^
     = note: this warning originates in the macro `mk_read_xx` which comes from the expansion of the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

error: Undefined Behavior: trying to retag from <2907067> for SharedReadOnly permission at alloc355283[0x1], but that tag does not exist in the borrow stack for this location
    --> iced-x86/src/decoder/handlers/legacy.rs:2584:23
     |
2584 |         let this = unsafe { &*(self_ptr as *const Self) };
     |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |                             |
     |                             trying to retag from <2907067> for SharedReadOnly permission at alloc355283[0x1], but that tag does not exist in the borrow stack for this location
     |                             this error occurs as part of retag at alloc355283[0x0..0x8]
     |
     = 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
help: <2907067> was created by a SharedReadOnly retag at offsets [0x0..0x1]
    --> iced-x86/src/decoder.rs:1571:12
     |
1571 |         (decode)(handler, self, instruction);
     |                  ^^^^^^^
     = note: BACKTRACE (of the first span):
     = note: inside `decoder::handlers::legacy::OpCodeHandler_Xchg_Reg_rAX::decode` at iced-x86/src/decoder/handlers/legacy.rs:2584:23: 2584:50
note: inside `decoder::Decoder::<'_>::decode_table2`
    --> iced-x86/src/decoder.rs:1571:3
     |
1571 |         (decode)(handler, self, instruction);
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `decoder::Decoder::<'_>::decode_out_ptr`
    --> iced-x86/src/decoder.rs:1418:3
     |
1418 |         self.decode_table2(handler, instruction);
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `decoder::Decoder::<'_>::decode`
    --> iced-x86/src/decoder.rs:1314:4
     |
1314 |             self.decode_out_ptr(instruction.as_mut_ptr());
     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `<decoder::DecoderIntoIter<'_> as std::iter::Iterator>::next`
    --> iced-x86/src/decoder.rs:2629:9
     |
2629 |             Some(self.decoder.decode())
     |                  ^^^^^^^^^^^^^^^^^^^^^
     = note: inside `<std::vec::Vec<instruction::Instruction> as std::vec::spec_from_iter_nested::SpecFromIterNested<instruction::Instruction, decoder::DecoderIntoIter<'_>>>::from_iter` at /home/lun/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/vec/spec_from_iter_nested.rs:26:32: 26:47
     = note: inside `<std::vec::Vec<instruction::Instruction> as std::vec::spec_from_iter::SpecFromIter<instruction::Instruction, decoder::DecoderIntoIter<'_>>>::from_iter` at /home/lun/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/vec/spec_from_iter.rs:33:9: 33:48
     = note: inside `<std::vec::Vec<instruction::Instruction> as std::iter::FromIterator<instruction::Instruction>>::from_iter::<decoder::DecoderIntoIter<'_>>` at /home/lun/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/vec/mod.rs:2747:9: 2747:76
     = note: inside `<decoder::DecoderIntoIter<'_> as std::iter::Iterator>::collect::<std::vec::Vec<instruction::Instruction>>` at /home/lun/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/iter/traits/iterator.rs:2053:9: 2053:38
note: inside `block_enc::tests::decode`
    --> iced-x86/src/block_enc/tests/mod.rs:34:2
     |
34   |     decoder.into_iter().collect()
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `block_enc::tests::encode_test`
    --> iced-x86/src/block_enc/tests/mod.rs:54:20
     |
54   |     let orig_instrs = decode(bitness, orig_rip, original_data, decoder_options);
     |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `block_enc::tests::br8_16::br8_bwd`
    --> iced-x86/src/block_enc/tests/br8_16.rs:157:2
     |
157  | /     encode_test(
158  | |         BITNESS,
159  | |         ORIG_RIP,
160  | |         &original_data,
...    |
166  | |         &expected_reloc_infos,
167  | |     );
     | |_____^
note: inside closure
    --> iced-x86/src/block_enc/tests/br8_16.rs:91:14
     |
90   | #[test]
     | ------- in this procedural macro expansion
91   | fn br8_bwd() {
     |              ^
     = note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to previous error; 1 warning emitted

error: test failed, to rerun pass `--lib`
@LunNova LunNova changed the title cargo miri tests fails with Undefined Behavior: trying to retag ... cargo miri test fails with Undefined Behavior: trying to retag ... Sep 24, 2023
@wtfsck
Copy link
Member

wtfsck commented Sep 24, 2023

The error you have is from an experimental miri feature:

     = 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

The pointer passed in to the handler is the instance. It's cast to the 'base class' in another part of the code then in the handler it's cast back to itself. So it's similar to

// C inherits from B
C* c = new C();
B* b = c;
C* c2 = (C*)b;

EDIT:

  • The B* is also converted to a &'static and stored in a static vec with elem &'static B, then that ref is converted to a pointer when passed to the handler.

@wtfsck
Copy link
Member

wtfsck commented Sep 26, 2023

This is a false positive by the experimental miri stacked borrow code. As explained above, the pointer passed into the handler is the instance itself cast to the base class and the current implementation of miri's stacked borrow can't handle those cases yet

As explained here, you can run it with MIRIFLAGS=-Zmiri-tree-borrows since it has no problem with base classes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

2 participants