Audit de sécurité du code Rust

Audit de sûreté ciblant les blocs unsafe, fuites mémoire, UB et violations de spécification Vulkan dans une base Rust.

Spar Skills Guide Bot
SecuriteAvancé
1007/08/2026
Claude CodeCursorWindsurfCopilotCodex
#safety-audit#rust#unsafe#memory-safety#vulkan

Recommandé pour


description: "Safety audit — unsafe blocks, memory leaks, undefined behavior, Vulkan spec violations"

Safety Audit

Read _audit-common.md (layout, methodology, dedup, report format) and _audit-severity.md (the unified scale + the Special-Rules table this domain leans on heavily) before starting. Do not restate their content here.

Severity anchors for this domain (from _audit-severity.md): FFI lifetime violation = CRITICAL · BLAS/TLAS wrong geometry/address or SSBO index mismatch = CRITICAL · leak that compounds per frame = HIGH · Vulkan spec violation = HIGH · unsafe without a safety comment = MEDIUM.

Scale of the surface

unsafe is concentrated, not scattered: ~760 occurrences live in crates/renderer/src (ash FFI + gpu-allocator), then a long tail — ~11 each in crates/nif and crates/fsr3-sys (the vendored FSR 3.1 FFI, Dimension 1), ~6 in crates/core, 2 in byroredux, and one each in crates/plugin, crates/facegen, crates/cxx-bridge, crates/ui, and crates/pex (the M47.2 decompiler — its single unsafe is a guarded transmute in opcode.rs, see Dimension 2). crates/save (M45) and crates/hkx (M47.2, a deliberately safe packfile reader) have no unsafe. Counts drift — recount with grep -ro unsafe crates/<c>/src | wc -l rather than trusting these figures. Renderer carries roughly nine SAFETY comments per ten unsafe tokens; the residual gap is where the unsafe-without-comment (MEDIUM) findings live. Budget your time accordingly — do not audit the nif/core/pex tail at the expense of the renderer FFI mass. The Dimension-4 sweep greps all of crates/ so pex / save / fsr3-sys / hkx are covered automatically.

Dimensions below are ordered by safety blast radius: FFI lifetime, then memory-corruption/UB, then per-frame leaks, then unsafe-block discipline, then Vulkan-spec compliance, then the narrower regression-guard surfaces.

Dimensions

