jensandClaude Sonnet 5
608ae003df
- synthChanged() no longer builds JaySynthActionListener Strings and calls sendActionMessage directly on the audio thread. It now pushes a small POD event into a lock-free SPSC queue (JUCE's AbstractFifo), drained by the existing 10ms UI timer on the message thread, which does the String-building/posting there instead. - VoiceProcessDataV's ~450KB-900KB of SYNTH_MAX_BUFSIZE-sized local arrays are now persistent per-voice buffers allocated once in VoiceSetBufsize (freed in VoiceFree), matching the pattern already used for pBuf_Q and by vcf.c/env.c/lfo.c, instead of being reallocated on the stack on every audio block. - SynthDebug uses vsnprintf instead of vsprintf, bounding writes to its 1024-byte buffer. Verified with clean debug and release builds (no new warnings) and a live playback smoke test in Reaper. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011dhtwRLARk4eiPngcQykLJ
37 lines
6.2 KiB
Markdown
37 lines
6.2 KiB
Markdown
# Hardening todo
|
||
|
||
Findings from a code-review pass over `src/plug` and `src/synth` (2026-07-27, branch `hardening`).
|
||
|
||
## Critical — crashes, memory corruption (fixed, commit `a65b71a`)
|
||
|
||
- [x] **NRPN controller ID caused an out-of-bounds write on the audio thread.** `JaySynth::handleController` (`src/plug/JaySynth.cpp:899`) now bounds-checks `midiCC_info.ID` against `NUM_MIDI_CONTROLLERS` before indexing `last_midiCC_info[]` (NRPN IDs can reach 16383, array is sized 1024).
|
||
- [x] **Negative controller IDs from patch files could corrupt memory.** `JaySynthMidiCC::add`/`remove` (`src/plug/JaySynthMidiCC.cpp:433-460`) now also reject `< 0`, not just out-of-range-high.
|
||
- [x] **Null-pointer dereference on malformed/unparsable patch or bank files.** `bankImportXml` and the `.xmp` branch of `loadPatchFromFile` (`src/plug/PluginProcessor.cpp:860-885, 949-975`) now null-check XML elements after `getFirstChildElement()`/`XmlDocument::parse()` before dereferencing.
|
||
- [x] **No size validation before casting raw file bytes to `fxBank`/`fxProgram` structs.** `loadBankFromFile`/`loadPatchFromFile` (`src/plug/PluginProcessor.cpp:834-858, 930-957`) now check the loaded file is at least header-sized and that the embedded chunk size fits within it before reading through the raw pointer.
|
||
- [x] **Host block sizes larger than `SYNTH_MAX_BUFSIZE` (8192) caused a stack buffer overflow or a permanent deadlock.** `processBlock` (`src/plug/PluginProcessor.cpp:547`) now chunks rendering into `SYNTH_MAX_BUFSIZE`-sized passes instead of writing the host's full block size into a fixed 8192-sample stack buffer; `JaySynth::renderNextBlock` (`src/plug/JaySynth.cpp:977`) clamps the same way and correctly holds a pending MIDI event over between passes.
|
||
- [x] **Unchecked file stream could be null, crashing on plugin construction.** `JaySynth::JaySynth` (`src/plug/JaySynth.cpp:55-57`) now guards the wavetable file read against `createInputStream()` returning null.
|
||
- [x] **Parameter-changing calls raced with real-time audio rendering — no lock protected them.** `JaySynth::setParameter`/`setControl`/`setPerVoiceControl`/`setParam`/`ClearControls` (`src/plug/JaySynth.cpp`) now take `ScopedLock sl(lock)`, matching `renderNextBlock` and the note/controller handlers.
|
||
|
||
## High — real-time audio-thread safety (fixed)
|
||
|
||
- [x] **Every note/CC/voice-count change allocated Strings and posted a message from the audio thread.** `synthChanged()` (`src/plug/PluginProcessor.cpp`) no longer calls `JaySynthActionListener::call*` directly from the audio thread. It now pushes a small POD event into a lock-free single-producer/single-consumer queue (JUCE's `AbstractFifo`, added to `PluginProcessor.h`), and the existing 10ms UI `Timer` (`timerCallback`/`dispatchUiEvents`) drains it on the message thread, where the String-building and `sendActionMessage` now safely happen.
|
||
- [x] **~450KB–900KB of stack arrays allocated per voice per audio block.** `VoiceProcessDataV`'s 11 `SYNTH_MAX_BUFSIZE`-sized locals (`src/synth/voice.c`) are now persistent per-voice buffers (`pBuf_VCO_fmout`, `pBuf_vco_pwm`, etc., declared in `voice.h`), allocated once in `VoiceSetBufsize`/freed in `VoiceFree` — same lifecycle/pattern already used by `pBuf_Q` and by `vcf.c`/`env.c`/`lfo.c`.
|
||
- [x] **`SynthDebug` used unbounded `vsprintf` into a fixed 1024-byte buffer.** `src/synth/synth_debug.c` now uses `vsnprintf` with the buffer size. (Left the blocking `puts()`/`OutputDebugString` I/O as-is — it's compiled out entirely unless `SYNTH_DEBUG` is defined, so release builds are unaffected, and a lock-free logging redesign felt like overreach for a debug-only path.)
|
||
|
||
## Memory management
|
||
|
||
- [ ] **`new[]`/`delete` mismatches (UB) in `JaySynth`'s destructor** — `m_pVoices`, `pPer_voice_controls`, `pCurrNoteInfos`, `humanize_voice_param[i]`, `ppHumanizedSliders[i]`, `m_ppAudioThread`, `m_ppEventAudioThreadRdy` all allocated with `new T[n]` but freed with scalar `delete` (`src/plug/JaySynth.cpp:62/186, 77/184, 80/185, 100-101/173-174, 124-125/160,191`). Fix: change to `delete[]` at each site.
|
||
- [ ] **`malloc` is never null-checked** across the entire C DSP core (`env.c`, `lfo.c`, `vcf.c`, `vco.c`, `blit.c`, `wavetable.c`, `voice.c`, `param_scale.c`). Fix: check and fail gracefully (or `abort()` with a diagnostic) rather than crashing on first audio callback.
|
||
|
||
## Patch/Bank import-export
|
||
|
||
- [ ] **Legacy patch import decodes the wrong XML node** — uses outer `xml` instead of loop variable `xml2` in the legacy branch (`src/plug/PluginProcessor.cpp:959-991`), unlike every other branch; currently masked but re-decodes redundantly and is a latent bug if the legacy format gains child elements.
|
||
- [ ] **"Regular" (non-chunk) `.fxb`/`.fxp` files are silently accepted but never loaded** — `is_regular` is computed and never used (`src/plug/PluginProcessor.cpp:834-852`, `930-948`). Fix: either implement the regular-format path or surface an error to the user.
|
||
- [ ] **`.fxp`/`.fxb` saving is entirely unimplemented** (`savePatchToFile`/`saveBankToFile`, `src/plug/PluginProcessor.cpp:1027-1067`, both marked `// ToDo`) while loading is implemented — asymmetric format support.
|
||
- [ ] **`patchDecodeXml` and `patchDecodeXml_legacy` are near-duplicated**, including a copy-pasted legacy frequency→cents conversion special case (`src/plug/PluginProcessor.cpp:749-765`, `767-783`). Fix: factor out the shared per-parameter decode loop.
|
||
|
||
## Design
|
||
|
||
- [ ] **`JaySynth` is a god object** (`src/plug/JaySynth.h`) — owns parameters, MIDI CC/NRPN, MIDI clock, 3 humanize subsystems, unison, monophonic orchestration, voice stealing, and the thread pool, all via direct field access with no internal boundaries. This is very plausibly why the parameter-locking bug (now fixed) was missed in the first place — no seam enforces consistent locking discipline. Consider splitting into a handful of focused collaborators (e.g. MIDI dispatch, voice allocation, humanize/unison modes) that `JaySynth` composes.
|
||
- [ ] **`void *the_editor` (`src/plug/PluginProcessor.h:394`) duplicates JUCE's own editor tracking**, is never cleared when the editor is destroyed, and every other call site correctly uses `getEditor(this)` instead — currently harmless but a dangling-pointer trap. Fix: remove the field and route `createEditor()` through `getEditor(this)` like everywhere else.
|