[solved] Help with FFI callback

help
1

Hello,
The following test crashes when I replace the original closure by a simple fn:
https://github.com/jgallagher/rusqlite/blob/master/src/hooks.rs#L286

-        let mut called = false;
-        db.rollback_hook(|| { called = true; });
+        fn my_hook() {
+            println!("rollbacked");
+        }
+        db.rollback_hook(my_hook);
         db.execute_batch("BEGIN; CREATE TABLE foo (t TEXT); ROLLBACK;")
             .unwrap();
-        assert!(called);

$ RUSTFLAGS="-Z sanitizer=address" cargo test --lib --features "hooks"
...
error: process didn't exit successfully: `/Users/gwen/Rust/rusqlite-fork/target/debug/deps/rusqlite-2a824b31a1fd588f` (signal: 11, SIGSEGV: invalid memory reference)

$ lldb /Users/gwen/Rust/rusqlite-fork/target/debug/deps/rusqlite-2a824b31a1fd588f
...
* thread #2, name = 'hooks::test::test_rollback_hook', stop reason = EXC_BAD_ACCESS (code=1, address=0xfffffffffffffff1)
    frame #0: 0x000000010074c4ae libclang_rt.asan_osx_dynamic.dylib`__asan::Allocator::Deallocate(void*, unsigned long, unsigned long, __sanitizer::BufferedStackTrace*, __asan::AllocType) + 62
libclang_rt.asan_osx_dynamic.dylib`__asan::Allocator::Deallocate:
->  0x10074c4ae <+62>: lock
    0x10074c4af <+63>: cmpxchgb %cl, -0x10(%rbx)
    0x10074c4b3 <+67>: jne    0x10074c53e               ; <+206>
    0x10074c4b9 <+73>: leaq   -0x10(%rbx), %rdx
Target 0: (rusqlite-2a824b31a1fd588f) stopped.
(lldb) bt
error: need to add support for DW_TAG_base_type '()' encoded with DW_ATE = 0x7, bit_size = 0
* thread #2, name = 'hooks::test::test_rollback_hook', stop reason = EXC_BAD_ACCESS (code=1, address=0xfffffffffffffff1)
  * frame #0: 0x000000010074c4ae libclang_rt.asan_osx_dynamic.dylib`__asan::Allocator::Deallocate(void*, unsigned long, unsigned long, __sanitizer::BufferedStackTrace*, __asan::AllocType) + 62
    frame #1: 0x00000001007a2cc2 libclang_rt.asan_osx_dynamic.dylib`wrap_free + 386
    frame #2: 0x000000010008d268 rusqlite-2a824b31a1fd588f`alloc::alloc::dealloc::h77ee1d0d581e1e84(ptr="", layout=Layout @ 0x0000700006d3a380) at alloc.rs:80
    frame #3: 0x00000001001aa535 rusqlite-2a824b31a1fd588f`alloc::alloc::box_free::h21ff2406ea0d74cd(ptr=Unique<*mut std::os::raw::c_void> @ 0x0000700006d3a528) at alloc.rs:175
    frame #4: 0x0000000100124c77 rusqlite-2a824b31a1fd588f`core::ptr::drop_in_place::h29c8a0e7cdf39e25((null)=0x0000700006d3a680) at ptr.rs:59
    frame #5: 0x00000001000b702c rusqlite-2a824b31a1fd588f`rusqlite::hooks::free_boxed_hook::h9f42efbbb6e9b992(hook=0x0000000000000001) at hooks.rs:256
    frame #6: 0x00000001001e2a38 rusqlite-2a824b31a1fd588f`rusqlite::hooks::_$LT$impl$u20$rusqlite..InnerConnection$GT$::rollback_hook::h2f7f88fd013325df(self=0x0000700006d3b5e8, hook=Option<fn()> @ 0x0000700006d3a8f0) at hooks.rs:200
    frame #7: 0x000000010000208d rusqlite-2a824b31a1fd588f`rusqlite::hooks::_$LT$impl$u20$rusqlite..InnerConnection$GT$::remove_hooks::h8a45c49a959591c9(self=0x0000700006d3b5e8) at hooks.rs:136
    frame #8: 0x000000010000b291 rusqlite-2a824b31a1fd588f`rusqlite::InnerConnection::close::h3ba0980410dda9e9(self=0x0000700006d3b5e8) at lib.rs:815
    frame #9: 0x000000010000d2f6 rusqlite-2a824b31a1fd588f`_$LT$rusqlite..InnerConnection$u20$as$u20$core..ops..drop..Drop$GT$::drop::h39024442dac241f1(self=0x0000700006d3b5e8) at lib.rs:947
    frame #10: 0x000000010012a755 rusqlite-2a824b31a1fd588f`core::ptr::drop_in_place::hd7479dceb6b58269((null)=0x0000700006d3b5e8) at ptr.rs:59
    frame #11: 0x0000000100128f65 rusqlite-2a824b31a1fd588f`core::ptr::drop_in_place::h976a5bacc8e59bfa((null)=0x0000700006d3b5e8) at ptr.rs:59
    frame #12: 0x000000010012ba99 rusqlite-2a824b31a1fd588f`core::ptr::drop_in_place::hf3cd4c6491081406((null)=0x0000700006d3b5e0) at ptr.rs:59
    frame #13: 0x0000000100123eda rusqlite-2a824b31a1fd588f`core::ptr::drop_in_place::h24cb0efa0158490b((null)=0x0000700006d3b5e0) at ptr.rs:59
    frame #14: 0x000000010013e886 rusqlite-2a824b31a1fd588f`rusqlite::hooks::test::test_rollback_hook::hc4b8828af1b21f0f at hooks.rs:291