1. FFI Lifetime Safety (cxx bridge) — CRITICAL class

  • The cxx surface is currently a placeholder. crates/cxx-bridge/src/lib.rs exposes one bridge fn, native_hello() -> String (impl in crates/cxx-bridge/cpp/native_utils.cpp). There is no raw-pointer exchange, no Rust-string-into-C++ borrow, no shared-ownership handoff across the boundary today. Do NOT report speculative "string lifetime / dangling pointer across cxx" findings against this crate — they describe a surface that does not exist yet. The real check here is a scope guard: confirm the bridge still has no owned-pointer / borrowed-slice signatures. The instant a *const, &[u8], Box<…>, or unsafe extern "C++" fn taking a Rust reference appears, this becomes a live CRITICAL-class dimension and the lifetime analysis from _audit-severity applies.
  • unsafe extern "C++" in the bridge marks the C++ side as trusted — verify no new fn returns a pointer Rust then dereferences past the call.
  • crates/fsr3-sys (added 2026-07-22) is a real, live FFI crossing — unlike the cxx-bridge placeholder above, this is not hypothetical. extern "C" functions take *mut RawContext/*const RawCreateDesc/*mut RawVersion etc.; pub unsafe fn Context::create/Context::dispatch carry # Safety doc sections stating caller contracts (device/physical-device/proc-addr must outlive the Context; dispatch handles must belong to the creating device), and Drop calls back into the native shim. Audit every unsafe fn here for a # Safety doc and a lifetime contract the way this dimension used to reserve for a hypothetical live cxx-bridge.

2. Memory Corruption / UB

  • ECS cached-pointer contract (regression guard, #35 + #1367). World::get (crates/core/src/ecs/world.rs) returns a ComponentRef<'_, T>, NOT a raw pointer with a dropped guard (the unsound #35 pattern). ComponentRef, StorageRef, and StorageRefMut in crates/core/src/ecs/query.rs cache a *const T / *mut T resolved once in new() and deref it in the hot path (#1367). Each cached-deref unsafe block carries a SAFETY comment tying the pointer's validity to the lock guard the wrapper pins. The invariant: the guard must outlive every deref, and &mut self must gate &mut *self.storage. Verify the SAFETY comments still match the field layout and that no refactor let a guard drop before its pointer (use-after-free → CRITICAL).
  • #[repr(C)] GPU-struct soundness (crates/renderer/src/vulkan/scene_buffer/gpu_types.rs): GpuInstance/GpuCamera/GpuLight etc. are uploaded byte-for-byte to SSBOs. vec3 must be three scalar f32, never [f32; 3] (std430 vec3 padding). A layout drift here is silent per-instance corruption — see Dimension 6 for the GpuMaterial pin and _audit-severity's #[repr(C)]-drift HIGH row.
  • NIF bulk POD reads (NifStream::read_pod_vec, crates/nif/src/stream.rs; the header mirror read_pod_vec_from_cursor, crates/nif/src/header.rs): read_exact of raw LE bytes into a T: AnyBitPattern vector. SAFETY comments must hold — T is restricted to bit-pattern-safe types (a sealed bound stops read_pod_vec::<bool>). Verify the byte-count overflow guard (count × size) is present and no caller widens T past AnyBitPattern.
  • sfmaterial enum decode (BuiltinType::from_u32, crates/sfmaterial/src/types.rs): MUST stay a checked match over the 0xFFFFFF## tags with a _ => return Err(Error::UnsupportedBuiltin { raw }) arm (confirmed present). The module doc's "transmute into this enum" wording is aspirational prose, NOT the impl — an actual std::mem::transmute of an unmatched #[repr(u32)] byte pattern is UB. Verify the match + Err arm survive any "optimization."
  • pex opcode decode (OpCode::from_u8, crates/pex/src/opcode.rs): unlike sfmaterial, this one IS a real unsafe { std::mem::transmute::<u8, OpCode>(byte) } — sound ONLY because the SAFETY comment's two preconditions hold: OpCode is #[repr(u8)] with contiguous discriminants 0..MAX_OPCODE, AND byte is range-checked (< MAX_OPCODE) before the transmute. Both must stay true: a gap in the discriminant sequence, or a refactor that drops the bound check, makes an out-of-range byte UB. Verify the guard and the contiguity (no skipped values in the enum) on any opcode-table change.
  • Stack-overflow risk: no unbounded recursion in block-walk / scene-graph traversal.

3. Memory & Resource Leaks (HIGH when per-frame/per-cell)

  • Rapier bodies on cell unload (regression guard, #1520, 34c7a218). crates/physics/src/world.rs::remove_* and byroredux/src/cell_loader/unload.rs must release a cell's rigid bodies, colliders, and impulse joints from RigidBodySet / ColliderSet / ImpulseJointSet (plus broad-phase / query-pipeline state) when the cell unloads. Without it they accumulate per cell — a steady leak under exterior streaming. Guard test: byroredux/src/cell_loader/rapier_release_tests.rs. Verify the release path is still wired and the test still asserts emptiness post-unload.
  • Deferred-destroy drain (crates/renderer/src/deferred_destroy.rs, DeferredDestroyQueue<T> shared by mesh + BLAS + BLAS-scratch buffer (#1782) + texture + skin compute): objects are destroyed only after the in-flight fence clears (#418 moved the tick after fence wait; #732 added an explicit shutdown drain). Verify the tick still runs after fence wait in context/draw.rs and the shutdown sweep drains the queue — a missed drain leaks GPU memory across the app lifetime, a too-early destroy is use-after-free (CRITICAL).
  • AllocatorResource drop ordering (regression guard, #1406, 299e6a84). AllocatorResource (crates/renderer/src/vulkan/allocator.rs; held in byroredux/src/main.rs) must be removed from the ECS World BEFORE VulkanContext::drop() runs. The allocator holds a live Arc<Device>; if the World outlives the context, the allocator's Drop calls the driver against a destroyed logical device (use-after-free → CRITICAL). Verify the main loop removes the resource before dropping the renderer, including the panic-unwind path that could skip the removal.
  • GPU allocation inventory — every long-lived allocation tracked and freed: BLAS scratch/result, TLAS instance/result, G-buffer images, SVGF history, TAA per-FIF history images, caustic + water-caustic R32_UINT accumulators (caustic.rs / water_caustic.rs), per-skinned-entity SkinSlot output buffers, MaterialBuffer SSBO, volumetric/bloom mip pyramids. Cross-check eviction thresholds against docs/engine/memory-budget.md; do not re-derive.
  • CPU-side unbounded growthVec/HashMap keyed by cell or path that never shrinks. The MaterialTable dedup map and AnimationClipRegistry (Dimension 8) are the known per-cell-growth risks.

4. Unsafe-Block Discipline (MEDIUM — the bread-and-butter sweep)

  • Grep every unsafe in crates/ + byroredux/ (.rs). For each: is there a SAFETY comment, and does the comment's stated invariant actually hold at this call site? A correct unsafe block with no comment is still a MEDIUM finding (_audit-severity Special Rules). A commented block whose invariant is FALSE is the higher-severity finding.
  • Heaviest in crates/renderer/src/vulkan/ ash FFI — the SAFETY/unsafe count gap (~676 vs ~761) is the haystack. Spot-check the ash dispatch wrappers, the gpu-allocator Arc<Mutex<…>> interactions, and any from_raw_parts / cast on mapped memory.
  • Report unsafe blocks lacking comments as a batched MEDIUM finding (list the sites) rather than one finding per block, unless an invariant is actually unsound.

5. Vulkan Spec Compliance (HIGH — but flag what cargo test can't see)

Per the No-Speculative-Vulkan-Fixes rule: render-pass / barrier / pipeline-state spec claims that are invisible to cargo test MUST be framed as "needs validation-layer or RenderDoc verification", not asserted as confirmed bugs. Run the engine with validation layers (debug build) and report ANY emitted error verbatim — that is the sound evidence channel for this dimension.

  • All vkCreate*/vkDestroy* paired; Drop ordering destroys children before parents (device-destroy is last).
  • Queue submission ordering: wait-before-signal; per-image semaphores.
  • Acceleration structures (crates/renderer/src/vulkan/acceleration/): correct geometry flags, valid device addresses, buffers carry SHADER_DEVICE_ADDRESS. TLAS UPDATE mode — instance/geometry count must match the original BUILD. Skin BLAS refit — vertex/geometry count must match BUILD; a bone-count change forces a full rebuild. (Wrong AS geometry/address = CRITICAL per _audit-severity.)
  • TLAS resize wait (regression guard, #1390, a7e1502b). The resize branch in acceleration/tlas.rs calls device.device_wait_idle() before freeing the old allocation (confirmed present). Verify the wait survives — without it the GPU may still consume the old TLAS scratch during free under a resize-under-load refactor.
  • VK_KHR_ray_query enabled + feature-gated before any ray-query use.
  • Per-frame compute layout hygiene (TAA / caustic / water-caustic / volumetrics / bloom): images that coexist as storage-write + sampled-read are held in GENERAL; initialize_layouts does the one-time UNDEFINED→GENERAL transition for every mip / FIF slot. A missed slot is an UNDEFINED-read validation error. CLEAR-before- COMPUTE invariant (caustic R32_UINT imageAtomicAdd, volumetric inject) — a missing clear is persistent cross-frame ghost accumulation. Verify the volumetrics caller honors the dispatch gate: VOLUMETRIC_OUTPUT_CONSUMED (crates/renderer/src/vulkan/volumetrics.rs) is now true, so the pass is live — dispatch is dead only while it reads false. Callers MUST gate vol.dispatch() on that const either way (context/post_passes.rs); read the const rather than assuming a state.
  • SPIR-V reflection (crates/renderer/src/vulkan/reflect.rs): the Rust descriptor layout must match shader-declared bindings — this is the one binding-drift check that IS visible to cargo test (scene_descriptor_reflection_tests). Prefer it over eyeballing descriptor writes.

6. R1 Material Table Layout Soundness

  • GpuMaterial size is pinned at 348 B by gpu_material_size_is_348_bytes (crates/renderer/src/vulkan/material.rs) — the test name now matches the asserted size (history: 272 → 260 after #804 dropped avg_albedo, → 296 with the Disney sheen/subsurface lobe #1249, → 300 with anisotropic #1250, → 348 on 2026-07-27 (1d94eb24) with the twelve common supplemental texture roles). A stale 260/272/296/300 in audit prose, or any test-name-vs-asserted-size mismatch, means the GPU is reading wrong bytes.
  • Per-field offset pin gpu_material_field_offsets_match_shader_contract (#806): every named field's byte offset asserted against the shader contract. The size pin alone cannot catch a within-vec4 reorder (swap texture_index ↔ normal_map_index is size-invisible, runtime-lethal). Adding a field without updating this assertion is a regression.
  • ALL fields are flat scalar f32/u32 — never [f32; 3] (std430 vec3 alignment). This includes the newest scalars: the BGSM translucency suite (translucency_subsurface_r/g/b, …_transmissive_scale, …_turbulence) and the Disney lobe (ior, subsurface, sheen, sheen_tint, anisotropic).
  • Pad fields explicitly zeroed (the byte-Hash/Eq dedup hashes the raw 348 B; an uninit hole poisons dedup). New scalars must be zeroed in GpuMaterial::default() so default materials still dedup to slot 0.
  • Intern cap (#797). MaterialTable::intern caps at MAX_MATERIALS = 16384 (scene_buffer/constants.rs); over-cap interns return id 0 with a one-shot warn — no SSBO over-index, no DEVICE_LOST. upload_materials (scene_buffer/upload.rs) debug_asserts len <= MAX_MATERIALS and clamps with .min(MAX_MATERIALS). Verify the intern cap and the upload truncation stay in lockstep.
  • GpuInstance.material_id indexes the SSBO with NO GPU bounds check — CPU must guarantee in-range (SSBO index mismatch = CRITICAL).
  • ui.vert MaterialBuffer read offsets must stay in lockstep with the canonical struct GpuMaterial / GpuInstance in crates/renderer/shaders/include/bindings.glsl (triangle.frag #includes it) — #785 was a stale-hunk regression reading wrong bytes — name ui.vert explicitly.

7. RT IOR-Refraction Safety (regression guards)

  • Glass-passthrough loop guard (#789): the texture-equality identity check at the refraction hit prevents unbounded recursion when coincident glass surfaces share an albedo/normal-map descriptor pair. A regression is a frame-time hang on any paired-glass cell. Verify the check is present.
  • Glass ray budget GLASS_RAY_BUDGET (crates/renderer/src/shader_constants_data.rs, mirrored in crates/renderer/shaders/include/shader_constants.glsl — verify the two stay in lockstep; raised from 8192 in 6efe1706, and again since — check the constant by name rather than trusting a hard-coded figure here). It is a runaway-recursion cap, not a quality knob. #1438 documented that the atomicAdd accounting can overshoot the budget unconditionally — note that nuance rather than reporting the overshoot as new. Verify the budget is enforced at every glass call site.
  • Frisvad orthonormal basis (#820): the naive cross(N, world-up) basis degenerates near-vertical (zero-length → NaN ray). Verify Frisvad is the active path for IOR refraction roughness spread.
  • IOR miss fallback for interiors uses cell-ambient, not global sky tint (open-sky leakage into dungeons is a visible regression).
  • DBG_VIZ_GLASS_PASSTHRU = 0x80 is a permanent diagnostic bit — verify it hasn't collided with a new debug flag (full catalog in crates/renderer/src/shader_constants_data.rs, mirrored to the generated crates/renderer/shaders/include/shader_constants.glsl).

8. NPC / Animation Spawn Safety

  • B-spline pose-fallback sentinel (#772): NPCs vanishing under FNV BSPSysSimpleColorModifier particle stacks sharing keyframe time-zero with the actor's player must be gated on an FLT_MAX sentinel. Removing the gate is whole-NPC disappearance, not a stuck pose. Verify the sentinel is wired.
  • AnimationClipRegistry dedup (#790): the registry interns by lowercased path so cell streaming doesn't grow it unboundedly (otherwise one keyframe set leaks per cell load → steady RAM growth). Verify case-insensitive interning is preserved.
  • B-splines reach FNV / FO3 too (feedback_bspline_not_skyrim_only.md) — do NOT rule out NiBSplineCompTransformInterpolator by game era.
  • Starfield content is WALKABLE (Cydonia) — SF cells reach the spawn/animation path; don't short-circuit spawn-safety reasoning with "no SF content exercises this."
  • MAX_TOTAL_BONES overflow guard must fire — silent truncation past cap was the M29 regression. The slot-exhaustion warn lives on SkinSlotPool (crates/core/src/ecs/resources/skin_slot_pool.rs), one-shot via the overflow_warned flag with overflow_attempt_count carrying total demand; excess skinned entities fall back to bind pose rather than over-indexing. Guard tests: byroredux/src/render/bone_palette_overflow_tests.rs.

9. NIFAL Boundary — NaN/Inf on the GPU (UB facet only)

See /audit-nifal for correctness-of-mapping; this dimension covers ONLY the safety facet — NaN/inf scalars reaching the GPU, unbounded allocation.

  • byroredux/src/material_translate.rs::translate_material deliberately seeds f32::NAN into Material.metalness/roughness (mesh.metalness_override.unwrap_or(f32::NAN), same for roughness). Material::resolve_pbr (crates/core/src/ecs/components/material.rs) is the ONLY thing that detects (is_nan()) and clamps these sentinels before they reach GpuMaterial. Both fields are now plain f32 (no Option), so a producer that skips resolve_pbr() ships a NaN into the SSBO silently (NaN-on-GPU = UB). Verify EVERY renderer-bound Material producer runs resolve_pbr() or constructs already-finite values (the static_meshes.rs fallback constructs finite defaults directly — confirm it still does).
  • Collision translate (crates/nif/src/import/collision/mod.rs, covers BhkMultiSphereShape + BhkConvexListShape): emitted half-extents / radii / sphere centers must be finite and bounded — a NaN/inf shape param propagates into the physics solver and the BLAS build.
  • Typed particle blocks (crates/nif/src/blocks/particle.rs) → extract_emitter_params/extract_emitter_rate (crates/nif/src/import/walk/mod.rs) → apply_emitter_params (byroredux/src/systems/particle.rs): emitter rate / lifespan / size must be finite and non-negative at the extract boundary — an unbounded or NaN rate is an unbounded-allocation / NaN-transform risk downstream.

10. debug-ui (egui overlay) Teardown & Shared-Allocator Safety

  • crates/debug-ui/src/lib.rs DebugUiState is the CPU half only — egui context, egui_winit state, last FullOutput, panel state. It holds no Vulkan handle. It lives as an ECS resource (impl Resource for DebugUiState) and is owned by the main loop.
  • The Vulkan half is EguiPass (crates/renderer/src/vulkan/egui_pass.rs, held as VulkanContext::egui_pass: Option<EguiPass>). It takes an ash::Device + the shared Arc<Mutex<gpu_allocator::vulkan::Allocator>> and wraps egui-ash-renderer, which owns its own descriptor pool + per-texture images, plus EguiPass's own vk::RenderPass + per-swapchain-image framebuffers. Those MUST be freed before the engine destroys the ash::Device — same class as Dimension 3's allocator-before-device rule. Verify the Option<EguiPass> teardown runs ahead of VulkanContext's device-destroy.
  • Texture free is deferred one frame (pending_free), leaning on draw_frame's fence wait — verify the defer survives; freeing on the arriving frame is a use-after-free.
  • The allocator mutex is SHARED with the render thread — verify it is held for minimum duration during egui texture upload; a long hold stalls rendering. The graphics queue is likewise passed as a Mutex (EguiDispatchCtx::queue) so the lock scopes to the set_textures submit only (CONC-D1-01 / #1713) — a widened hold regresses that.

Procedure

  1. Grep all unsafe in crates/ + byroredux/ (.rs); note the renderer mass and the SAFETY-comment gap (Dimension 4).
  2. Confirm the cxx bridge is still a no-pointer placeholder (Dimension 1).
  3. Audit the cached-pointer ECS contract + repr(C) GPU structs + NIF POD reads + sfmaterial decode (Dimension 2).
  4. Walk the leak inventory and the three drop-ordering regression guards — Rapier release, deferred-destroy drain, AllocatorResource removal (Dimension 3).
  5. Sweep unsafe-block discipline; batch the comment-less blocks (Dimension 4).
  6. Vulkan-spec pass — run validation layers, report emitted errors verbatim; frame barrier/layout claims invisible to cargo test as "needs RenderDoc verification" (Dimension 5).
  7. R1 material layout pins (Dimension 6), IOR/glass guards (7), NPC/anim spawn (8), NIFAL NaN boundary (9), debug-ui teardown (10).
  8. Dedup against open/closed issues (_audit-common Deduplication) — most items above are regression guards; recast a confirmed-intact guard as PASS, not a NEW finding.
  9. Save the report to docs/audits/AUDIT_SAFETY_<TODAY>.md (see _audit-common Report Finalization).
Skills similaires