gfx-rs / wgpu

A cross-platform, safe, pure-Rust graphics API.
https://wgpu.rs
Apache License 2.0
12.49k stars 914 forks source link

Naga snapshot tests should not include binary files #6332

Open jimblandy opened 1 month ago

jimblandy commented 1 month ago

The Naga snapshot tests should be changed to take SPIR-V input as assembly language, not binary files. Alternatively, CI should disassemble the binary SPIR-V files and assert that they match accompanying source files.

The xz utils backdoor concealed the portion of its code that should have aroused suspicion as test input for the decompression library. Binary data discourages review in a way that textual data does not. (As evidence for this, recent PRs from perfectly trustworthy contributors to wgpu have included .spv input files that didn't match the disassembly present alongside them in the PR. These problems were caught in PR review, but the author themselves hadn't noticed.)

In wgpu, the naga/tests/in/spv/ directory contains various .spv binary files. There are a few approaches:

cwfitzgerald commented 1 month ago

I don't see easy bindings to spirv-as, but shelling out likely isn't too costly, and it's not unreasonable to expect the vulkan sdk installed and in-path.

evilpie commented 1 month ago

Rust does have the advantage compared to C code (including xz) that the build system is a lot more sane and it's harder to hide some nefarious instructions in there. It would be nice if wgpu didn't use build.rs at all, but the current usage is at least very easy to audit.

Imberflur commented 4 weeks ago

Some thoughts on this: