jensandClaude Sonnet 5
54a67562db
- JaySynth's destructor now uses delete[] to match every new T[n] allocation (m_pVoices, pPer_voice_controls, pCurrNoteInfos, humanize_voice_param[i], ppHumanizedSliders[i], m_ppAudioThread, m_ppEventAudioThreadRdy), fixing undefined behavior from the mismatched scalar delete. - Found and fixed the same new[]/delete mismatch pattern via ScopedPointer in PluginProcessor.cpp: ScopedPointer always calls scalar delete (per JUCE's own doc comment "do not give it an array to hold!"), so ScopedPointer<char> holding a new char[...] in setStateInformation/setCurrentProgramStateInformation had the same bug. Replaced both with HeapBlock<char>, JUCE's array-owning, malloc/free-backed smart pointer. - Added a shared SynthCheckAlloc() helper (synth_defs.h/synth_debug.c) that aborts with a diagnostic instead of returning NULL, and wrapped all 24 malloc call sites across the C DSP core (env.c, lfo.c, vcf.c, vco.c, blit.c, wavetable.c, voice.c, param_scale.c). All are one-time init/bufsize-change calls, never in the per-block render path, so this adds no real-time-thread overhead. Verified with clean debug and release builds (no new warnings/errors) and a full link. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011dhtwRLARk4eiPngcQykLJ
37 lines
6.7 KiB
Markdown
37 lines
6.7 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 (fixed)
|
||
|
||
- [x] **`new[]`/`delete` mismatches (UB) in `JaySynth`'s destructor** — `m_pVoices`, `pPer_voice_controls`, `pCurrNoteInfos`, `humanize_voice_param[i]`, `ppHumanizedSliders[i]`, `m_ppAudioThread`, `m_ppEventAudioThreadRdy` (`src/plug/JaySynth.cpp`) all now freed with `delete[]` to match their `new T[n]` allocations. Also found and fixed the same pattern via `ScopedPointer` in `PluginProcessor.cpp` (`setStateInformation`/`setCurrentProgramStateInformation`) — `ScopedPointer` always calls scalar `delete` (per JUCE's own doc comment), so `ScopedPointer<char> pUncompressedData = new char[...]` was the same bug; replaced with `HeapBlock<char>`, JUCE's array-owning smart pointer.
|
||
- [x] **`malloc` was never null-checked** across the C DSP core. Added a shared `SynthCheckAlloc()` helper (`synth_defs.h`/`synth_debug.c`) that aborts with a clear diagnostic instead of returning NULL, and wrapped all 24 `malloc` call sites across `env.c`, `lfo.c`, `vcf.c`, `vco.c`, `blit.c`, `wavetable.c`, `voice.c`, `param_scale.c`. All are one-time init/bufsize-change calls, never in the per-block render path, so this adds no real-time overhead.
|
||
|
||
## 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.
|