Open JOE1994 opened 3 years ago
I initially tried a naive fix of changing result: Vec<i32>
to a zero initialized vector (vec![0_u32; words]
),
which turned out to deteriorate performance :(
read_spirv time: [2.0334 us 2.0352 us 2.0372 us]
change: [+214.74% +215.54% +216.27%] (p = 0.00 < 0.05)
Performance has regressed.
Found 8 outliers among 100 measurements (8.00%)
4 (4.00%) high mild
4 (4.00%) high severe
The above results are from running the simple benchmark below.
use criterion::{black_box, criterion_group, criterion_main, Criterion};
use gfx_auxil::read_spirv;
use std::io::Cursor;
fn bench_read_spirv(c: &mut Criterion) {
c.bench_function("read_spirv", |b| b.iter(|| {
for _ in 0..10 {
let spirv1 = read_spirv(Cursor::new(&include_bytes!("./data/quad.vert.spv")[..])).unwrap();
let spirv2 = read_spirv(Cursor::new(&include_bytes!("./data/quad.frag.spv")[..])).unwrap();
black_box(&spirv1);
black_box(&spirv2);
}
}));
}
criterion_group!(benches, bench_read_spirv);
criterion_main!(benches);
Hello :crab: , we (Rust group @sslab-gatech) found a memory-safety/soundness issue in this crate while scanning Rust code on crates.io for potential vulnerabilities.
Issue Description
https://github.com/gfx-rs/gfx/blob/83a437d34b6f3082217c7ac9a476a1429d7f615c/src/auxil/auxil/src/lib.rs#L72-L82
gfx_auxil::read_spirv()
method creates an uninitialized buffer (result
) and passes it to user-providedRead
implementation. This is unsound, because it allows safe Rust code to exhibit an undefined behavior (read from uninitialized memory).This part from the
Read
trait documentation explains the issue:Suggested Fix
It is safe to zero-initialize the newly allocated buffer before
read_exact()
, in order to prevent user-provided code from accessing old contents of the newly allocated heap memory.Also, there are two nightly features for handling such cases.
Thank you for checking out this issue :+1: