residfp_calculate_samples() during C64 power cycle/reset**DISCLOSURE: Code analysis, debug, fixing and this report is made with AI assistance.
**
Power-cycling or resetting an emulated C64 in x64sc can corrupt the heap and crash VICE when the reSIDfp sound engine is active.
The initial non-instrumented crash appeared in CoreAudio or later in unrelated VICE code. However, an AddressSanitizer build identifies the first invalid access as an out-of-bounds write in reSIDfp::SID::clock(), called by residfp_calculate_samples() in src/sid/residfp.cc.
The temporary buffer is allocated from nr, which is the capacity of VICE's destination buffer. However, reSIDfp::SID::clock(unsigned int cycles, int16_t *buf) has no output-capacity argument and writes every sample produced while processing int_delta_t cycles.
During a reset or power-cycle path it can produce more than nr samples, overflowing the temporary buffer before clock() returns.
x64scx64sc.The crash was consistently reproducible on the reported system.
The C64 power cycle or reset completes, and emulation continues with sound reinitialized.
An optimized build crashes after the reset.
Depending on allocator and thread timing, the visible crash may occur in CoreAudio cleanup or in unrelated VICE code because the actual heap corruption occurred earlier.
An AddressSanitizer build reports the first invalid write directly in reSIDfp:
ERROR: AddressSanitizer: heap-buffer-overflow
WRITE of size 2
#0 reSIDfp::SID::clock(unsigned int, short*) SID.h:377
#1 residfp_calculate_samples(...) residfp.cc:298
#2 sid_sound_machine_calculate_samples sid.c
#3 sound_run_sound sound.c:1399
#4 sound_store sound.c:1772
#5 do_reset_cia c64cia2.c:114
0 bytes after 8832-byte region
allocated by lib_calloc from residfp_calculate_samples()
at residfp.cc:296
As a diagnostic experiment, increasing the temporary allocation from nr to nr + 1 samples did not fix the issue.
ASan then reported another two-byte write exactly after the new 8834-byte allocation:
#0 reSIDfp::SID::clock(unsigned int, short*) SID.h:377
#1 residfp_calculate_samples(...) residfp.cc:302
0 bytes after 8834-byte region
allocated from residfp_calculate_samples()
at residfp.cc:300
This confirms that fixed padding based on nr is not a valid bound.
In the non-float residfp_calculate_samples() implementation, the temporary buffer is allocated using the requested VICE output capacity:
tmp_buf = getbuf(2 * nr);
retval = psid->sid->clock(int_delta_t, tmp_buf);
For the SID-card branch, it similarly uses:
tmp_buf = getbuf(2 * nr * psid->factor / 1000);
retval = psid->sid->clock(int_delta_t, tmp_buf);
getbuf() takes a byte count, so these expressions allocate storage for nr samples or for the factor-adjusted expected sample count.
The reSIDfp overload used here is:
int SID::clock(unsigned int cycles, int16_t *buf);
It does not receive the capacity of buf. Its implementation processes all supplied cycles and appends every sample produced by the resampler.
Consequently, the temporary buffer must be large enough for the maximum number of samples that can be emitted while processing int_delta_t cycles. A safe upper bound is one output sample per emulated cycle.
There is a second bounds issue after clock() returns: retval is used as the copy-loop limit even though VICE's destination buffer has capacity for only nr samples. The returned count must therefore be clamped before copying.
Allocate the temporary buffer using int_delta_t, then clamp the number copied into VICE's output buffer to nr.
diff --git a/vice/src/sid/residfp.cc b/vice/src/sid/residfp.cc
--- a/vice/src/sid/residfp.cc
+++ b/vice/src/sid/residfp.cc
@@ -286,12 +286,19 @@ static int residfp_calculate_samples(sound_t *psid, short *pbuf, int nr,
int retval;
int int_delta_t = (int)*delta_t;
+ if (nr <= 0 || int_delta_t <= 0) {
+ return 0;
+ }
+
/* Tried not to mess with resid during 64-bit conversion. clock(...) wants to modify *delta_t ... */
if (psid->factor == 1000) {
- tmp_buf = getbuf(2 * nr);
+ tmp_buf = getbuf((int)(sizeof(*tmp_buf) * (size_t)int_delta_t));
/* CAUTION: unlike ReSID; this does NOT return the number of cycles "left to do" in int_delta_t */
retval = psid->sid->clock(int_delta_t, tmp_buf);
+ if (retval > nr) {
+ retval = nr;
+ }
{
int n, p = 0;
for (n = 0; n < retval; n++) {
@@ -305,8 +312,11 @@ static int residfp_calculate_samples(sound_t *psid, short *pbuf, int nr,
}
/* Used when SID does not run at system clock ("SID card") */
- tmp_buf = getbuf(2 * nr * psid->factor / 1000);
+ tmp_buf = getbuf((int)(sizeof(*tmp_buf) * (size_t)int_delta_t));
retval = psid->sid->clock(int_delta_t, tmp_buf);
+ if (retval > nr) {
+ retval = nr;
+ }
{
int n, p = 0;
for (n = 0; n < retval; n++) {
The sizeof(*tmp_buf) expression is intentional because getbuf() accepts a number of bytes rather than a number of samples.
The change was tested using the same AddressSanitizer build and runtime settings that reproduced the failure.
Results after applying the fix:
The failure was observed on macOS, but the defective code is in the generic non-float reSIDfp wrapper rather than in the CoreAudio backend.
It may therefore affect other operating systems or audio backends when SID::clock(cycles, buf) produces more samples than VICE predicted in nr.
The proposed minimal fix discards samples returned beyond the caller-provided nr capacity.
During the observed reset path, these samples cannot be copied safely and the sound state is immediately reset. A more elaborate implementation could preserve excess samples for a later call, but the proposed change is a minimal memory-safety correction that retains exact SID cycle advancement and prevents both temporary-buffer and destination-buffer overflow.
