jensandClaude Sonnet 5
cee75c523b
- patchImportXml: handle the legacy single-patch format (JSYNTH_PATCH root) as its own top-level branch instead of incorrectly nested inside the modern per-child loop, which checked/decoded the outer xml on every iteration instead of the loop variable xml2. Switched its CC-table import to import_midi_cc, matching the convention already used (and exercised) by bankDecodeXml_legacy. loadPatchFromFile's .xmp branch now also accepts legacy-rooted files. - setCurrentProgramStateInformation: fix a related bug found in passing - it was passing patchImportXml an XML node one level too deep, making the VST2 "copy plugin state to another track" feature a complete no-op. - Fix the fundamentally broken fxb/fxp magic-number check: it compared raw file bytes (as text, e.g. "CcnK") against JUCE's int-to-decimal- text String constructor (e.g. "1130589771"), which can never match. This meant the entire opaque .fxb/.fxp load path was dead code. Fixed using ByteOrder::bigEndianInt, matching the pattern JUCE's own VST3 wrapper uses for the identical struct. "Regular" (non-chunk) .fxb/.fxp is still unsupported, but now shows an AlertWindow explaining why instead of silently doing nothing. - Implement .fxp/.fxb saving using the same opaque-chunk format the load path expects, reusing getCurrentProgramStateInformation/ getStateInformation for the payload. - Deduplicate patchDecodeXml/patchDecodeXml_legacy (the latter now just delegates to the former). Verified with clean debug/release builds and a standalone round-trip test of the binary format logic (magic detection, byte order, size/payload round-trip, safe rejection of truncated/bogus files). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011dhtwRLARk4eiPngcQykLJ
37 lines
7.7 KiB
Markdown
37 lines
7.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 (fixed)
|
||
|
||
- [x] **Legacy patch import decoded the wrong XML node.** `patchImportXml` now handles the legacy single-patch format (`JSYNTH_PATCH` root) as its own top-level branch instead of incorrectly nested inside the modern per-child loop, which checked/decoded the outer `xml` on every iteration instead of the loop variable. Also switched its CC-table import to `import_midi_cc` (matching the actually-exercised convention already used by `bankDecodeXml_legacy`, since this branch was previously unreachable dead code). `loadPatchFromFile`'s `.xmp` branch now also accepts legacy-rooted files, so single legacy patches can actually be loaded. Also fixed a related bug found in passing: `setCurrentProgramStateInformation` (VST2 "copy plugin state to another track") was passing `patchImportXml` an XML node one level too deep, making it a complete no-op.
|
||
- [x] **Found a much bigger bug while investigating "regular .fxb/.fxp silently ignored": the magic-number check itself was completely broken.** It compared `String((const char*)&bank->chunkMagic)` (raw file bytes as text, e.g. `"CcnK"`) against `String(cMagic)` (JUCE's `int`-to-*decimal-text* constructor, e.g. `"1130589771"`) — these can never be equal, so **the entire opaque `.fxb`/`.fxp` load path was dead code**, regardless of format. Fixed using `ByteOrder::bigEndianInt`, the correct byte-order-aware comparison (matches the pattern JUCE's own VST3 wrapper uses for the identical struct). "Regular" (non-chunk) format is still not supported for load, but now surfaces an `AlertWindow` explaining why instead of silently doing nothing. Verified with a standalone round-trip test of the binary format logic (magic detection, size/payload round-trip, safe rejection of truncated/bogus files).
|
||
- [x] **`.fxp`/`.fxb` saving was unimplemented.** Now implemented using the same opaque-chunk format the (now-fixed) load path expects, reusing `getCurrentProgramStateInformation`/`getStateInformation` for the payload.
|
||
- [x] **`patchDecodeXml`/`patchDecodeXml_legacy` duplication** — `_legacy` now just delegates to `patchDecodeXml`.
|
||
|
||
## 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.
|