Skip to content

Fix rate-position desync from repeated sonicSetPitch calls - #73

Open
austek wants to merge 1 commit into
waywardgeek:masterfrom
ZirekHQ:fix/pitch-position-desync
Open

Fix rate-position desync from repeated sonicSetPitch calls#73
austek wants to merge 1 commit into
waywardgeek:masterfrom
ZirekHQ:fix/pitch-position-desync

Conversation

@austek

@austek austek commented Aug 23, 2026

Copy link
Copy Markdown

Fixes #41, fixes #39.

Summary

Both issues report a crash/hang after several sonicSetPitch calls on a live stream. #41's original report includes Assertion failed: stream->newRatePosition != newSampleRate — a manual (fprintf+exit) invariant check that existed until it was quietly dropped in a1746b3 (2021-10-13) — over a year before either issue was filed, meaning both reporters were on stale/vendored builds predating that removal and the literal crash text can't reproduce on any current code. Removing the check didn't fix the underlying bug, just stopped it from failing loudly.

adjustRate's wraparound arithmetic (oldRatePosition/newRatePosition) assumes both counters were accumulated under one consistent (oldSampleRate, newSampleRate) ratio — processStreamInput computes that ratio as stream->rate * stream->pitch. sonicSetRate resets both counters to 0 because changing rate invalidates the ratio the counters were accumulated under; sonicSetPitch changes the exact same ratio but never got the same reset.

Left to run, the counters drift further from what the (now-changed) ratio expects with every subsequent pitch change. Confirmed via a targeted random sweep (sample rate × channels × many pitch/rate/speed changes) that the invariant genuinely gets violated on current code, not just hypothetically — and traced one violating case all the way to a real crash: the drifted counters eventually overflow interpolate()'s position arithmetic, indexing sincTable far out of bounds (confirmed via ASan: global-buffer-overflow in findSincCoefficient, called from interpolate, called from adjustRate).

Fix

Reset oldRatePosition/newRatePosition in sonicSetPitch too, mirroring sonicSetRate exactly (a two-line diff).

Tests

Adds tests/pitch_position_test.c with a deterministic repro (10 fixed pitch values, no randomness) trimmed down from the random sweep that found it — verified crashing under ASan before this fix, clean after.

Test plan

  • make test — passes
  • make CC=clang CFLAGS="-Wall -g -fsanitize=address,undefined -fno-omit-frame-pointer" test — clean (only the separate, already-known findSincCoefficient finding, tracked in Add GitHub Actions CI and SECURITY.md #67)
  • Re-ran the full random sweep (sample rate × channels × 30 pitch/rate/speed changes × 10 seeds = 140 runs) against the fixed code under ASan+UBSan — zero failures, was failing consistently before the fix

Fixes waywardgeek#41, fixes waywardgeek#39. Both report a crash/hang after several
sonicSetPitch calls on a live stream; waywardgeek#41's original report includes
"Assertion failed: stream->newRatePosition != newSampleRate", a
manual (fprintf+exit) invariant check that existed until it was
quietly dropped in a1746b3 (2021-10-13) -- over a year before either
issue was filed, meaning both reporters were on stale/vendored builds
predating that removal and the literal crash text can't reproduce on
any current code. Removing the check didn't fix the underlying bug,
just stopped it from failing loudly.

adjustRate's wraparound arithmetic
(oldRatePosition/newRatePosition) assumes both counters were
accumulated under one consistent (oldSampleRate, newSampleRate)
ratio -- processStreamInput computes that ratio as
stream->rate * stream->pitch. sonicSetRate resets both counters to 0
because changing rate invalidates the ratio the counters were
accumulated under; sonicSetPitch changes the exact same ratio but
never got the same reset.

Left to run, the counters drift further from what the (now-changed)
ratio expects with every subsequent pitch change. Confirmed via a
targeted random sweep (sample rate x channels x many pitch/rate/speed
changes) that the invariant genuinely gets violated on current code,
not just hypothetically -- and traced one violating case all the way
to a real crash: the drifted counters eventually overflow
interpolate()'s position arithmetic, indexing sincTable far out of
bounds (confirmed via ASan: global-buffer-overflow in
findSincCoefficient, called from interpolate, called from adjustRate).

Fix: reset oldRatePosition/newRatePosition in sonicSetPitch too,
mirroring sonicSetRate exactly.

Adds tests/pitch_position_test.c with a deterministic repro (10 fixed
pitch values, no randomness) trimmed down from the sweep that found
it -- verified crashing under ASan before this fix, clean after.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant