Closed MageSlayer closed 10 months ago
I'm unable to reproduce test failure on my Arch Linux using both stable and nightly rust. Whats your config more exactly ?.
Hm. That's really strange. I cannot reproduce it anymore.
I've made some upgrades recently (glibc & others) at my Devuan laptop. Looks like it influenced it somehow.
Its scary, cause if there is an issue for real, its super bad :(
My colleague helped me with that. It can be reproduced now. See attached full project.
Now it crashes with:
running 3 tests
test tests::it_works ... ok
test tests::wont_work ... ok
test tests::it_works2 ... FAILED
failures:
---- tests::it_works2 stdout ----
thread 'tests::it_works2' panicked at 'Align 12 4096', src/main.rs:34:13
stack backtrace:
0: rust_begin_unwind
at /rustc/5dc8789e300930751a78996da0fa906be5a344a2/library/std/src/panicking.rs:515:5
1: std::panicking::begin_panic_fmt
at /rustc/5dc8789e300930751a78996da0fa906be5a344a2/library/std/src/panicking.rs:457:5
2: mimalloc_test::tests::it_works2
at ./src/main.rs:34:13
3: mimalloc_test::tests::it_works2::{{closure}}
at ./src/main.rs:26:5
4: core::ops::function::FnOnce::call_once
at /rustc/5dc8789e300930751a78996da0fa906be5a344a2/library/core/src/ops/function.rs:227:5
5: core::ops::function::FnOnce::call_once
at /rustc/5dc8789e300930751a78996da0fa906be5a344a2/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
use mimalloc::MiMalloc;
#[global_allocator]
static GLOBAL: MiMalloc = MiMalloc;
#[repr(align(64))]
struct Overaligned(u8);
fn main() {
for i in 1.. {
let x = Box::new(Overaligned(0));
let ptr: *const Overaligned = &*x;
assert!(((ptr as usize) % std::mem::align_of::<Overaligned>()) == 0, "unaligned box on try {i}");
}
}
On my machine, this program reliably panics on x86_64-unknown-linux-gnu
and i686-unknown-linux-gnu
with mimalloc
0.1.37 with default features, running in debug mode or release mode, after 1 or 2 tries. (It also panics on aarch64-unknown-linux-gnu
and armv7-unknown-linux-gnueabi
under cross
, and x86_64-pc-windows-gnu
under wine). It does not panic without the secure
feature.
(In debug mode it panics on the let ptr =
line, as Rust since version 1.70.0 performs alignment checks on dereference in debug mode.)
Currently looking into if this is an issue in mimalloc_rust
or mimalloc
itself.
I think the issue is in mimalloc_rust
, specifically fn may_use_unaligned_api
, which assumes that allocations whose size is a power of 2 <= 4096 will be aligned to that size, which is not true when the secure
feature is enabled as shown by the below program which panics when using libmimalloc-sys
with features = ["secure"]
fn main() {
for i in 1.. {
let ptr = unsafe { libmimalloc_sys::mi_malloc(64) };
assert!(((ptr as usize) % 64) == 0, "unaligned on try {i}");
}
}
Yes, this line should probably be removed https://github.com/purpleprotocol/mimalloc_rust/blob/934a2fb453705a7c7b42890ba36f665bed5e53d6/src/lib.rs#L56
That said people probably shouldn't be using the secure mode. It's a bad default. (not that this means the bug shouldn't be fixed)
@thomcc The initial reasoning behind the secure mode by default was that secure mode is still as performant or more than other allocators while providing enhanced security. It seems it always introduces more problems though so I will remove it as a default feature.
Regarding this line:
|| (alignment == size && alignment <= 4096)
Maybe it should only be removed with secure mode?
I'm not sure if it's actually something stable we can rely on. @daanx might be able to say if it is, but it's not clear to me.
I've been trying to find more info about this. Looks like mi_heap_malloc_aligned_at
, which mi_aligned_alloc
is built on, actually dispatches to mi_heap_malloc_small
. The relevant code also suggests the alignment promises change depending on whether MI_PADDING
is on, like when secure mode is on. This can explain the behavior seen earlier in this thread, may_use_unaligned_api
likely is insufficient.
Furthermore, following the calls eventually leads to to mi_heap_malloc_zero_aligned_at_fallback
, where it looks like it does its own checks to use unaligned malloc calls where possible. In addition, it looks like mi_malloc_satisfies_alignment
was removed in favor of trusting mi_aligned_alloc
to do the right thing.
As a result, I think may_use_unaligned_api
should be removed in favor of passing all calls to mi_aligned_alloc
, especially since it seems like this function makes the secure
feature, ironically, unsound. Would a PR be accepted for this?
@nathaniel-daniel Thank you for your research into this. Yes, go ahead.
Hi
Following example reliably fails on mimalloc-0.1.32. Crash is always at 4096 alignment.