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

Stacked borrows error when replacing the default allocator with... the default allocator #2678

Closed
Diggsey opened this issue Nov 18, 2022 · 8 comments

Comments

@Diggsey
Copy link

Diggsey commented Nov 18, 2022

I have some code which passes MIRI. However, when I replace the default allocator with the System allocator, it gives a stacked borrows error:

#[global_allocator]
static ALLOCATOR: System = System;

This implies something very weird is going on. None of my code is in the backtrace.

This is the full error:

error: Undefined Behavior: attempting a read access using <3316050> at alloc95894[0x8], but that tag does not exist in the borrow stack for this location
   --> C:\Users\diggs\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\sys\windows\alloc.rs:217:26
    |
217 |                 unsafe { ptr::read((ptr as *mut Header).sub(1)).0 }
    |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |                          |
    |                          attempting a read access using <3316050> at alloc95894[0x8], but that tag does not exist in the borrow stack for this location
    |                          this error occurs as part of an access at alloc95894[0x8..0x10]
    |
    = 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: <3316050> was created by a Unique retag at offsets [0x10..0x210]
    = note: BACKTRACE:
    = note: inside `std::sys::windows::alloc::<impl std::alloc::GlobalAlloc for std::alloc::System>::dealloc` at C:\Users\diggs\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\sys\windows\alloc.rs:217:26
note: inside `tests::_::__rg_dealloc` at src\lib.rs:53:23
   --> src\lib.rs:53:23
    |
52  |     #[global_allocator]
    |     ------------------- in this procedural macro expansion
53  |     static ALLOCATOR: System = System;
    |                       ^^^^^^
    = note: inside `std::alloc::dealloc` at C:\Users\diggs\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\alloc\src\alloc.rs:113:14
    = note: inside `<std::alloc::Global as std::alloc::Allocator>::deallocate` at C:\Users\diggs\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\alloc\src\alloc.rs:250:22
    = note: inside `alloc::alloc::box_free::<std::sync::mpmc::counter::Counter<std::sync::mpmc::list::Channel<test::event::CompletedTest>>, std::alloc::Global>` at C:\Users\diggs\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\alloc\src\alloc.rs:348:9
    = note: inside `std::ptr::drop_in_place::<std::boxed::Box<std::sync::mpmc::counter::Counter<std::sync::mpmc::list::Channel<test::event::CompletedTest>>>> - shim(Some(std::boxed::Box<std::sync::mpmc::counter::Counter<std::sync::mpmc::list::Channel<test::event::CompletedTest>>>))` at C:\Users\diggs\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ptr\mod.rs:490:1
    = note: inside `std::mem::drop::<std::boxed::Box<std::sync::mpmc::counter::Counter<std::sync::mpmc::list::Channel<test::event::CompletedTest>>>>` at C:\Users\diggs\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\mem\mod.rs:979:24
    = note: inside `std::sync::mpmc::counter::Sender::<std::sync::mpmc::list::Channel<test::event::CompletedTest>>::release::<[closure@<std::sync::mpmc::Sender<test::event::CompletedTest> as std::ops::Drop>::drop::{closure#1}]>` at C:\Users\diggs\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\sync\mpmc\counter.rs:66:17
    = note: inside `<std::sync::mpmc::Sender<test::event::CompletedTest> as std::ops::Drop>::drop` at C:\Users\diggs\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\sync\mpmc\mod.rs:231:45
    = note: inside `std::ptr::drop_in_place::<std::sync::mpmc::Sender<test::event::CompletedTest>> - shim(Some(std::sync::mpmc::Sender<test::event::CompletedTest>))` at C:\Users\diggs\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ptr\mod.rs:490:1
    = note: inside `std::ptr::drop_in_place::<std::sync::mpsc::Sender<test::event::CompletedTest>> - shim(Some(std::sync::mpsc::Sender<test::event::CompletedTest>))` at C:\Users\diggs\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ptr\mod.rs:490:1
    = note: inside `test::run_tests::<[closure@test::run_tests_console::{closure#2}]>` at C:\Users\diggs\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\test\src\lib.rs:455:1
    = note: inside `test::run_tests_console` at C:\Users\diggs\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\test\src\console.rs:293:5
    = note: inside `test::test_main` at C:\Users\diggs\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\test\src\lib.rs:140:15
    = note: inside `test::test_main_static` at C:\Users\diggs\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\test\src\lib.rs:159:5
    = note: inside `main`
    = note: this error originates in the attribute macro `global_allocator` (in Nightly builds, run with -Z macro-backtrace for more info)

As you can see, I'm running MIRI on windows.

@RalfJung
Copy link
Member

I think this is a duplicate of #2104.

@RalfJung
Copy link
Member

RalfJung commented Nov 18, 2022

You are not replacing the default allocator with the default allocator, you are replacing it with the system allocator. That's not the same thing. :)

On some platforms the default allocator used to be jemalloc. Not sure if that is still the case. On Miri, the default allocator is a special Miri-supplied allocator that can do more checking than would be possible if we directly forwarded to whatever std uses as default allocator on that platform. As a side-effect, this also fixes the aliasing trouble on Windows.

@Diggsey
Copy link
Author

Diggsey commented Nov 18, 2022

You are not replacing the default allocator with the default allocator, you are replacing it with the system allocator. That's not the same thing. :)

Well... without MIRI, System is the default allocator on windows, but sure 😛 I think it's surprising that MIRI replaces the default allocator in a way which is visible to the program, let's put it that way.

@RalfJung
Copy link
Member

Well... without MIRI, System is the default allocator on windows, but sure

I don't think this is a guarantee -- AFAIK code which allocates via Global but then frees via System is considered wrong. Is that not the case?

It is certainly a bug that System does not work properly on Windows. That is what is being tracked at #2104. Without that bug, Miri replacing the allocator would only manifest itself in Miri being able to detect more UB.

@Diggsey
Copy link
Author

Diggsey commented Nov 18, 2022

To provide more context: it's not uncommon to have "middleware" allocators which wrap another allocator. For example, I have a crate mockalloc which checks that allocations/frees match up correctly, etc.

In order to use mockalloc you need to wrap a "default" allocator. I was doing that by wrapping System. Is there some other way to really get the default allocator?

@Diggsey
Copy link
Author

Diggsey commented Nov 18, 2022

Also, I think this backs up what I'm saying:

https://doc.rust-lang.org/std/alloc/struct.System.html

This type implements the GlobalAlloc trait and Rust programs by default work as if they had this definition:

use std::alloc::System;

#[global_allocator]
static A: System = System;

fn main() {
    let a = Box::new(4); // Allocates from the system allocator.
    println!("{a}");
}

You can also define your own wrapper around System if you’d like, such as keeping track of the number of all bytes allocated:

@RalfJung
Copy link
Member

In order to use mockalloc you need to wrap a "default" allocator. I was doing that by wrapping System. Is there some other way to really get the default allocator?

I'm not disputing that setting System as the global allocator should work. :) It's just pretty hard for Windows -- we need to replace Stacked Borrows by something new altogether. The good news is that I am actively working with an intern on exactly that project.

I don't think there is a new bug here so I'll close this as a duplicate of #2104. I still feel like if you are not actually setting the global allocator, you better not rely on it actually being System -- IOW, I still think this code should raise an error. But that would be a separate issue.

@RalfJung RalfJung closed this as not planned Won't fix, can't repro, duplicate, stale Nov 19, 2022
@RalfJung
Copy link
Member

As for the docs you quoted, there are also other docs that say the exact opposite, so it's rather unclear what Miri should do... see rust-lang/wg-allocators#108.

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