Convert *mut *mut f64 to Box<f64>

[jamwaffles]

I've got some code that allocates some memory using the bindgen-ed C function hal_malloc which returns an *mut *mut c_void. I cast this to *mut *mut f64 to get a double pointer to a mutable f64. f64 can be other types in my real application, but they're always scalars if that makes life any easier. I then want to store a reference to that memory in a struct so I can modify it later.

I'm sort of writing a smart pointer where the allocator is external to the program I guess?

Here's the broken code:

struct Wrapper {
    ptr: Box<f64>
}

impl Wrapper {
    pub fn new() -> Self {
        let ptr = hal_malloc(mem::size_of::<f64>().try_into().unwrap()) as *mut *mut f64;

        Wrapper {
            ptr: unsafe { Box::from_raw(*ptr) }
        };
    }

    pub fn set_value(&mut self, value: f64) {
        // ...
    }

    pub fn get_value(&self) {
        // ...
    }
}

What I would like to do for safety's sake is convert *mut *mut f64 to some kind of mutable Box<f64>. I can do this with Box::from_raw(*ptr) as shown above, however that no longer mutates the original hal_malloced memory, which I think is to be expected because I deref ptr in Box::from_raw?

My two questions:

1. Does using Box make reading/writing to the original memory any safer in set_value and get_value?
2. If so, how do I cast my double-star (**) pointer into a mutable Box when I create it?

[alice]

If you didn't allocate the memory with a Box, I definitely wouldn't turn it into a Box. I'm also confused at to why it's a double pointer?

[RustyYato]

@jamwaffles

Does using Box make reading/writing to the original memory any safer in set_value and get_value ?

No, unsafety infects the entire module. Using Box would actually be worse because you may drop the box and deallocate the memory with a different allocator.

Just keep it as a *mut *mut f32 and implement Deref and DerefMut to easily access the inner value. This is fine because Wrapper is basically a smart pointer.

[fpagliughi]

I agree that a Box is the wrong way to go, and am also confused by why an allocation function is returning a pointer to a pointer?!?

But I would also look for a guarantee that it's returning a memory address that's properly aligned for an f64 on your system. A mis-aligned float could cause some issues depending on your platform.

[jamwaffles]

I took a closer look at hal_malloc and it does indeed return *mut f64 so my as screwed that up. Thanks for prompting me to check that! However, I have another unsafe fn that needs to use the pointer returned, but as *mut *mut f64 (actually a double pointer this time!) My demo code now looks like this:

use std::mem;
use std::ffi::c_void;
use std::convert::TryInto;

extern "C" {
    fn hal_malloc(size: i64) -> *mut c_void;
    fn hal_modify_thing(pointer: *mut *mut f64);
}

struct Wrapper {
    ptr: *mut f64
}

impl Wrapper {
    pub fn new() -> Self {
        let ptr = unsafe { hal_malloc(mem::size_of::<f64>().try_into().unwrap()) as *mut f64 };

        unsafe { hal_modify_thing(ptr as *mut *mut f64) };

        Wrapper {
            ptr
        }
    }

    pub fn set_value(&mut self, value: f64) {
        // ...
    }

    pub fn get_value(&self) {
        // ...
    }
}

This compiles, but the value in ptr is never mutated. If I go back to coercing hal_malloc() to *mut *mut f64, it's now mutated again. What am I missing?

playground link

[jamwaffles]

Thanks for the suggestion. How can I check the alignment? A oneliner or pointer (haha) to a blog post would be cool

[RustyYato]

You can use ptr.cast::<u8>().align_offset(align) == 0 to check alignment.

Or if you want a more direct check

fn is_aligned_to<T: ?Sized>(ptr: *const T, align: usize) -> bool {
    assert!(align.is_power_of_two());
    let ptr = ptr as *const u8 as usize;
    let mask = align.wrapping_sub(1);
    (ptr & mask) == 0
}

[jamwaffles]

Thanks for the extra bit of security. I'm now checking pointer alignment which is all ok. Still confused about the ** weirdness but I think I'll leave it for now with some comments around it to explain why it's like that. There are a few other moving parts I'm not showing which might be causing some weirdness. For anyone interested, this is all happening around linuxcnc-hal-sys and (eventually) linuxcnc-hal

[fpagliughi]

Typically a C function that takes a pointer to a pointer does so because it will (may) modify the pointer itself.

@jamwaffles

fn hal_modify_thing(pointer: *mut *mut f64);

This function is likely asking you to send the address of your pointer in case it wants to change it. That means the value of your pointer (i.e. the address that it points to) may well be different on return than on entry.

What it's likely expecting is:

unsafe { hal_modify_thing(&mut ptr) };

That also means that since you're saying that you might change your pointer, it must also be declared mutable:

let mut ptr = unsafe { hal_malloc(...) as *mut f64 };

Oh, and I'm not sure if you're example is just to get the call signatures correct. If you're actually mutating a pointer to a buffer that you just allocated, are you leaking memory? If not, you probably need to implement Drop for your wrapper to free the memory.

[jamwaffles]

What it's likely expecting is:

...

hal_modify_thing definitely expects a double pointer. The actual C function is here, and is bindgen-ed into hal_pin_float_new(... ptr as *mut *mut f64, ...). I'm not sure I can get bindgen to change the signature, but I believe that's the correct conversion anyway?

