jens/JaySynth
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Hardening pass: critical/high/memory/patch-bank fixes + VCF buffer overflow #2

Merged
jens merged 7 commits from hardening into master 2026-07-27 20:44:34 +02:00
Conversation 0 Commits 7 Files Changed 17
+577 -157
jens commented 2026-07-27 20:40:27 +02:00
Owner
Copy Link
Copy Source

Summary

A code-review + hardening pass over src/plug and src/synth, tracked in the added TODO.md. Fixes are grouped by severity across 6 commits; every item is documented in TODO.md with file:line references and rationale.

Critical - crashes, memory corruption

  • NRPN controller ID out-of-bounds write: JaySynth::handleController wrote last_midiCC_info[midiCC_info.ID] with no bounds check; NRPN IDs can reach 16383 but the array is sized 1024. Now bounds-checked.
  • Negative controller IDs from patch files: JaySynthMidiCC::add/remove only rejected out-of-range-high IDs, not negative ones.
  • Null-pointer dereferences on malformed patch/bank files: missing null checks after getFirstChildElement()/XmlDocument::parse().
  • No size validation before casting raw file bytes to fxBank/fxProgram structs: a truncated/malformed .fxb/.fxp could read out of bounds.
  • Host block sizes larger than SYNTH_MAX_BUFSIZE (8192) caused a stack buffer overflow or a permanent deadlock: processBlock now chunks rendering; JaySynth::renderNextBlock clamps the same way.
  • Unchecked file stream in the constructor could crash on plugin construction if the wavetable file couldn't be opened.
  • Parameter-changing calls raced with real-time audio rendering: setParameter/setControl/setPerVoiceControl/setParam/ClearControls didn't take the synth's lock, unlike renderNextBlock and the note/controller handlers.
  • Found later, while building tools/testhost's regression suite for the block-size fix above: it wasn't sufficient on its own. VCF_CalcCoeff_LPF/_HPF/_BPF advance the coefficient buffer pointer once per (sample, filter-section) pair - a 4th-order filter uses 2 cascaded sections per sample - but the buffer was only ever allocated for bufsize samples, not bufsize × sections. Any host block between 4097 and 8192 samples with a 4th-order filter selected wrote past the allocation. Confirmed with AddressSanitizer: heap-buffer-overflow, WRITE of size 8, exactly 8 bytes past a 393216-byte (8192 × sizeof(vcf_coef_t)) allocation. Fixed by sizing the allocation for the worst case; re-verified with ASan across block sizes 4097/8192/16384/20000 - zero errors after the fix.

