[Audit][High] Mixer loop cursor reset skips bounds re-check, risking OOB read and audio artifacts

[MichaelFisher1997]

🔍 Module Scanned

(automated audit scan)

📝 Summary

In the function of , when a looping voice reaches end-of-buffer, the cursor is reset to 0.0 and looping continues, but the bounds check that follows does not re-fetch the updated cursor value. Since was just modified to 0.0 before the comparison at line 265, the subsequent condition still uses the stale from before the reset. This means after looping, the first sample read can be at an incorrect offset, causing audio glitches or OOB memory access if the buffer was freed.

📍 Location

• File:
• Function/Scope: — loop cursor reset path

🔴 Severity: High

• Critical: Potential out-of-bounds memory read, possible crash or audio corruption
• High: Audio artifacts (clicking/popping) when looped sounds wrap
• Medium: The bug is masked by the subsequent re-calculation at line 275, but that second check can also trigger false positives when it shouldn't

💥 Impact

When a looping voice reaches its end-of-buffer:

1. is set to 0.0 at line 267
2. The loop continues without breaking
3. Line 265's bounds check uses stale (from before cursor reset)
4. An incorrect sample offset may be used for the next iteration before re-calculation
5. In pathological cases (e.g., very small buffers, or pitch > 1.0 causing rapid wrap), this could read past buffer end before the second check at line 276 catches it

🔎 Evidence

The problem is that after , control flows directly to line 275 without a or break. The code continues to line 283 where it reads using the updated cursor (now 0), but the second bounds check at line 276 uses the newly calculated (which would be 0 after loop). If buffer length is less than 2 bytes (unlikely but possible for malformed data), the check at line 276 would catch it, but the deeper issue is the flow: after in the loop, the code continues to use (line 283) which was calculated before the reset.

🛠️ Proposed Fix

After setting inside the loop, add a to skip the rest of the iteration and allow the next iteration to properly calculate from the reset cursor:

Additionally, consolidate the redundant bounds checks. The check at line 276-279 duplicates logic that could be handled more cleanly after the cursor reset.

✅ Acceptance Criteria

• All unit tests in pass
• Looping sounds correctly restart from sample 0 without artifacts
• No out-of-bounds memory access when mixing looped audio with pitch != 1.0
• The fix has been verified with Zig 0.16.0 + SDL3 Dev Environment
  Compiler: 0.16.0
  assets/shaders/vulkan/shadow.vert
  assets/shaders/vulkan/lpv_propagate.comp
  assets/shaders/vulkan/ui.frag
  assets/shaders/vulkan/ui_tex.frag
  assets/shaders/vulkan/g_pass.frag
  assets/shaders/vulkan/debug_shadow.vert
  assets/shaders/vulkan/lpv_inject.comp
  assets/shaders/vulkan/ssao.vert
  assets/shaders/vulkan/sky.frag
  assets/shaders/vulkan/water.vert
  assets/shaders/vulkan/bloom_downsample.vert
  assets/shaders/vulkan/bloom_upsample.vert
  assets/shaders/vulkan/debug_shadow.vert
  assets/shaders/vulkan/fxaa.vert
  assets/shaders/vulkan/post_process.vert
  assets/shaders/vulkan/shadow.vert
  assets/shaders/vulkan/sky.vert
  assets/shaders/vulkan/ssao.vert
  assets/shaders/vulkan/taa.vert
  assets/shaders/vulkan/terrain.vert
  assets/shaders/vulkan/ui.vert
  assets/shaders/vulkan/ui_tex.vert
  assets/shaders/vulkan/water.vert
  assets/shaders/vulkan/bloom_downsample.frag
  assets/shaders/vulkan/bloom_upsample.frag
  assets/shaders/vulkan/debug_shadow.frag
  assets/shaders/vulkan/fxaa.frag
  assets/shaders/vulkan/g_pass.frag
  assets/shaders/vulkan/post_process.frag
  assets/shaders/vulkan/shadow.frag
  assets/shaders/vulkan/sky.frag
  assets/shaders/vulkan/ssao.frag
  assets/shaders/vulkan/ssao_blur.frag
  assets/shaders/vulkan/taa.frag
  assets/shaders/vulkan/terrain.frag
  assets/shaders/vulkan/terrain_debug.frag
  assets/shaders/vulkan/ui.frag
  assets/shaders/vulkan/ui_tex.frag
  assets/shaders/vulkan/water.frag
  assets/shaders/vulkan/culling.comp
  assets/shaders/vulkan/depth_pyramid.comp
  assets/shaders/vulkan/lpv_inject.comp
  assets/shaders/vulkan/lpv_propagate.comp
  assets/shaders/vulkan/mesh.comp
  assets/shaders/vulkan/debug_shadow.frag
  assets/shaders/vulkan/taa.frag
  assets/shaders/vulkan/mesh.comp
  assets/shaders/vulkan/water.frag
  assets/shaders/vulkan/taa.vert
  assets/shaders/vulkan/depth_pyramid.comp
  assets/shaders/vulkan/culling.comp
  assets/shaders/vulkan/ssao.frag
  assets/shaders/vulkan/ui.vert
  assets/shaders/vulkan/shadow.frag
  assets/shaders/vulkan/ssao_blur.frag
  assets/shaders/vulkan/terrain.vert
  assets/shaders/vulkan/ui_tex.vert
  assets/shaders/vulkan/terrain.frag
  assets/shaders/vulkan/sky.vert

📚 References

• Related issue: [Audit][High] Dangling pointer risk: Mixer holds pointers to SoundData that can be freed by SoundManager #683 (Dangling pointer risk in Mixer — same file, different function)
• Zig std.mem.Allocator patterns for buffer management
• SDL3 Audio Stream documentation for proper mixing semantics

[github-actions]

Comment on Issue:

I've verified the code in modules/engine-audio/src/backends/sdl_audio.zig:265-283. The issue description is partially correct but contains a factual error about the execution flow.

What's actually happening:

1. Line 255: pos_idx is calculated from voice.cursor (pre-reset value)
2. Lines 265-272: If OOB and voice.loop == true, voice.cursor is set to 0.0 — but no continue/break follows, so the iteration continues
3. Line 275: valid_pos_idx is recalculated from the reset voice.cursor (now 0.0)
4. Lines 283-284: Sample is read from valid_pos_idx * 2 (i.e., from byte 0)

The proposed continue fix is correct and necessary. Adding continue after voice.cursor = 0.0 at line 267 ensures the next iteration properly calculates pos_idx from the reset cursor, rather than reading sample 0 in the same iteration that performed the reset.

However, the severity is overstated. The code does not cause an OOB read in the current flow — valid_pos_idx is recalculated after the reset (line 275) and a second bounds check exists at line 276. The real issues are:

• Architectural confusion: mixing reset logic and sample reading in the same iteration
• Potential audio glitch: reading sample 0 at a point in the iteration where pitch hasn't yet been re-applied to the reset cursor
• Redundant bounds checks: lines 276-279 are a patch over the missing continue

Suggested fix: Add continue after voice.cursor = 0.0 at line 267. This makes the loop restart from the top on the next iteration with the correct cursor position.

opencode session  |  github run