This function is likely asking you to send the address of your pointer in case it wants to change it.

Here's some more code with your suggestions applied:

unsafe {
    let mut storage = linuxcnc_hal_sys::hal_malloc(
        std::mem::size_of::<f64>().try_into().unwrap()
    ) as *mut f64;

    hal_pin_float_new(
        /* extra args here */
        &mut storage,
        /* more args here */
    )
};

This unfortunately doesn't work, spewing this error from the C code. I think this confirms your comment where the pointer is changed (I'm not great at reading C). How do I fix this?

If you're actually mutating a pointer to a buffer that you just allocated, are you leaking memory? If not, you probably need to implement Drop for your wrapper to free the memory.

Good point. I believe it's enough to call the magical hal_exit() function at the end of the program but I need to confirm that it frees any shared memory too.

Thanks everyone for bearing with me working through this XY problem!

[fpagliughi]

Oh, man. This is a painful API. The pointer to the value must reside in shared memory?!? So I guess that errors out because your pointer is on the Rust stack? So, do you need to allocate memory for the pointer from the library?!?

You're in deep.

[mbrubeck]

The example code for that crate simply does this:

let storage = hal_malloc(mem::size_of::<f64>() as i64) as *mut *mut f64;

let ret = hal_pin_float_new(
    pin_name.as_ptr() as *const i8,
    hal_pin_dir_t_HAL_IN,
    storage,
    id,
);

If my reading of the docs (and of examples of C code using this library) is correct, this is slightly wrong. The hal_malloc call should be allocating space for a pointer, not for a float, so it should be:

let storage = hal_malloc(mem::size_of::<*mut f64>() as i64) as *mut *mut f64;

(On platforms where f64 and *mut f64 have the same size, this mistake ends up not mattering.)

At this point, you have allocated space for a pointer (a *mut f64), and you have the address of this pointer (*mut *mut f64), but the pointer itself is uninitialized; nobody has given it an f64 to point to.

Then the hal_pin_float_new takes the address of the pointer and writes a new, valid pointer to that address. Now the address points to a *mut f64 which in turn points to a valid f64 which corresponds to the new "pin" object.

[mbrubeck]

This C file is another example of the same API. Translated into Rust, it would start off something like this:

struct HalData {
    d1: *const f64,
    d2: *const f64,
    d3: *const f64,
    d4: *const f64,
    d5: *const f64,
    d6: *const f64,
}

let comp_id = hal_init(b"scarakins\0".as_ptr().cast());
let hal_data = hal_malloc(size_of::<HalData>() as i64) as *mut HalData;

let ret = hal_pin_float_new(
    b"scarakins.D1\0".as_ptr().cast(),
    hal_pin_dir_t_HAL_IO,
    &mut hal_data.d1,
    comp_id,
);
if ret != 0 { panic!() }

let ret = hal_pin_float_new(
    b"scarakins.D2\0".as_ptr().cast(),
    hal_pin_dir_t_HAL_IO,
    &mut hal_data.d2,
    comp_id,
);
// ...

In this example we can more clearly see that we first allocate space for all the pointers we need, and then call hal_pin_float_new to initialize the pointers, one at a time.

[jamwaffles]

If my reading of the docs ... this is slightly wrong.

I wrote them. Thanks for not ragging on me too hard

At this point, you have allocated space for a pointer (a *mut f64 ), and you have the address of this pointer ( *mut *mut f64 ), but the pointer itself is uninitialized; nobody has given it an f64 to point to.

That makes sense now! I think I've been reading the C examples incorrectly with the structs using *field instead of field then. I've changed the hal_malloc and pin code to this:

let storage =
    hal_malloc(mem::size_of::<*mut f64>() as i64) as *mut *mut f64;

let ret = hal_pin_float_new(
    pin_name.as_ptr().cast(),
    hal_pin_dir_t_HAL_IN,
    storage,
    id,
);

Which works now, but please tell me if the usage of storage in hal_pin_float_new is incorrect. It should be as the types are the same, but the as escape hatch has obviously bitten me previously.

I had to modify your struct example slightly to get it to compile (mostly just following and correcting errors here). Is this correct, with the usage of &mut (*hal_data).field?

struct HalData {
    d1: *mut f64,
    d2: *mut f64,
}

let comp_id = hal_init(b"scarakins\0".as_ptr().cast());
let hal_data = hal_malloc(std::mem::size_of::<HalData>() as i64) as *mut HalData;

let ret = hal_pin_float_new(
    b"scarakins.D1\0".as_ptr().cast(),
    hal_pin_dir_t_HAL_IO,
    &mut (*hal_data).d1,
    comp_id,
);
if ret != 0 {
    panic!()
}

let _ret = hal_pin_float_new(
    b"scarakins.D2\0".as_ptr().cast(),
    hal_pin_dir_t_HAL_IO,
    &mut (*hal_data).d2,
    comp_id,
);

[fpagliughi]

Ah, yeah. Allocate memory for the pointer. This is all starting to make sense...

[mbrubeck]

Yes, that looks correct.

[jamwaffles]

Awesome, thank you all so much!