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

Miri reports UB in tests #145

Closed
y21 opened this issue Jul 6, 2022 · 7 comments · Fixed by #146 or #147
Closed

Miri reports UB in tests #145

y21 opened this issue Jul 6, 2022 · 7 comments · Fixed by #146 or #147

Comments

@y21
Copy link

y21 commented Jul 6, 2022

Haven't seen anyone mention this yet so I thought I'd open an issue here. Running the current tests under miri reports Undefined Behavior in Gc<T>'s Drop impl:

test test_derive_bounds ... error: Undefined Behavior: dereferencing pointer failed: 0x1c56a8 is not a valid pointer
   --> /home/timo/rust-gc/gc/src/lib.rs:134:18
    |
134 |         unsafe { &*clear_root_bit(self.ptr_root.get()).as_ptr() }
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ dereferencing pointer failed: 0x1c56a8 is not a valid pointer
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
    = note: backtrace:
    = note: inside `gc::Gc::<Thunk<NotTrace>>::inner` at /home/timo/rust-gc/gc/src/lib.rs:134:18
note: inside `<gc::Gc<Thunk<NotTrace>> as std::ops::Drop>::drop` at /home/timo/rust-gc/gc/src/lib.rs:279:17
   --> /home/timo/rust-gc/gc/src/lib.rs:279:17
    |
279 |                 self.inner().unroot_inner();
    |                 ^^^^^^^^^^^^
@Manishearth
Copy link
Owner

I don't have time to debug this, but I bet this is mostly because this crate was written a while ago before a lot of this stuff was pinned down and it's doing some pointer stuff in a way that's not ideal.

In particular, we're doing pointer masking, which probably falls afoul of strict provenance.

@andersk
Copy link
Collaborator

andersk commented Jul 6, 2022

If that is the issue, it might be easy to fix using https://crates.io/crates/sptr.

@Manishearth
Copy link
Owner

My understanding was that strict provenance was something that was optional in miri, however. Unsure if I want to add new deps for it ust yet, might want to wait till those things are in stable.

andersk added a commit to andersk/rust-gc that referenced this issue Jul 6, 2022
@andersk
Copy link
Collaborator

andersk commented Jul 6, 2022

I was able to reproduce and find a simple fix: #146.

@y21
Copy link
Author

y21 commented Jul 6, 2022

Can confirm that PR did fix this specific error, however miri still reports UB (specifically in GcStates Drop code when sweeping objects)

Error
error: Undefined Behavior: attempting a write access using <210556> at alloc88933[0x38], but that tag does not exist in the borrow stack for this location
   --> /workspaces/rust-gc/gc/src/gc.rs:231:13
    |
231 |             *incoming = node.header.next.take();
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |             |
    |             attempting a write access using <210556> at alloc88933[0x38], but that tag does not exist in the borrow stack for this location
    |             this error occurs as part of an access at alloc88933[0x38..0x48]
    |
    = 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: <210556> was created by a retag at offsets [0x38..0x48]
   --> /workspaces/rust-gc/gc/src/gc.rs:213:31
    |
213 |                     incoming: unmark_head,
    |                               ^^^^^^^^^^^
help: <210556> was later invalidated at offsets [0x38..0x48]
   --> /workspaces/rust-gc/gc/src/gc.rs:243:14
    |
243 |         mark(&mut st.boxes_start);
    |              ^^^^^^^^^^^^^^^^^^^
    = note: backtrace:
    = note: inside `gc::gc::collect_garbage::sweep` at /workspaces/rust-gc/gc/src/gc.rs:231:13
note: inside `gc::gc::collect_garbage` at /workspaces/rust-gc/gc/src/gc.rs:244:9
   --> /workspaces/rust-gc/gc/src/gc.rs:244:9
    |
244 |         sweep(unmarked, &mut st.stats.bytes_allocated);
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `<gc::gc::GcState as std::ops::Drop>::drop` at /workspaces/rust-gc/gc/src/gc.rs:15:13
   --> /workspaces/rust-gc/gc/src/gc.rs:15:13
    |
15  |             collect_garbage(self);
    |             ^^^^^^^^^^^^^^^^^^^^^

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

Interestingly this error doesn't have an associated test and all tests pass, but I suppose that's just because it happens in the destructor of a threadlocal.

@andersk
Copy link
Collaborator

andersk commented Jul 7, 2022

Oh hmm, that one is a new error with this week’s nightlies.

  • rustc 1.64.0-nightly (2f3ddd9f5 2022-06-27), miri 0.1.0 (9e2dac4 2022-06-26): works
  • rustc 1.64.0-nightly (7425fb293 2022-06-30), miri 0.1.0 (ff62c3a 2022-06-30): fails

Probably due to rust-lang/miri#2275, which was merged within that range. Indeed, earlier nightlies fail in the same way with MIRIFLAGS=-Zmiri-permissive-provenance.

@andersk
Copy link
Collaborator

andersk commented Jul 7, 2022

#147 seems to fix everything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants