`Handle::from_raw` can't pass miri tests with strict provenance due to integer to pointer cast

[06393993]

With the following test:

    #[test]
    fn test_from_raw() {
        let _ = <vk::Instance as vk::Handle>::from_raw(20);
    }

and the command to run miri test: MIRIFLAGS="-Zmiri-tree-borrows -Zmiri-backtrace=full" cargo +nightly miri nextest run --all-targets --all-features -j20 --no-fail-fast -E'test(test_from_raw)' (with miri and cargo-nextest set up), I end up with the following error:

    test vk::tests::test_from_raw ... 
  stderr ───
    error: unsupported operation: integer-to-pointer casts and `ptr::with_exposed_provenance` are not supported with `-Zmiri-strict-provenance`
       --> ash/src/vk/macros.rs:142:22
        |
    142 |                   Self(x as _)
        |                        ^^^^^^ integer-to-pointer casts and `ptr::with_exposed_provenance` are not supported with `-Zmiri-strict-provenance`

I am writing Vulkan layers in Rust, use vk::Handle::from_raw extensively, and use miri with tree borrows to catch UB for my unsafe code. Therefore, I am wondering if the upstream is willing to accept a PR that removes the integer to pointer casting in vk::Handle::from_raw.

I have 2 different proposals with different drawbacks:

• Change the definition of a handle type, e.g. vk::Instance to struct Instance(usize). The disadvantage is that:

  • The implementation of null(e.g. ash::vk::Instance::null) has to change. We either change the method from a const fn to a non const fn with Self(::core::ptr::null_mut::<u8>() as usize), or we use Self(0). Dropping const will break the API slightly, while using 0 is not great because it is not guaranteed that C++'s nullptr or C's NULL is 0.
  • While usize should be the same size as *mut u8 on platforms supported by Vulkan, so that ABI should be kept. However, I am not sure if this is strongly guaranteed by Rust.

• Use ::core::ptr::with_addr to implement Handle::from_raw, i.e., ::core::ptr::dangling_mut::<u8>().with_addr(x). However, with_addr is not introduced until Rust 1.84.0, so we either need to change MSRV or use crates like rustversion for conditional compilation based on the Rust version which adds new dependencies and complicates code.

[Ralith]

Both of these proposed solutions seem to have the effect of hiding the provenance issue rather than trying to address it. The latter is probably outright UB because you're claiming the pointers are dangling when they aren't, and perhaps you even dereference them later.

What's leading you to use from_raw extensively in the first place? Could you make broader use of the handle types instead? Alternatively, maybe what you really need is provenance-aware APIs for handles?

[06393993]

Both of these proposed solutions seem to have the effect of hiding the provenance issue rather than trying to address it. The latter is probably outright UB because you're claiming the pointers are dangling when they aren't, and perhaps you even dereference them later.

I think the spirit is that one should never create reference from the raw handle or dereference it, because ultimately this handle is returned from a C FFI, the Vulkan loader, and Rust can't have any provenance here: the memory is allocated by the Vulkan loader.

What's leading you to use from_raw extensively in the first place? Could you make broader use of the handle types instead?

As I mentioned, I am writing a Vulkan layer and dealing with the loader layer interface described in https://github.com/KhronosGroup/Vulkan-Loader/blob/main/docs/LoaderLayerInterface.md. To test my implementation, I need to mock the behavior of the loader + the ICD, e.g. vkCreateDevice. To construct a Vulkan handle in my mock function, e.g., ash::vk::Instance in vkCreateInstance, I need to useHandle::from_raw

Alternatively, maybe what you really need is provenance-aware APIs for handles?

Sure, that also works with the same disadvantage that we need to conditionally compile to keep the MSRV. If we prefer this solution, I can come up with a different change.

[Ralith]

Rust can't have any provenance here: the memory is allocated by the Vulkan loader.

I haven't reviewed tree borrows in too much depth recently, but I'm not sure "allocation obtained over FFI" is interchangeable with "dangling pointer". The former describes malloc, for one thing.

To construct a Vulkan handle in my mock function, e.g., ash::vk::Instance in vkCreateInstance, I need to useHandle::from_raw

