JaySynth/TODO.md

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)

• 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).
• 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.
• 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.
• 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.
• 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.
• 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.
• 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.
• Found while building tools/testhost's regression suite for this list: the block-size fix above wasn't sufficient on its own. VCF_CalcCoeff_LPF/_HPF/_BPF (src/synth/vcf.c) advance the coefficient pointer once per (sample, filter-section) pair - a 4th-order filter uses FILTER_MAX_ORDER/2 = 2 cascaded sections per sample - but VCF_SetBufsize only allocated pCoef with bufsize entries, not bufsize * sections. Any host block between 4097 and 8192 samples (inclusive of the now-correctly-chunked range from the fix above) with a 4th-order filter selected wrote past the end of that buffer, corrupting adjacent heap memory (confirmed with AddressSanitizer: heap-buffer-overflow, WRITE of size 8, exactly 8 bytes past a 393216-byte/8192-vcf_coef_t allocation). Fixed by sizing the allocation for the worst case, bufsize*(FILTER_MAX_ORDER/2). Verified with ASan across the full range (4097, 8192, 16384, 20000 samples) after the fix - zero errors.

High — real-time audio-thread safety (fixed)

• 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.
• ~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.
• 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)

• 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.
• 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)

• 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.
• 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).
• .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.
• 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.