High - real-time audio-thread safety

  • Every note/CC/voice-count change allocated Strings and posted a message from the audio thread. synthChanged() now pushes a small POD event into a lock-free SPSC queue (JUCE's AbstractFifo); the existing 10ms UI timer drains it and does the String-building/posting on the message thread instead.
  • VoiceProcessDataV's ~450KB-900KB of SYNTH_MAX_BUFSIZE-sized locals are now persistent per-voice buffers allocated once, not re-allocated on the stack every audio block.
  • SynthDebug used unbounded vsprintf; now uses vsnprintf.

Memory management

  • new[]/delete mismatches (UB) in JaySynth's destructor, and the same pattern via ScopedPointer in PluginProcessor.cpp (which always calls scalar delete) - both fixed.
  • malloc was never null-checked anywhere in the C DSP core; added a shared SynthCheckAlloc() helper that aborts with a diagnostic instead of returning NULL, applied to all 24 call sites. All are one-time init/bufsize-change calls, not in the per-block render path.

Patch/Bank import-export

  • Legacy patch import decoded the wrong XML node (dead code path, but latent).
  • A much bigger bug found while investigating "regular .fxb/.fxp silently ignored": the magic-number check itself compared raw file bytes as text against JUCE's int-to-decimal-string constructor - these can never match, so the entire opaque .fxb/.fxp load path was dead code regardless of format. Fixed using ByteOrder::bigEndianInt, matching the pattern JUCE's own VST3 wrapper uses for the identical struct.
  • .fxp/.fxb saving was unimplemented; now implemented using the same opaque-chunk format the (now-fixed) load path expects.
  • patchDecodeXml/patchDecodeXml_legacy duplication removed.

Still open (tracked in TODO.md, not addressed here)

  • JaySynth is a god object (owns parameters, MIDI CC/NRPN, MIDI clock, humanize, unison, monophonic orchestration, voice stealing, and the thread pool with no internal boundaries).
  • void *the_editor duplicates JUCE's own editor tracking and is never cleared.

Verification

  • Clean debug and release builds throughout, no new compiler warnings.
  • Manual smoke-testing in Reaper (VST2 host) at each stage.
  • A standalone round-trip test of the .fxb/.fxp binary-format logic (magic detection, byte order, size/payload round-trip, safe rejection of truncated/bogus files).
  • The VCF coefficient-buffer bug was found and confirmed with an AddressSanitizer build (CONFIG=asan, -fsanitize=address on src/synth+src/plug), and the fix re-verified the same way across the full affected block-size range with zero errors.
  • A dynamic regression suite exists for these findings in tools/testhost/tests/ (see PR #1 / branch testhost), which is what surfaced the VCF bug in the first place.

Test plan

  • make (release) and make CONFIG=debug both build clean
  • Manual Reaper smoke test (load, play, save/load patch & bank)
  • Standalone .fxb/.fxp binary-format round-trip test
  • AddressSanitizer verification of the VCF fix across block sizes 4097/8192/16384/20000
  • tools/testhost/tests/run_tests.sh (branch testhost, PR #1) - 10/10 checks pass
## Summary A code-review + hardening pass over `src/plug` and `src/synth`, tracked in the added `TODO.md`. Fixes are grouped by severity across 6 commits; every item is documented in `TODO.md` with file:line references and rationale. ### Critical - crashes, memory corruption - **NRPN controller ID out-of-bounds write**: `JaySynth::handleController` wrote `last_midiCC_info[midiCC_info.ID]` with no bounds check; NRPN IDs can reach 16383 but the array is sized 1024. Now bounds-checked. - **Negative controller IDs from patch files**: `JaySynthMidiCC::add`/`remove` only rejected out-of-range-high IDs, not negative ones. - **Null-pointer dereferences on malformed patch/bank files**: missing null checks after `getFirstChildElement()`/`XmlDocument::parse()`. - **No size validation before casting raw file bytes to `fxBank`/`fxProgram` structs**: a truncated/malformed `.fxb`/`.fxp` could read out of bounds. - **Host block sizes larger than `SYNTH_MAX_BUFSIZE` (8192) caused a stack buffer overflow or a permanent deadlock**: `processBlock` now chunks rendering; `JaySynth::renderNextBlock` clamps the same way. - **Unchecked file stream in the constructor** could crash on plugin construction if the wavetable file couldn't be opened. - **Parameter-changing calls raced with real-time audio rendering**: `setParameter`/`setControl`/`setPerVoiceControl`/`setParam`/`ClearControls` didn't take the synth's lock, unlike `renderNextBlock` and the note/controller handlers. - **Found later, while building `tools/testhost`'s regression suite for the block-size fix above**: it wasn't sufficient on its own. `VCF_CalcCoeff_LPF/_HPF/_BPF` advance the coefficient buffer pointer once per *(sample, filter-section)* pair - a 4th-order filter uses 2 cascaded sections per sample - but the buffer was only ever allocated for `bufsize` samples, not `bufsize × sections`. Any host block between 4097 and 8192 samples with a 4th-order filter selected wrote past the allocation. **Confirmed with AddressSanitizer**: `heap-buffer-overflow`, `WRITE of size 8`, exactly 8 bytes past a 393216-byte (`8192 × sizeof(vcf_coef_t)`) allocation. Fixed by sizing the allocation for the worst case; re-verified with ASan across block sizes 4097/8192/16384/20000 - zero errors after the fix. ### High - real-time audio-thread safety - Every note/CC/voice-count change allocated `String`s and posted a message **from the audio thread**. `synthChanged()` now pushes a small POD event into a lock-free SPSC queue (JUCE's `AbstractFifo`); the existing 10ms UI timer drains it and does the String-building/posting on the message thread instead. - `VoiceProcessDataV`'s ~450KB-900KB of `SYNTH_MAX_BUFSIZE`-sized locals are now persistent per-voice buffers allocated once, not re-allocated on the stack every audio block. - `SynthDebug` used unbounded `vsprintf`; now uses `vsnprintf`. ### Memory management - `new[]`/`delete` mismatches (UB) in `JaySynth`'s destructor, and the same pattern via `ScopedPointer` in `PluginProcessor.cpp` (which always calls scalar `delete`) - both fixed. - `malloc` was never null-checked anywhere in the C DSP core; added a shared `SynthCheckAlloc()` helper that aborts with a diagnostic instead of returning NULL, applied to all 24 call sites. All are one-time init/bufsize-change calls, not in the per-block render path. ### Patch/Bank import-export - Legacy patch import decoded the wrong XML node (dead code path, but latent). - **A much bigger bug found while investigating "regular .fxb/.fxp silently ignored"**: the magic-number check itself compared raw file bytes as text against JUCE's int-to-decimal-string constructor - these can never match, so the entire opaque `.fxb`/`.fxp` load path was dead code regardless of format. Fixed using `ByteOrder::bigEndianInt`, matching the pattern JUCE's own VST3 wrapper uses for the identical struct. - `.fxp`/`.fxb` saving was unimplemented; now implemented using the same opaque-chunk format the (now-fixed) load path expects. - `patchDecodeXml`/`patchDecodeXml_legacy` duplication removed. ### Still open (tracked in TODO.md, not addressed here) - `JaySynth` is a god object (owns parameters, MIDI CC/NRPN, MIDI clock, humanize, unison, monophonic orchestration, voice stealing, and the thread pool with no internal boundaries). - `void *the_editor` duplicates JUCE's own editor tracking and is never cleared. ## Verification - Clean debug and release builds throughout, no new compiler warnings. - Manual smoke-testing in Reaper (VST2 host) at each stage. - A standalone round-trip test of the `.fxb`/`.fxp` binary-format logic (magic detection, byte order, size/payload round-trip, safe rejection of truncated/bogus files). - **The VCF coefficient-buffer bug was found and confirmed with an AddressSanitizer build** (`CONFIG=asan`, `-fsanitize=address` on `src/synth`+`src/plug`), and the fix re-verified the same way across the full affected block-size range with zero errors. - A dynamic regression suite exists for these findings in `tools/testhost/tests/` (see PR #1 / branch `testhost`), which is what surfaced the VCF bug in the first place. ## Test plan - [x] `make` (release) and `make CONFIG=debug` both build clean - [x] Manual Reaper smoke test (load, play, save/load patch & bank) - [x] Standalone `.fxb`/`.fxp` binary-format round-trip test - [x] AddressSanitizer verification of the VCF fix across block sizes 4097/8192/16384/20000 - [x] `tools/testhost/tests/run_tests.sh` (branch `testhost`, PR #1) - 10/10 checks pass
jens added 7 commits 2026-07-27 20:40:28 +02:00
Add CLAUDE.md with codebase architecture and build guidance 468b4c36de 
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011dhtwRLARk4eiPngcQykLJ
Fix critical memory-safety and concurrency bugs found in code review a65b71ab86 
- handleController: bounds-check NRPN/CC controller ID before indexing
  last_midiCC_info[] (NRPN IDs can reach 16383, array is sized 1024)
- JaySynthMidiCC::add/remove: also reject negative controller IDs coming
  from patch/bank file data, not just out-of-range-high ones
- bankImportXml/loadPatchFromFile: null-check XML elements after
  getFirstChildElement()/XmlDocument::parse() before dereferencing, so
  malformed/truncated patch or bank files no longer crash on load
- loadBankFromFile/loadPatchFromFile: validate the loaded file is large
  enough for the fxBank/fxProgram header, and that the embedded chunk
  size fits within the file, before reading through the raw pointer
- processBlock/JaySynth::renderNextBlock: host blocks larger than
  SYNTH_MAX_BUFSIZE no longer overflow the fixed-size stack work buffers
  or deadlock the render worker threads; oversized blocks are now
  rendered in multiple SYNTH_MAX_BUFSIZE-sized passes (pending MIDI
  events are correctly held over between passes)
- JaySynth constructor: guard against createInputStream() returning null
  when the wavetable file can't be opened
- JaySynth::setParameter/setControl/setPerVoiceControl/setParam/
  ClearControls: take the synth's lock, matching renderNextBlock and the
  note/controller handlers, to close a data race between parameter
  changes (UI/host automation) and audio-thread rendering

Verified with a full rebuild (make) producing JaySynth.so with no new
warnings or errors.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011dhtwRLARk4eiPngcQykLJ
Add TODO.md tracking hardening review findings 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
Fix High-severity real-time audio-thread safety issues from TODO.md 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
Fix Memory-management issues from TODO.md 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
Fix Patch/Bank import-export issues from TODO.md 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
Fix VCF coefficient buffer undersized for 4th-order filters d713ca50bb 
Found while building tools/testhost's regression suite for the
Critical #5 block-size fix: VCF_CalcCoeff_LPF/_HPF/_BPF advance the
coefficient pointer once per (sample, filter-section) pair, but a
4th-order filter uses FILTER_MAX_ORDER/2 = 2 cascaded 2nd-order
sections per sample. VCF_SetBufsize only allocated pCoef with bufsize
entries, not bufsize*sections, so any host block between 4097 and
8192 samples with a 4th-order filter selected wrote past the
allocation.

Confirmed with AddressSanitizer before the fix: heap-buffer-overflow,
WRITE of size 8, exactly 8 bytes past a 393216-byte (8192 x
sizeof(vcf_coef_t)) allocation. Fixed by sizing pCoef for the worst
case. Re-verified with ASan across blockSize 4097/8192/16384/20000 -
zero errors after the fix.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011dhtwRLARk4eiPngcQykLJ
jens closed this pull request 2026-07-27 20:41:47 +02:00
jens reopened this pull request 2026-07-27 20:44:10 +02:00
jens merged commit 161f57bc4e into master 2026-07-27 20:44:34 +02:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
jens (Jens Ahrensfeld)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: jens/JaySynth#2