`Device::from_native` is unsafe

[Wybxc]

The signature of Device::from_native does not constrain the relationship between the lifetimes of its input parameters and the lifetime of its return value, which may result in dangling references.

For example:

let mut custom = CustomDevice(...);
let device = Device::from_native(&mut custom);
drop(custom);
// Here `device` still holds a pointer to `custom`

One solution is to add a PhantomData containing NativeDevice to the Device to inform the compiler of the implicit lifetime requirements here.

struct Device<D> {
    // ...
    phantom: std::marker::PhantomData<D>,
}

[ginnyTheCat]

My understanding is that adding a 'static bound on NativeDevice in Device::from_native and native::create would make this safe.

This therefore wouldn't allow passing in mutable or immutable references anymore, so passing in a Rc like in the native_device_drop test would be the "only" option if the result of the computation wants to be accessed from the outside. That would be two levels of reference counting (once the fz_device::refs on the C side + the Rc on the Rust side) but adding a Rc::into_inner requivalent on Device to recover the NativeDevice would be more difficult as we don't know Devices underlying type anymore at that point. I think overall even without this it's still a pretty good API surface to work with.

[Wybxc]

The discussion from #162 is continued here.

Device is reference-counted in C, which means the approach proposed in #162, using the lifetime of NativeDevice to mark the lifetime of Device, is not feasible. If a reference to non-'static NativeDevice is duplicated in C, even if its corresponding Device in Rust has the correct lifetime, other references in C might still have a longer lifetime, potentially leading to dangling references.

Specifically, look at this example:

let mut custom = CustomDevice(...);
let device = Device::from_native(&mut custom);
// Do something with `device` ...
drop(device);
drop(custom);

Here device holds a pointer to a fz_device, which holds a pointer (reference) to the Rust struct custom (note that D is &mut T in the code below):

mupdf-rs/src/device/native.rs

Lines 365 to 367 in c425562

	let c_device: *mut CDevice<D> =
	ffi_try!(mupdf_new_derived_device(context(), c"RustDevice"))?;
	ptr::write(&raw mut (*c_device).rust_device, device);

When MuPDF operates on the device, the pointer inside it may be copied (as it is reference-counted). This copied pointer could outlive both the device and the custom data structures. There is no guarantee that MuPDF will not store the device pointer and use it later.

The only safe way to handle this situation is to require that NativeDevice must be 'static. Non-'static data stored behind a reference-counted pointer in C can lead to dangling references, as is analyzed aboew.

[Wybxc]

This therefore wouldn't allow passing in mutable or immutable references anymore, so passing in a Rc like in the native_device_drop test would be the "only" option if the result of the computation wants to be accessed from the outside.

That's true. I will close #162 and open a new Pull Request to implement this fix.