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 allocation of size 0 in in Thinbox::new() #96485

Closed
matthiaskrgr opened this issue Apr 27, 2022 · 2 comments · Fixed by #96642
Closed

miri reports allocation of size 0 in in Thinbox::new() #96485

matthiaskrgr opened this issue Apr 27, 2022 · 2 comments · Fixed by #96642
Assignees
Labels
C-bug Category: This is a bug.

Comments

@matthiaskrgr
Copy link
Member

I tried this code:

#![feature(thin_box)]

use std::boxed::ThinBox;
fn main() {
    let _thin_concrete_error: ThinBox<Foo> = ThinBox::new(Foo);
}

struct Foo;

cargo miri run

error: Undefined Behavior: creating allocation with size 0
  --> /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/alloc.rs:89:14
   |
89 |     unsafe { __rust_alloc(layout.size(), layout.align()) }
   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ creating allocation with size 0
   |
   = 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: inside `std::alloc::alloc` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/alloc.rs:89:14
   = note: inside `std::boxed::thin::WithHeader::<()>::new::<Foo>` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/boxed/thin.rs:159:23
   = note: inside `std::boxed::ThinBox::<Foo>::new` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/boxed/thin.rs:52:19
note: inside `main` at src/main.rs:5:46
  --> src/main.rs:5:46
   |
5  |     let _thin_concrete_error: ThinBox<Foo> = ThinBox::new(Foo);
   |                                              ^^^^^^^^^^^^^^^^^

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

error: aborting due to previous error

Technically miri already shows that the code is unsafe-wrapped. Maybe it needs some special handing of Thinbox?

miri 0.1.0 (a71a008 2022-04-25)

@matthiaskrgr matthiaskrgr added C-bug Category: This is a bug. A-miri Area: The miri tool labels Apr 27, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Apr 27, 2022

This looks like a bug in thinbox. Box and Vec don't go into the allocator at all for zsts

@thomcc
Copy link
Member

thomcc commented Apr 28, 2022

This is a bug in ThinBox. I don't mind taking it, but won't have time until Monday evening. If someone wants to grab it before then feel free.

@RalfJung RalfJung removed the A-miri Area: The miri tool label Apr 30, 2022
@thomcc thomcc self-assigned this May 1, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jun 4, 2022
Avoid zero-sized allocs in ThinBox if T and H are both ZSTs.

This was surprisingly tricky, and took longer to get right than expected. `ThinBox` is a surprisingly subtle piece of code. That said, in the end, a lot of this was due to overthinking[^overthink] -- ultimately the fix ended up fairly clean and simple.

[^overthink]: Honestly, for a while I was convinced this couldn't be done without allocations or runtime branches in these cases, but that's obviously untrue.

Anyway, as a result of spending all that time debugging, I've extended the tests quite a bit, and also added more debug assertions. Many of these helped for subtle bugs I made in the middle (for example, the alloc/drop tracking is because I ended up double-dropping the value in the case where both were ZSTs), they're arguably a bit of overkill at this point, although I imagine they could help in the future too.

Anyway, these tests cover a wide range of size/align cases, nd fully pass under miri[^1]. They also do some smoke-check asserting that the value has the correct alignment, although in practice it's totally within the compiler's rights to delete these assertions since we'd have already done UB if they get hit. They have more boilerplate than they really need, but it's not *too* bad on a per-test basis.

A notable absence from testing is atypical header types, but at the moment it's impossible to manually implement `Pointee`. It would be really nice to have testing here, since it's not 100% obvious to me that the aligned read/write we use for `H` are correct in the face of arbitrary combinations of `size_of::<H>()`, `align_of::<H>()`, and `align_of::<T>()`. (That said, I spent a while thinking through it and am *pretty* sure it's fine -- I'd just feel... better if we could test some cases for non-ZST headers which have unequal and align).

[^1]: Or at least, they pass under miri if I copy the code and tests into a new crate and run miri on it (after making it less stdlibified).

Fixes rust-lang#96485.

I'd request review `@yaahc,` but I believe you're taking some time away from reviews, so I'll request from the previous PR's reviewer (I think that the context helps, even if the actual change didn't end up being bad here).

r? `@joshtriplett`
@bors bors closed this as completed in 07f586f Jun 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants