[Audit][High] Use-after-free in AudioSystemManager.deinit — reads AudioSystem.allocator after self-free

[MichaelFisher1997]

🔍 Module Scanned

src/game/ (automated audit scan)

📝 Summary

AudioSystemManager.deinit calls self.audio_system.deinit() which internally frees the AudioSystem heap allocation via self.allocator.destroy(self). Control then returns to AudioSystemManager.deinit and dereferences self.audio_system.allocator to free the manager itself, reading freed memory (use-after-free / CWE-416). The bug also causes a double-destroy hazard if the allocator's backing page is reused.

📍 Location

• File: src/game/audio_system_manager.zig:18-22
• Function/Scope: AudioSystemManager.deinit

The free of the inner struct happens at:

• File: modules/engine-audio/src/system.zig:78
• Function/Scope: AudioSystem.deinit

🔴 Severity: High

• Critical: Crashes, data corruption, security vulnerabilities, GPU device loss
• High: Memory safety bugs (use-after-free), undefined behavior, resource lifecycle violations
• Medium: Performance degradation, missing error handling, suboptimal patterns
• Low: Code style, dead code, minor improvements

💥 Impact

Whenever the application exits normally (or fails late in init thanks to the errdefer audio_manager.deinit() chain in src/game/app.zig:189), the heap allocator will see reads of an already-freed AudioSystem struct. In a debug/safety allocator this surfaces as a panic ("invalid free" or "use after free"); in release builds it can return a stale allocator pointer that mutates arbitrary memory, silently corrupts the next allocation, or causes a double-free of the manager when the allocator tracks its own state. Because this is on the shutdown path it can manifest as mysterious late-exit crashes that are hard to reproduce and attribute.

🔎 Evidence

src/game/audio_system_manager.zig:

pub fn deinit(self: *AudioSystemManager) void {
    self.audio_system.deinit();            // (1) AudioSystem.deinit frees 
    const allocator = self.audio_system.allocator;  // (2) UAF: AudioSystem memory was freed
    allocator.destroy(self);
}

modules/engine-audio/src/system.zig:

pub fn deinit(self: *AudioSystem) void {
    self.stopAll();
    if (self.backend_ptr) |ptr| {
        const backend_inst: *sdl_backend.SDLAudioBackend = @ptrCast(@alignCast(ptr));
        backend_inst.destroy();
        self.backend_ptr = null;
    }
    self.manager.deinit();
    self.allocator.destroy(self);   // frees the AudioSystem heap allocation
}

Call site in src/game/app.zig:

errdefer audio_manager.deinit();  // line 189 — triggered on any later init failure
...
self.audio_manager.deinit();       // line 284 — normal shutdown

The pattern of "inner type destroys itself inside its own deinit" is fragile and has already been a source of UAF bugs in this codebase (cf. the comment block at system.zig:62-69 referencing issue #683 about Mixer/SoundData ordering).

🛠️ Proposed Fix

Pick one of the following, in order of preference:

1. Capture the allocator before delegating in AudioSystemManager.deinit:

   pub fn deinit(self: *AudioSystemManager) void {
       const allocator = self.audio_system.allocator; // capture first
       self.audio_system.deinit();                      // AudioSystem still valid here
       allocator.destroy(self);
   }

   Optionally, also stash the allocator inside AudioSystemManager at init time so it doesn't depend on the inner struct at all.

2. Make AudioSystem.deinit non-destructive on self: split it so AudioSystem no longer calls self.allocator.destroy(self); let the caller (AudioSystemManager) own the destroy. Update all callers (AudioSystem.init already does errdefer allocator.destroy(self) so the error path is covered). This is more invasive but eliminates the "inner self-destroy" footgun for every future caller and matches the convention recommended in the comment block at system.zig:62-69.

A regression test should be added in src/tests.zig that constructs an AudioSystemManager with a debug-tracking allocator (e.g., std.testing.allocator) and calls deinit, asserting no leak/double-free is reported.

✅ Acceptance Criteria

• AudioSystemManager.deinit no longer reads self.audio_system after audio_system.deinit has returned
• Running nix develop --command zig build test passes with no leak or double-free diagnostics when the audio manager is exercised
• A unit test exists in src/tests.zig that constructs and destroys an AudioSystemManager using std.testing.allocator and confirms clean teardown
• The fix is applied to the recommended option (1) or (2) above; whichever is chosen, document the ownership contract in the module header so future wrappers don't reintroduce the bug

📚 References

• See the existing ordering-safety comment at modules/engine-audio/src/system.zig:62-69 referencing issue [Audit][High] Dangling pointer risk: Mixer holds pointers to SoundData that can be freed by SoundManager #683 (Mixer/SoundData UAF) for the prior art on this class of bug in the same module.
• CWE-416: Use After Free — https://cwe.mitre.org/data/definitions/416.html
• Zig memory ownership convention: prefer "inner type is non-destructive in deinit; caller destroys" for heap-allocated wrapper structs to keep destruction order unambiguous.

[github-actions]

Issue Analysis

Classification: Bug — memory safety (use-after-free, CWE-416).

Audit validation: ✅ Fully confirmed against current dev. The code at src/game/audio_system_manager.zig:18-22 and modules/engine-audio/src/system.zig:70-79 matches the report verbatim, and the call sites at src/game/app.zig:189 (errdefer) and :284 (normal shutdown) are real and exercised on every run. src/tests.zig:115 only imports engine-audio.sdl_audio; there is no test covering AudioSystemManager teardown, which is exactly how this slipped through.

Scope: Narrow, single-subsystem, single-PR. This is not a candidate for splitting — the fix is one (or a few) lines plus one regression test. No duplication with an existing PR (latest commit is the unrelated #915).

Recommended fix: Option 1 (capture allocator before delegating) is clearly preferable here:

• It is a 3-line, fully localized change in audio_system_manager.zig.
• It does not touch the public contract of AudioSystem.deinit, which is also called directly from src/game/app.zig flow paths that need to remain unchanged.
• It sidesteps the deeper "inner self-destroy" footgun without requiring a sweep of every caller (e.g. SoundManager, backend wrappers).

Option 2 is the architecturally cleaner long-term direction (and the audit's system.zig:62-69 doc comment already hints at it), but it's a wider refactor that risks scope creep. Best treated as a follow-up tracking issue once Option 1 lands and the ownership contract in the module header is updated.

Suggested concrete patch sketch for Option 1:

// src/game/audio_system_manager.zig
pub fn deinit(self: *AudioSystemManager) void {
    const allocator = self.audio_system.allocator; // capture before inner deinit frees it
    self.audio_system.deinit();
    allocator.destroy(self);
}

Even better: store the allocator on AudioSystemManager itself in init (so the manager doesn't depend on the inner struct's lifetime for its own destruction — this also future-proofs against any further inner-struct changes).

Acceptance criteria addition: Beyond the items listed, please add a --test-filter name for the new test and verify it runs under both zig build test and that std.testing.allocator reports zero leaks/double-frees after the call.

Missing info for the author: None blocking — the bug is deterministic and reproducible from code inspection alone; a zig build test run with the new test under std.testing.allocator will surface it immediately under safety builds.

opencode session  |  github run