Does not honor alignment reliably

[MageSlayer]

Hi

Following example reliably fails on mimalloc-0.1.32.
Crash is always at 4096 alignment.

 #[test]                                                                                                                                                                                                                                     
    fn test1() { ▶️ Run Test|Debug                                                                                                                                                                                                           
        for i in 0..1000 {                                                                                                                                                                                                                      
            let size = 4096;                                                                                                                                                                                                                    
            let align = 1 << (i % 16);                                                                                                                                                                                                          
            let ptr = unsafe {                                                                                                                                                                                                                  
                std::alloc::alloc(std::alloc::Layout::from_size_align_unchecked(size, align))                                                                                                                                                   
            };                                                                                                                                                                                                                                  
            assert!(!ptr.is_null());                                                                                                                                                                                                            
            assert!((ptr as usize) % align == 0, "Align {} {}", i, align);                                                                                                                                                                      
        }                                                                                                                                                                                                                                       
    }

[gustav3d]

I'm unable to reproduce test failure on my Arch Linux using both stable and nightly rust.
Whats your config more exactly ?.

[MageSlayer]

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.

[gustav3d]

Its scary, cause if there is an issue for real, its super bad :(

[MageSlayer]

My colleague helped me with that.
It can be reproduced now.
See attached full project.

mimalloc.tar.gz

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.

[zachs18]

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}");
    }
}

[thomcc]

Yes, this line should probably be removed

mimalloc_rust/src/lib.rs

Line 56 in 934a2fb

|| (alignment == size && alignment <= 4096)

[thomcc]

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)

[octavonce]

@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?

[thomcc]

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.

[nathaniel-daniel]

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?

[octavonce]

@nathaniel-daniel Thank you for your research into this. Yes, go ahead.