Undefined behavior in conv::map_format

[aloucks]

Passing in a VkFormat that hal doesn't yet support results in Illegal instruction (core dumped) on linux, presumably because the enum result is not valid.

https://github.com/gfx-rs/portability/blob/5796b6b158f01311c24f01184d96b850eaf967e4/libportability-gfx/src/conv.rs#L186-L195

It looks like many of the mem::transmutes that convert to Rust enums are unsound. I didn't do an exhaustive search, but I did find these: https://github.com/gfx-rs/portability/commit/694474f46c5d8225107019ebf7650a5fa68d7241

[grovesNL]

You're right – when going from Vulkan enum values to gfx-hal we should probably map each variant instead. It should be fine to transmute when going from gfx-hal enum values to Vulkan though.

Alternatively we could have do a bounds check around the supported range if the enum values are contiguous, but I doubt there's any benefit from doing that.

[kvark]

If the format is unknown to gfx-hal, wouldn't its value be above format::NUM_FORMATS ? My understanding is that our formats map 1:1 up to NUM_FORMATS constant. Summoning @msiglreith to double check.

[aloucks]

I iterate over all the formats (that were known to ash a few months ago) and collect their properties in one go during instance creation. It blows up on G8B8G8R8_422_UNORM.

[msiglreith]

@kvark yes, that's right.

@aloucks Are you sure that this is not caused by hitting unimplemented!() here? Nonetheless more graceful handling would be good.

(The issue is similar to https://github.com/gfx-rs/portability/issues/22)

[kvark]

The value for this format is "1000156001", which is clearly hitting our unimplemented!() path

[aloucks]

I've replaced the unimplemented!() path with None and it still fails the same way. I've hit other unimplemented blocks and always get a nice panic message on those.

I'm honestly not sure what's going on but the issue happens after querying ASTC_12X12_SRGB_BLOCK, which happens to be the last hal format.

#[inline]
pub extern "C" fn gfxGetPhysicalDeviceFormatProperties(
    adapter: VkPhysicalDevice,
    format: VkFormat,
    pFormatProperties: *mut VkFormatProperties,
) {
    println!("ENTER: gfxGetPhysicalDeviceFormatProperties({:?}, {:?})", adapter, format);
    let properties = adapter.physical_device.format_properties(conv::map_format(format));
    unsafe {
        *pFormatProperties = conv::format_properties_from_hal(properties);
    }
    println!("LEAVE: gfxGetPhysicalDeviceFormatProperties");
}

I'm not sure where the UB is being triggered. It still panics even with the unimplemented and transmutes removed.

pub fn map_format(format: VkFormat) -> Option<format::Format> {
    println!("map_format");
    println!("map_format({:?})", format);
    if format == VkFormat::VK_FORMAT_UNDEFINED {
        None
    } else if (format as usize) < format::NUM_FORMATS {
        println!("transmute");
        // HAL formats have the same numeric representation as Vulkan formats
        //let value = unsafe { mem::transmute(format) };
        //println!("value: {:?}", value);
        //Some(value)
        Some(format::Format::Rgba4Unorm)
    } else {
        println!("unimplemented");
        //unimplemented!("Unknown format {:?}", format);
        None
    }
}

And the calling code:

static VK_FORMATS: &'static [vk::Format] = &[
    vk::Format::UNDEFINED,
    vk::Format::R4G4_UNORM_PACK8,
    ...
    vk::Format::ASTC_12X10_SRGB_BLOCK,
    vk::Format::ASTC_12X12_UNORM_BLOCK,
    vk::Format::ASTC_12X12_SRGB_BLOCK,
    vk::Format::G8B8G8R8_422_UNORM,
    vk::Format::B8G8R8G8_422_UNORM,
    ...
];

If i adjust the take() so that G8B8G8R8_422_UNORM is never queried for, the UB is not triggered.

for format in VK_FORMATS.iter().take(500).cloned() {
    println!("instance.raw.get_physical_device_format_properties({:?})", format);
    let format_properties = unsafe {
        instance
            .raw
            .get_physical_device_format_properties(physical_device, format)
    };
    println!("returned from: {:?}", format);
    physical_device_format_properties.push((format, format_properties));
}

Logs:

ENTER: gfxGetPhysicalDeviceFormatProperties(Handle(0x55e88b104640), VK_FORMAT_ASTC_12x12_UNORM_BLOCK)
map_format
map_format(VK_FORMAT_ASTC_12x12_UNORM_BLOCK)
transmute
LEAVE: gfxGetPhysicalDeviceFormatProperties
returned from: ASTC_12X12_UNORM_BLOCK
instance.raw.get_physical_device_format_properties(ASTC_12X12_SRGB_BLOCK)
ENTER: gfxGetPhysicalDeviceFormatProperties(Handle(0x55e88b104640), VK_FORMAT_ASTC_12x12_SRGB_BLOCK)
map_format
map_format(VK_FORMAT_ASTC_12x12_SRGB_BLOCK)
transmute
LEAVE: gfxGetPhysicalDeviceFormatProperties
returned from: ASTC_12X12_SRGB_BLOCK
instance.raw.get_physical_device_format_properties(G8B8G8R8_422_UNORM)
error: process didn't exit successfully: `/home/ubuntu/git/vki/target/debug/deps/binding-81298beb7b9cd496 --nocapture --test-threads=1` (signal: 4, SIGILL: illegal instruction)

One last note.. I'm using opt-level = 2

Edit: same result with opt-level = 0