jensandClaude Sonnet 5
e151036963
Records the critical fixes already applied (commit a65b71a) and the
remaining high/memory/patch-export/design findings from the code
review, for later reference.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011dhtwRLARk4eiPngcQykLJ
37 lines
6.1 KiB
Markdown
37 lines
6.1 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
|
||
|
||
- [ ] **Every note/CC/voice-count change allocates Strings and posts a message from the audio thread.** `JaySynthActionListener::call*` (`src/plug/PluginProcessor.h:41-127`) build messages via chained `String` concatenation, invoked synchronously from `synthChanged` inside `noteOn`/`noteOff`/`handleController`/`Voicestart`/`renderNextBlock` — all on the audio thread whenever the GUI is open. Fix: pass typed structs instead of encoding into Strings, or hop to the message thread before allocating.
|
||
- [ ] **~450KB–900KB of stack arrays allocated per voice per audio block.** `VoiceProcessDataV` (`src/synth/voice.c:836-858`) declares ~11 `SYNTH_MAX_BUFSIZE`-sized locals, called once per active voice per block from the worker threads. Fix: hoist these into `voice_common_t`/`voice_t` as pre-allocated buffers (same pattern already used by `vcf.c`/`env.c`/`lfo.c`/`blit.c`).
|
||
- [ ] **`SynthDebug` uses unbounded `vsprintf` into a fixed 1024-byte buffer plus blocking I/O**, called from `handleMidiEvent` inside the audio-thread render path (`src/synth/synth_debug.c:27-38`). Fix: use `vsnprintf` with the buffer size, and consider a lock-free ring buffer instead of blocking I/O for debug builds.
|
||
|
||
## 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.
|