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
7 Commits
Author SHA1 Message Date
jensandClaude Sonnet 5 d713ca50bb Fix VCF coefficient buffer undersized for 4th-order filters
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
2026-07-27 20:02:04 +02:00
jensandClaude Sonnet 5 cee75c523b Fix Patch/Bank import-export issues from TODO.md
- 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
2026-07-27 18:32:52 +02:00
jensandClaude Sonnet 5 54a67562db Fix Memory-management issues from TODO.md
- 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
2026-07-27 18:19:56 +02:00
jensandClaude Sonnet 5 608ae003df Fix High-severity real-time audio-thread safety issues from TODO.md
- 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
2026-07-27 18:08:35 +02:00
jensandClaude Sonnet 5 e151036963 Add TODO.md tracking hardening review findings
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
2026-07-27 17:26:41 +02:00
jensandClaude Sonnet 5 a65b71ab86 Fix critical memory-safety and concurrency bugs found in code review
- 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
2026-07-27 17:22:46 +02:00
jensandClaude Sonnet 5 468b4c36de Add CLAUDE.md with codebase architecture and build guidance
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011dhtwRLARk4eiPngcQykLJ
2026-07-27 17:14:24 +02:00