Could you back them with a real memory allocation, and transmute from a pointer rather than using from_raw? That way we avoid passing through integers. Maybe from_raw should ultimately accept a pointer type (which would also enable use of strict provenance APIs without requiring conditional compilation), but this approach could unblock you immediately.

[06393993]

Could you back them with a real memory allocation, and transmute from a pointer rather than using from_raw?

Thanks. transmute does work.

Maybe from_raw should ultimately accept a pointer type

Are we Ok with adding such capability right away?

We can either change from_raw to a generic function like this:

trait AsU8Pointer {
    fn as_ptr(self) -> *mut u8;
}

impl AsU8Pointer for u64 {
    fn as_ptr(self) -> *mut u8 {
        self as *mut u8
    }
}

impl<T> AsU8Pointer for *mut T {
    fn as_ptr(self) -> *mut u8 {
        self as *mut u8
    }
}

impl Instance {
    fn from_raw(x: impl AsU8Pointer) -> Self {
        Self(x.as_ptr())
    }
    ...
}

or add another from_raw_ptr function like this:

impl Instance {
    fn from_raw_ptr<T>(x: *mut T) -> Self {
        Self(x as *mut u8)
    }
    ...
}

If we believe we need longer time to consider the necessity of the API or the design, I can close the comment and leave it to the future too.

[Ralith]

Thanks. transmute does work.

Great! This should be fine to rely on, since we guarantee the representation of these types anyway.

We can either change from_raw to a generic function like this: [...]

I figured we'd just change the signature of the existing function to take a *mut u8 or similar. There's not much value to having both, and we've got a breaking release coming up anyway.

[06393993]

Do we care to change the as_raw to return *mut u8 for consistency or potential dereference use case as well? I personally don't think we should do that. Dereferencing the dispatchable object handles as a void ** in C/C++ layer development is common:

https://github.com/LunarG/VulkanTools/blob/640f35dd85d25122371fa99b8788d944b71bc5e8/layersvt/monitor.cpp#L243

VKAPI_ATTR VkResult VKAPI_CALL vkQueuePresentKHR(VkQueue queue, const VkPresentInfoKHR *pPresentInfo) {
    monitor_layer_data *my_data = GetLayerDataPtr(get_dispatch_key(queue), layer_data_map);

https://github.com/LunarG/VulkanTools/blob/640f35dd85d25122371fa99b8788d944b71bc5e8/layersvt/vk_layer_table.cpp#L28

dispatch_key get_dispatch_key(const void *object) { return (dispatch_key) * (VkuDeviceDispatchTable **)object; }

https://github.com/LunarG/VulkanTools/blob/640f35dd85d25122371fa99b8788d944b71bc5e8/layersvt/vk_layer_table.h#L35

typedef void *dispatch_key;

I don't think changing Handle::as_raw is worthwhile at all, because existing ash code already relies on Handle::as_raw returning u64:

https://github.com/ash-rs/ash/blob/55dc692946f52f0dff326bd13b1722d98f52fd2b/ash/src/device.rs#L108-L122

This is the Vulkan spec of vkSetPrivateData for reference. vkSetPrivateData does expect u64 as the 3rd parameter.

And just in case you miss it, the PR #997 is ready for review.

[Ralith]

Ah, right, we used u64 here because it allows for a uniform interface with non-dispatchable handles. Probably best to keep that. Would a separate, inherent method work for your purposes, or is a trait method specifically useful?

[06393993]

A trait method is specifically useful, because my use case tries to handle all the dispatchable object. However, I don't feel strong on it, as I can definitely work around by introducing my own trait type outside ash.

[Ralith]

I'm not opposed to having something like a trait DispatchableHandle: Handle { fn from_raw(x: *mut u8); }, in that case. On the one hand it's a small amount of additional trivial code with a clear use case, though on the other it doesn't seem terribly important since there's an adequate down-stream solution and the use case is a bit niche. Feel free to close this issue if you're happy as-is, or otherwise revise the open PR into that form and we can see what @MarijnS95 thinks?

[06393993]

Sure, thanks. I have revised the PR because a new interface at least help me get rid of unnecessary unsafe transmute_copy. Let's wait for the final decision then.