EXC_BAD_ACCESS: pointer being freed was not allocated

[Kerollmops]

Currently the RedisAlloc is enforced at compilation and then at runtime a flag is used to enabled the allocator or not.

The first problem is that if the application do not want to use it it is currently not possible as it is always enabled in the macro.

The second problem is that the global allocator is enforced and even if the use_redis_alloc function is not called the allocator will fallback on the System allocator, therefore it is not possible to use, for example, the jemalloc one (which can be way more performant than the system's one).

The third and last problem is that I faced BAD_ACCESS crashes that were triggered inside the RedisAlloc::dealloc function of the kind: "pointer used after being freed". I think about it and it was probably because the USE_REDIS_ALLOC static was enabled after some alloc calls were already made to the allocator and therefore allocations were made by the system's one but freed by the redis one.

One solution could have been to enforce the allocator only when a specific feature is set and do not rely on a secondary runtime setting (i.e. the flag) to enable or disable the allocator, it is dangerous.

# Cargo.toml
[features]
default = ["redis-allocator"]
experimental-api = []
redis-allocator = []

// lib.rs
#[cfg_attr(feature="redis-allocator", global_allocator)]
static ALLOC: crate::alloc::RedisAlloc = crate::alloc::RedisAlloc;

By the way I did not take the time to look at what does declaring a global_allocator do in a library, wasn't it made to be used in a binary project? What happen if I declare another global_allocator inside my main.rs file?

[Kerollmops]

I just hit the "used after freed" error and it did not comes from the RedisAlloc type.

redis-server(31749,0x700005d85000) malloc: *** error for object 0x101900e80: pointer being freed was not allocated
Process 31749 stopped
* thread #5, stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
    frame #0: 0x0000000100008c87 redis-server`dictSdsKeyCompare + 71
redis-server`dictSdsKeyCompare:
->  0x100008c87 <+71>: movzbl -0x1(%rdx), %edi
    0x100008c8b <+75>: movl   %edi, %ecx
    0x100008c8d <+77>: andb   $0x7, %cl
    0x100008c90 <+80>: xorl   %eax, %eax

lldb thread backtrace

All those lines report are relative to this repo at this specific commit: https://github.com/Kerollmops/redismodule-rs/tree/3566dab01a62d310627709837c601815a37b90ce

``` * thread #5, stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT) * frame #0: 0x0000000100008c87 redis-server`dictSdsKeyCompare + 71 frame #1: 0x0000000100006a82 redis-server`dictAddRaw + 146 frame #2: 0x00000001000069b7 redis-server`dictAdd + 23 frame #3: 0x0000000100025bba redis-server`dbAdd + 42 frame #4: 0x0000000100025d9f redis-server`setKey + 63 frame #5: 0x000000010008ed2b redis-server`RM_StringSet + 75 frame #6: 0x0000000101e339b1 libspo2.dylib`redismodule::raw::string_set::h43380443db61ae94(key=0x0000000100600440, s=0x0000000101904ab0) at raw.rs:237:13 frame #7: 0x0000000101e3af5f libspo2.dylib`redismodule::key::RedisKeyWritable::write::h1837aa3bbfad52e1(self=0x0000000102900228, val=(data_ptr = "208 Already Reportedlate, gzip\r\n", length = 20)) at key.rs:149:14 frame #8: 0x0000000101a13a06 libspo2.dylib`spo2::health_checker::health_checker::_$u7b$$u7b$closure$u7d$$u7d$::h12785cbc7d0c1d66 at health_checker.rs:27:24 frame #9: 0x0000000101a163dd libspo2.dylib`_$LT$std..future..GenFuture$LT$T$GT$$u20$as$u20$core..future..future..Future$GT$::poll::_$u7b$$u7b$closure$u7d$$u7d$::h2ba3aadc66258a96 at future.rs:42:38 frame #10: 0x0000000101a15938 libspo2.dylib`std::future::set_task_context::h78731454346506a0(cx=0x000070000577b6d0, f=closure-1 @ 0x000070000577b228) at future.rs:78:4 frame #11: 0x0000000101a161c2 libspo2.dylib`_$LT$std..future..GenFuture$LT$T$GT$$u20$as$u20$core..future..future..Future$GT$::poll::h64c9a35070938477(self=Pin<&mut std::future::GenFuture> @ 0x000070000577b288, cx=0x000070000577b6d0) at future.rs:42:8 frame #12: 0x0000000101a15f8d libspo2.dylib`std::future::poll_with_tls_context::_$u7b$$u7b$closure$u7d$$u7d$::h052e8d9a5407d711(cx=0x000070000577b6d0) at future.rs:120:26 frame #13: 0x0000000101a14efe libspo2.dylib`std::future::get_task_context::h3372b149ae2e96f3(f=closure-0 @ 0x000070000577b318) at future.rs:110:13 frame #14: 0x0000000101a15f5d libspo2.dylib`std::future::poll_with_tls_context::hea014bc45aa26761(f=Pin<&mut std::future::GenFuture> @ 0x000070000577b380) at future.rs:120:4 frame #15: 0x0000000101a0c010 libspo2.dylib`spo2::event_subscription::_$u7b$$u7b$closure$u7d$$u7d$::hce391bb07a6d15e4 at lib.rs:43:26 frame #16: 0x0000000101a168dd libspo2.dylib`_$LT$std..future..GenFuture$LT$T$GT$$u20$as$u20$core..future..future..Future$GT$::poll::_$u7b$$u7b$closure$u7d$$u7d$::hcb96981c17263148 at future.rs:42:38 frame #17: 0x0000000101a15ad8 libspo2.dylib`std::future::set_task_context::h8cf5647d2a7d7ad4(cx=0x000070000577b6d0, f=closure-1 @ 0x000070000577b4c8) at future.rs:78:4 frame #18: 0x0000000101a16262 libspo2.dylib`_$LT$std..future..GenFuture$LT$T$GT$$u20$as$u20$core..future..future..Future$GT$::poll::hc864537e56d9b55c(self=Pin<&mut std::future::GenFuture> @ 0x000070000577b528, cx=0x000070000577b6d0) at future.rs:42:8 frame #19: 0x0000000101dbe2c2 libspo2.dylib`_$LT$futures_core..future..future_obj..LocalFutureObj$LT$T$GT$$u20$as$u20$core..future..future..Future$GT$::poll::hea15f96502219cdc(self=Pin<&mut futures_core::future::future_obj::LocalFutureObj<()>> @ 0x000070000577b578, cx=0x000070000577b6d0) at future_obj.rs:80:12 frame #20: 0x0000000101dbe22a libspo2.dylib`_$LT$futures_core..future..future_obj..FutureObj$LT$T$GT$$u20$as$u20$core..future..future..Future$GT$::poll::h2299da3adb3ff85a(self=Pin<&mut futures_core::future::future_obj::FutureObj<()>> @ 0x000070000577b5d0, cx=0x000070000577b6d0) at future_obj.rs:131:8 frame #21: 0x0000000101dbe4ad libspo2.dylib`futures_util::future::FutureExt::poll_unpin::h0164e638ade8660b(self=0x000070000577b698, cx=0x000070000577b6d0) at mod.rs:538:8 frame #22: 0x0000000101dbb881 libspo2.dylib`futures_executor::thread_pool::Task::run::h3d4f975d7171f1d0(self=) at thread_pool.rs:340:26 frame #23: 0x0000000101dba761 libspo2.dylib`futures_executor::thread_pool::PoolState::work::h9db7b8303385c3fd(self=0x0000000100315b60, idx=2, after_start=Option>> @ 0x000070000577b840, before_stop=Option>> @ 0x000070000577b850) at thread_pool.rs:174:38 frame #24: 0x0000000101dbb6a0 libspo2.dylib`futures_executor::thread_pool::ThreadPoolBuilder::create::_$u7b$$u7b$closure$u7d$$u7d$::h156cc90b23bb7bc3 at thread_pool.rs:302:41 frame #25: 0x0000000101dadcd1 libspo2.dylib`std::sys_common::backtrace::__rust_begin_short_backtrace::hdf8d6f36a9d23b40(f=) at backtrace.rs:77:4 frame #26: 0x0000000101dbedf1 libspo2.dylib`std::thread::Builder::spawn_unchecked::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::hdc7dd9fb41a2b122 at mod.rs:470:16 frame #27: 0x0000000101dadc11 libspo2.dylib`_$LT$std..panic..AssertUnwindSafe$LT$F$GT$$u20$as$u20$core..ops..function..FnOnce$LT$$LP$$RP$$GT$$GT$::call_once::hdf95eef6b80f4b28(self=, _args=) at panic.rs:315:8 frame #28: 0x0000000101dae6da libspo2.dylib`std::panicking::try::do_call::hb47c4e52703ae4fe(data="P[1") at panicking.rs:296:39 frame #29: 0x0000000101ea1abf libspo2.dylib`__rust_maybe_catch_panic at lib.rs:80:7 [opt] frame #30: 0x0000000101dae552 libspo2.dylib`std::panicking::try::h1f32b7be8462fbec(f=) at panicking.rs:275:12 frame #31: 0x0000000101dade31 libspo2.dylib`std::panic::catch_unwind::hade1dd5d54af91cc(f=) at panic.rs:394:8 frame #32: 0x0000000101dbec27 libspo2.dylib`std::thread::Builder::spawn_unchecked::_$u7b$$u7b$closure$u7d$$u7d$::h695f8cc8d8473efe at mod.rs:469:29 frame #33: 0x0000000101da2995 libspo2.dylib`core::ops::function::FnOnce::call_once$u7b$$u7b$vtable.shim$u7d$$u7d$::h98e1dead630bc35e((null)=0x000070000577be60, (null)=) at function.rs:227:4 frame #34: 0x0000000101e75a0e libspo2.dylib`_$LT$alloc..boxed..Box$LT$F$GT$$u20$as$u20$core..ops..function..FnOnce$LT$A$GT$$GT$::call_once::h5c962d686ddc4dfd at boxed.rs:922:8 [opt] frame #35: 0x0000000101ea090e libspo2.dylib`std::sys::unix::thread::Thread::new::thread_start::h965e643076cc7fbb [inlined] _$LT$alloc..boxed..Box$LT$F$GT$$u20$as$u20$core..ops..function..FnOnce$LT$A$GT$$GT$::call_once::hb93dff57bcf1de1b at boxed.rs:922:8 [opt] frame #36: 0x0000000101ea0902 libspo2.dylib`std::sys::unix::thread::Thread::new::thread_start::h965e643076cc7fbb [inlined] std::sys_common::thread::start_thread::ha1aa535fbfc4fb6c at thread.rs:13 [opt] frame #37: 0x0000000101ea088e libspo2.dylib`std::sys::unix::thread::Thread::new::thread_start::h965e643076cc7fbb at thread.rs:79 [opt] frame #38: 0x00007fff730802eb libsystem_pthread.dylib`_pthread_body + 126 frame #39: 0x00007fff73083249 libsystem_pthread.dylib`_pthread_start + 66 frame #40: 0x00007fff7307f40d libsystem_pthread.dylib`thread_start + 13 ```

[gavrie]

@Kerollmops thanks for the detailed report!

Regarding the use of a feature to enable the Redis allocator: That's a good idea; I'll look at it.

Regarding the use of the System allocator fallback: Good point; I agree that it it should fall back to the Global allocator and not the system one.

Regarding the crash:

A while ago, I also encountered a crash related to the allocator. It seems similar, but I haven't dived into the details of your crash yet. It was related to the use of std::io (via println!) before switching the allocator to the Redis one; I guess there is something in the std::io implementation that allocates an internal buffer or such that can be reallocd at a later stage.

What I did to solve it is make sure that no std::io calls were performed before switching the allocator. This was fixed by 2c37545 and 08d8fbc.

Does your crash still happen with current master?

[gavrie]

@Kerollmops Update: It seems like your version already included these commits, so that may not be the root cause.

[Kerollmops]

Thank you for the reply,

I do not think it would be possible to fallback on the global allocator as it is RedisAlloc once declared.

Notice that the crash is not related to the allocator in any way because I disabled it on my repository.

[gavrie]

Due to the recent changes (#68) this is probably no longer relevant so I'm closing it. Please reopen if it is still relevant.