Warn users of potentially undefined behaviour when using references

[stefnotch]

We looked at the ash examples and documentation, read the warnings about unsafe, and figured "as long as we imitate them, and don't do anything crazy, everything should be fine".

Turns out that we probably ran into undefined behaviour rather quickly. We clearly missed a memo about what to do and what to avoid.

So we wrote the following code

unsafe { device.end_command_buffer(setup_command_buffer) }
        .expect("Could not end command buffer");

let submit_info = vk::SubmitInfo::builder()
        .command_buffers(&[setup_command_buffer])
        .build();

unsafe { device.queue_submit(queue, &[submit_info], vk::Fence::null()) }
        .expect("Could not submit to queue");

unsafe { device.device_wait_idle() }.expect("Could not wait for queue");

https://github.com/stefnotch/rtr23-round-cat/blob/03aba602044cf64fd50d13f6810cf17116946371/src/scene_uploader.rs#L311-L322

and were very confused when it suddenly started crashing out of nowhere. Turns out that the submit_info has a pointer that points at...this [setup_command_buffer]. A single element array with the setup_command_buffer. And since that array is only passed to the command_buffers method, Rust can get rid of it right afterwards.

So submit_info ends up with an illegal pointer. And later, when we're using it device.queue_submit(queue, &[submit_info], vk::Fence::null()), then of course the SubmitInfo.p_command_buffers points at garbage.

Could this maybe be mentioned in the documentation or examples? The whole &[something] pattern is used very often, but sometimes, the array is created in a separate line of code. And it's hard for a novice to know when to do what.

[stefnotch]

A bit of searching and reproducing later, we realised what the issue is. If I understood it correctly, this is roughly what happens

fn main() {
    // Create a command buffer
    let command_buffers = CommandBuffer {
        foo: "Hello".to_string(),
    };

    // Create a submit info, and pass it a reference to a newly created command_buffer array
    // Ash internally does *not* keep a reference to the array. Instead it saves a *pointer* to the command buffer array.
    let submit_info = SubmitInfo::with_command_buffers(&[command_buffers]);

    // And here, the command_buffers is still valid.
    // However, the [command_buffers] array is not. It's a temporary array that was already dropped.

    // And call the unsafe queue_submit
    // This is where undefined behaviour happens in this particular example, I believe
    // After all, we misused the Ash API
    queue_submit(&[submit_info]);

    
    println!("Bye!");
}



// Ash has a command buffer https://docs.rs/ash/latest/ash/vk/struct.CommandBuffer.html
// For this example, we'll use this dummy
struct CommandBuffer {
    foo: String,
}

// And we'll use this simplified submit info
// Notice how it has a pointer. Nice
#[repr(C)]
#[derive(Copy, Clone)]
struct SubmitInfo {
    command_buffer_count: u32,
    p_command_buffers: *const CommandBuffer,
}

impl SubmitInfo {
    pub fn with_command_buffers<'a>(command_buffers: &'a [CommandBuffer]) -> Self {
        Self {
            // Take the pointer
            command_buffer_count: command_buffers.len() as _,
            p_command_buffers: command_buffers.as_ptr(),
        }
    }
}

// And when queue_submit is called, we take that pointer and look at what's there
fn queue_submit(submit_info: &[SubmitInfo]) {
    let command_buffers = unsafe {
        std::slice::from_raw_parts(
            submit_info[0].p_command_buffers,
            submit_info[0].command_buffer_count as _,
        )
    };
    println!("submits = {:?}", command_buffers[0].foo);
}

[MarijnS95]

This information is definitely in the readme for the latest release that you are using:

Calling .build() will discard that lifetime because Vulkan structs use raw pointers internally. This should be avoided as much as possible because this can easily lead to dangling pointers.

But has been removed from this repo as it is no longer relevant since .build() has been removed entirely. The code sample you provided will no longer compile, and the borrow checker will tell you exactly that the temporary &[] slice is going out of scope.

[stefnotch]

Ah, I see, so the issue will go away as soon as we use the latest version of Ash. Thank you very much!

[MarijnS95]

Unfortunately there is no release with this yet. It is a breaking change and requires a breaking (per semver) release, but we found more technical issues that also require a semver-breaking release to solve, but the solution hasn't materialized.

For your second comment, I think we lost this useful comment from the readme:

To not lose this lifetime single items can be "cast" to a slice of length one with std::slice::from_ref while still taking advantage of Deref:

The Deref part is no longer relevant (because we removed builder structs entirely) , but std::slice::from_ref() is still very useful to solve the "size-one temporary slice" going out of scope without constructing a longer-lasting size-one array on a separate line with a new let binding.