Could you please give me some help ?
Maybe the fn should not be boxed ? But then, how to support both closure and fn ?
Thanks.

2

Your free_boxed_hook function is not correct. If you create a box of type T, you also need to deallocate it as a box of type T. You're deallocating everything as Box<*mut c_void>.

The easiest way is to convert your boxed closure to a boxed boxed trait object, and make free_boxed_hook a generic function that you parameterize based on the type of closure that's being freed. You need the double box because the boxed trait object is a fat pointer.

Something like this (untested):

    fn update_hook<F>(&mut self, hook: F)
        where F: FnMut(Action, &str, &str, i64)
    {
        unsafe extern "C" fn call_boxed_closure(p_arg: *mut c_void,
                                                   action_code: c_int,
                                                   db_str: *const c_char,
                                                   tbl_str: *const c_char,
                                                   row_id: i64)
        {
            use std::ffi::CStr;
            use std::str;

            let boxed_hook: *mut F = p_arg as *mut Box<FnMut(Action, &str, &str, i64)>;
            assert!(!boxed_hook.is_null(),
                    "Internal error - null function pointer");

            let action = Action::from(action_code);
            let db_name = {
                let c_slice = CStr::from_ptr(db_str).to_bytes();
                str::from_utf8_unchecked(c_slice)
            };
            let tbl_name = {
                let c_slice = CStr::from_ptr(tbl_str).to_bytes();
                str::from_utf8_unchecked(c_slice)
            };

            (*boxed_hook)(action, db_name, tbl_name, row_id);
        }

        unsafe {
            let previous_hook = {
                let boxed_hook: *mut F = Box::into_raw(Box::new(Box::new(hook) as Box<FnMut(Action, &str, &str, i64)>));
                ffi::sqlite3_update_hook(self.db(),
                                         Some(call_boxed_closure),
                                         boxed_hook as *mut _)
            };
            free_boxed_hook::<FnMut(Action, &str, &str, i64)>(previous_hook);
        }
    }

/// # Unsafe
/// The caller must guarantee to call `free_boxed_hook` with the right type `F` for the pointer that's being passed.
unsafe fn free_boxed_hook::<F>(hook: *mut c_void) {
    if !hook.is_null() {
        drop(Box::<Box<F>>::from_raw(hook as *mut _));
    }
}
1 Like
3

Many thanks for your response.
But I am afraid I cannot fix free_boxed_hook because F should be the type of the previous hook, not the new one...
Anyway, now I know that the current implementation is broken and should be fixed.

4

In my example that's accounted for by converting the hook to a trait object.