alexheretic / ktx

Rust KTX texture storage format parsing
Apache License 2.0
11 stars 5 forks source link

Panic when reading a ktx file #9

Closed ghost closed 3 years ago

ghost commented 3 years ago

This code causes a segmentation fault:

fn main() {
    let ktx_bytes = std::fs::read(local_file!("street.ktx")).unwrap();
    let texture = ktx::Ktx::new(ktx_bytes.as_slice());
    for data in texture.textures() {
        trace!(data.len());
    }
    trace!(texture);
}

streets.ktx: https://github.com/KhronosGroup/Vulkan-Samples-Assets/blob/177aea4beaa264e6e9823c80d9d0460e61db222f/textures/uffizi_rgba16f_cube.ktx

alexheretic commented 3 years ago

I don't recognise the local_file! stuff. I'd be surprised to see a seg fault from this crate as there is no unsafe code here.

I can reproduce a panic from Ktx<&[u8]>.

// [dependencies]
// ktx = "0.3.1"

fn main() {
    let texture = ktx::include_ktx!("../uffizi_rgba16f_cube.ktx");

    dbg!(texture);

    for data in texture.textures() {
        dbg!(data.len());
    }
}
[src/main.rs:7] texture = Ktx {
    header: KtxHeader {
        big_endian: false,
        gl_type: 5131,
        gl_type_size: 2,
        gl_format: 6408,
        gl_internal_format: 34842,
        gl_base_internal_format: 6408,
        pixel_width: 512,
        pixel_height: 512,
        pixel_depth: 0,
        array_elements: 0,
        faces: 6,
        mipmap_levels: 10,
        bytes_of_key_value_data: 0,
    },
}
[src/main.rs:10] data.len() = 2097152
thread 'main' panicked at 'range end index 798044278 out of range for slice of length 16777304', /home/alex/.cargo/registry/src/github.com-1ecc6299db9ec823/ktx-0.3.1/src/slice.rs:102:19
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

A bug in the texture level index calculation maybe?

Whereas the read at runtime path (more similar to your example) seems to work fine.

// [dependencies]
// ktx = "0.3.1"

use std::{fs::File, io::BufReader};

fn main() {
    let ktx_file = BufReader::new(File::open("uffizi_rgba16f_cube.ktx").unwrap());
    let texture = ktx::Decoder::new(ktx_file).unwrap();

    dbg!(texture.header());

    for data in texture.read_textures() {
        dbg!(data.len());
    }
}
[src/main.rs:10] texture.header() = KtxHeader {
    big_endian: false,
    gl_type: 5131,
    gl_type_size: 2,
    gl_format: 6408,
    gl_internal_format: 34842,
    gl_base_internal_format: 6408,
    pixel_width: 512,
    pixel_height: 512,
    pixel_depth: 0,
    array_elements: 0,
    faces: 6,
    mipmap_levels: 10,
    bytes_of_key_value_data: 0,
}
[src/main.rs:13] data.len() = 2097152
[src/main.rs:13] data.len() = 14680080
alexheretic commented 3 years ago

It looks like the texture level index info is buggy in that file, it points to a large index bigger than the file. The runtime path automatically caps at eof so doesn't panic, whereas the static/include_ktx! path gets an index panic.

10 fixes the latter to cap at the max data len.

I'm not sure if this file corrupt or is this a ktx2 thing or spec change that isn't being handled.

ghost commented 3 years ago

I'd be surprised to see a seg fault from this crate

My bad, it was panicking.

I tried the patched library and the code doesn't panick anymore, but texture.textures() returns just two items, even though the image has 6 faces and 10 miplevels. Am I correct when I say that there should be 60 textures inside this image? I don't know much about ktx file format.

ghost commented 3 years ago

This is the code that uses this ktx file: https://github.com/KhronosGroup/Vulkan-Samples/tree/master/samples/api/hdr

alexheretic commented 3 years ago

Thanks, I'd guess the file isn't corrupt and instead is ktx2/different spec to what is implemented.

This needs a little investigation and some new test cases.

On Wed, 28 Jul 2021, 13:41 fctorial, @.***> wrote:

This is the code that uses this ktx file: https://github.com/KhronosGroup/Vulkan-Samples/tree/master/samples/api/hdr

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/alexheretic/ktx/issues/9#issuecomment-888276003, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARZHVZPA64QSDIEKRV5WG3TZ73GTANCNFSM5BDDOROQ .

alexheretic commented 3 years ago

Ah it looks like this is just the lacking support for ktx1 6-face textures. #5 added support for this with a bunch of other stuff but it was never merged.

I'll pull out the 6-face support and get that merged in, it should fix this case.

alexheretic commented 3 years ago

Thanks for raising this. I've put in non-breaking support for cubemap texture levels. For cubemaps each level will contain all 6 faces in order: +X, -X, +Y, -Y, +Z, -Z.

I'll put a new release out at the end of the week

ghost commented 3 years ago

Great, thanks.