jensandClaude Sonnet 5
8194bda9c8
Add Vst2RawChunk, a raw effGetChunk/effSetChunk bypass around JUCE's VSTPluginInstance::usesChunks() gate, plus --loadBankChunk/--saveBankChunk/ --loadPatchChunk/--savePatchChunk CLI flags and a new test_chunk_roundtrip regression test asserting the saved file is genuinely zlib/XML (0x78 header, >1000 bytes), not a per-parameter fallback. Building and using this bypass disproved an earlier documented finding: the compiled VST2 wrapper does set effFlagsProgramChunks and usesChunks() is genuinely true, so the previous "chunk-advertisement gap" theory in TODO.md and tests/README.md was wrong. Corrected in both places. The real cause of old bundled .fxp sample files having no effect remains open and is now narrowed to a JaySynth-side XML/schema parsing question, not a host/plugin capability mismatch. Also adds a PluginHost-method-to-VST2-wiring-to-test coverage table in tests/README.md, and documents the remaining "exercised but not asserted" gaps (channel/param counts, DSP correctness) in TODO.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011dhtwRLARk4eiPngcQykLJ
171 lines
12 KiB
Markdown
171 lines
12 KiB
Markdown
# Regression suite for JaySynth's hardening findings
|
||
|
||
`run_tests.sh` uses `testhost` itself to dynamically exercise the findings
|
||
tracked in the repo-root `TODO.md` (the "hardening" review) — no GUI, no
|
||
manual clicking, runs in well under a second of wall-clock time.
|
||
|
||
## Running
|
||
|
||
```sh
|
||
MAKE_HOME=/path/to/submodule/make ./run_tests.sh [path/to/JaySynth.so]
|
||
```
|
||
|
||
Builds `testhost` first if it isn't already built. Defaults to
|
||
`build/linux/release/JaySynth.so` if no path is given. Exit code is 0 if
|
||
everything passed, 1 otherwise (CI-friendly). On failure, per-test logs and
|
||
WAVs are kept in a `mktemp -d` workdir printed at the end; on success
|
||
that workdir is deleted.
|
||
|
||
## What each test actually covers
|
||
|
||
**`oversized_blocksize`** (Critical #5) — renders through `--blockSize
|
||
16384` (double `SYNTH_MAX_BUFSIZE`). Before the fix this either overflowed
|
||
`processBlock`'s fixed 8192-sample stack buffer or deadlocked the render
|
||
worker threads; `timeout` is what actually catches the deadlock case, since
|
||
a hung process would otherwise just sit here forever. **This test is also
|
||
what caught a second, previously-unknown bug**: `VCF_CalcCoeff_LPF/_HPF/_BPF`
|
||
advance their coefficient-buffer pointer once per *(sample, filter-section)*
|
||
pair, but the buffer was only ever allocated for `bufsize` samples, not
|
||
`bufsize × sections` — a 4th-order filter (2 sections) overflowed it for
|
||
any block between 4097 and 8192 samples, a range this fix's own "chunk to
|
||
≤8192" logic doesn't protect against on its own. Found via AddressSanitizer
|
||
after this test kept crashing at blockSize values *below* 8192; fixed in
|
||
`src/synth/vcf.c` (see TODO.md's Critical section). If this test ever
|
||
regresses again, don't assume it's the same bug — bisect the block size
|
||
first (see the debugging notes at the bottom of this file for how that
|
||
investigation went).
|
||
|
||
**`nrpn_out_of_range`/`nrpn_in_range`** (Critical #1) — sends the standard
|
||
5-message MIDI NRPN CC sequence (99/98/6/38/98) via a JSON scenario, once
|
||
targeting the maximum possible 14-bit NRPN ID (16383) and once a legitimate
|
||
in-range ID (500). Both must complete cleanly; the in-range case exists so
|
||
a regression that made the bounds check *too* aggressive (rejecting valid
|
||
IDs) would also be visible as a behavioural difference. **Caveat:** "doesn't
|
||
crash" is a necessary but not fully sufficient check here. The original bug
|
||
was an out-of-bounds *write* into `last_midiCC_info[]`, a plain array
|
||
embedded inside the larger heap-allocated `JaySynth` object — not a
|
||
separate heap allocation — so an overflow of a few hundred entries lands in
|
||
*other member fields of the same object* rather than past its heap
|
||
allocation. That means it can silently corrupt adjacent state without
|
||
necessarily crashing in a short test run, and it's why the ID is pushed to
|
||
the actual maximum (16383) rather than something more modest: to maximize
|
||
the chance that, absent the fix, the write would land far enough away to
|
||
hit unrelated memory and actually crash. The real guarantee is the
|
||
bounds-check line itself (`src/plug/JaySynth.cpp`, `handleController`), not
|
||
this test's silence.
|
||
|
||
**`patch_save_load_roundtrip`** / **`bank_save_load_roundtrip`** — covers
|
||
two related fixes: (a) `setCurrentProgramStateInformation` used to pass
|
||
`patchImportXml` an XML node one level too deep, making the VST2 "copy
|
||
plugin state" path a complete no-op; (b) the `patchDecodeXml`/
|
||
`patchDecodeXml_legacy` deduplication and `patchImportXml`'s legacy-branch
|
||
restructuring. Sets a continuous parameter (index 0 = `SYNTH_PARAM_VOLUME`)
|
||
to a distinctive value, saves, reloads in a fresh process, and checks the
|
||
value reads back within a small tolerance (not exact equality — the
|
||
slider/internal-value scaling curve, `toParam`/`toSlider`, isn't bit-exact
|
||
across a round trip). Goes through JUCE's generic `AudioProcessor::get/
|
||
setStateInformation`/`get/setCurrentProgramStateInformation` - see the
|
||
corrected-finding note below for what that path actually does.
|
||
|
||
**`bank_chunk_roundtrip`** — same round-trip, but via `effGetChunk`/
|
||
`effSetChunk` called *directly* on the raw `AEffect*` (`Vst2RawChunk.h`/
|
||
`--saveBankChunk`/`--loadBankChunk`), bypassing JUCE's host-side wrapper
|
||
entirely. Also positively confirms the real chunk path was used (not just
|
||
that a parameter round-trips): checks the saved file starts with a zlib
|
||
stream header (`0x78`) and is far larger than a raw 140-parameter float
|
||
array could be (140×4 = 560 bytes) - a plain per-parameter fallback
|
||
wouldn't look anything like this.
|
||
|
||
**`stability_sweep`** — loads every bundled `.fxp` under `extras/sounds/`
|
||
and renders a held note through each, checking for crashes/hangs/NaN. A
|
||
broad regression net, not a targeted test for any one finding. See the
|
||
corrected-finding note below for why this only checks "doesn't crash," not
|
||
"changes the patch."
|
||
|
||
## `PluginHost` method coverage table
|
||
|
||
Every method on `PluginHost` (`src/PluginHost.h`), what it actually wires
|
||
to on the VST2 side for a hosted plugin, and which test(s) exercise it.
|
||
"Exercised" means the call happens during that test; "asserted" means the
|
||
test actually checks something about its result, not just that it ran
|
||
without crashing.
|
||
|
||
| `PluginHost` method | VST2 wiring | Covering test(s) |
|
||
|---|---|---|
|
||
| `load()` | `effOpen` (via `createPluginInstance`), then inside `prepareToPlay`: `effSetSampleRate`, `effSetBlockSize`, `effMainsChanged(1)`, `effStartProcess` | Every test - exercised, not specifically asserted beyond "didn't return false" |
|
||
| `releaseResources()` | `effMainsChanged(0)`/`effStopProcess` (`VSTPluginInstance::releaseResources`) | Every test calls it before exit (`main.cpp`) - exercised only, no assertion |
|
||
| `getNumInputChannels()` / `getNumOutputChannels()` | Reads cached `AEffect::numInputs`/`numOutputs`, populated at `effOpen` | Every test (printed in the `"loaded ... (N in/N out...)"` banner) - exercised, not asserted against an expected value for any specific plugin |
|
||
| `getNumParameters()` | Reads cached `AEffect::numParams` | Every test (same load banner; also implicit in every `--printParams` use) - exercised, not asserted |
|
||
| `getParameter(index)` | `AEffect::getParameter` callback | `patch_save_load_roundtrip`, `bank_save_load_roundtrip`, `bank_chunk_roundtrip`, `patch_chunk_roundtrip` (all via `--printParams`) - asserted (value checked within tolerance) |
|
||
| `setParameter(index, value)` | `AEffect::setParameter` callback | Same four round-trip tests (via `--param`) - asserted indirectly (the value it sets is what gets read back) |
|
||
| `process()` - audio only | `processReplacing`/`processDoubleReplacing` | Every test - asserted only for NaN/Inf absence, not correctness of the DSP output itself |
|
||
| `process()` - MIDI note on/off | `effProcessEvents` (`VstMidiEvent`) | `oversized_blocksize`, `nrpn_out_of_range`, `nrpn_in_range`, `stability_sweep` (all use `--note`) - exercised (audio is produced), not asserted beyond non-silence/no-NaN |
|
||
| `process()` - MIDI CC/NRPN | `effProcessEvents` (`VstMidiEvent`, controller sub-type) | `nrpn_out_of_range`, `nrpn_in_range` - exercised and the specific regression (bounds check) is asserted via clean completion |
|
||
| `getCurrentProgramStateInformation()` | `saveToFXBFile(isFXB=false)` → `effGetChunk(index=1)` (confirmed chunk-based, see below) | `patch_save_load_roundtrip` - asserted |
|
||
| `setCurrentProgramStateInformation()` | `loadFromFXBFile` → `effSetChunk(index=1)` | `patch_save_load_roundtrip` - asserted; also used by `stability_sweep` via `--patch`, but **not asserted there** - see the corrected-finding note, those particular files don't actually change anything |
|
||
| `getStateInformation()` | `saveToFXBFile(isFXB=true)` → `effGetChunk(index=0)` | `bank_save_load_roundtrip` - asserted |
|
||
| `setStateInformation()` | `loadFromFXBFile` → `effSetChunk(index=0)` | `bank_save_load_roundtrip` - asserted |
|
||
| `getInstance()` | n/a (plain accessor, returns the raw `AudioPluginInstance*`) | Used internally by every `Vst2RawChunk`-based test to reach the raw `AEffect*` |
|
||
| *(bypass, not on `PluginHost`)* `Vst2RawChunk::getChunk`/`setChunk` | `effGetChunk`/`effSetChunk` called directly on the raw `AEffect*`, bypassing `usesChunks()` and `AudioProcessor` entirely | `bank_chunk_roundtrip`, `patch_chunk_roundtrip` - asserted, plus the zlib-header/size check that positively confirms the real chunk path ran |
|
||
|
||
**Known gaps**: `load()`/`releaseResources()`/`getNumInputChannels()`/
|
||
`getNumOutputChannels()`/`getNumParameters()` are exercised by literally
|
||
every test run but never *asserted* against a specific expected value for
|
||
a specific plugin (e.g. nothing currently pins "JaySynth must report
|
||
exactly 0 in / 2 out / 140 parameters" - a change to any of those would go
|
||
unnoticed by this suite unless it also broke something else). Same for
|
||
`process()`'s actual DSP correctness - the suite checks "ran without
|
||
crashing/NaN," never "the audio sounds right." Both are reasonable
|
||
follow-ups noted in `tools/testhost/TODO.md`.
|
||
|
||
## Corrected finding: it's not a chunk-advertisement gap after all
|
||
|
||
An earlier version of this suite (and `tools/testhost/TODO.md`) claimed
|
||
that JaySynth's compiled VST2 wrapper never sets `effFlagsProgramChunks`,
|
||
so JUCE's host-side `usesChunks()` would see false and silently skip the
|
||
chunk mechanism - offered as the reason bundled sample `.fxp` files have no
|
||
effect when loaded via `--patch`. **That diagnosis was wrong.** It's
|
||
disproved directly: `--saveBank`/`--savePatch` (the exact same generic
|
||
`AudioProcessor::getStateInformation` call) produce real `FBCh`/`FPCh`-
|
||
magic files containing a genuine zlib-compressed XML blob (confirmed via
|
||
hex dump), not a per-parameter fallback - meaning `usesChunks()` is
|
||
genuinely `true`. To be certain, `Vst2RawChunk` was added specifically to
|
||
rule out any remaining JUCE-side gating: it calls `effGetChunk`/
|
||
`effSetChunk` straight on the raw `AEffect*`, and feeding a bundled sample
|
||
file's inner chunk bytes through it *this way* still has zero effect.
|
||
`test_chunk_roundtrip` above proves this exact mechanism round-trips
|
||
correctly for files this build produces.
|
||
|
||
So the real, still-open question is narrower: **why do these specific
|
||
(10+ year old) external sample files fail to apply**, given the mechanism
|
||
that's supposed to load them demonstrably works? Most likely a compression
|
||
or XML-schema shape from an older JaySynth/JUCE combination that
|
||
`JaySynthAudioProcessor::setCurrentProgramStateInformation`/
|
||
`getXmlFromBinary` no longer parses - not investigated further here (would
|
||
mean decompressing and diffing one of these files' XML against what this
|
||
build produces). Not one of the tracked repo-root `TODO.md` findings.
|
||
Tracked in `tools/testhost/TODO.md`'s "Investigate" section.
|
||
|
||
## Debugging notes: how the VCF bug was actually found
|
||
|
||
`oversized_blocksize` initially failed with `free(): invalid next size
|
||
(normal)` - glibc's heap-corruption detector, tripped during process
|
||
teardown, not at the point of the actual overflow. Bisecting `--blockSize`
|
||
found the real threshold was 4096/4097, not 8192 as expected. Rather than
|
||
guess further, an ASan build of just `src/synth` + `src/plug` (env-var
|
||
override: `CFLAGS/CXXFLAGS="-fsanitize=address -fno-omit-frame-pointer -g
|
||
-O1" LDFLAGS="-fsanitize=address"`, `CONFIG=asan` to keep it in its own
|
||
build directory) run via `LD_PRELOAD=$(clang -print-file-name=libclang_rt.
|
||
asan-x86_64.so)` pinpointed it exactly. Two gotchas worth remembering if
|
||
this needs doing again: (1) `ASAN_OPTIONS=symbolize=0` is necessary in this
|
||
environment — the default external `llvm-symbolizer` fork hangs
|
||
indefinitely rather than resolving addresses, so use `symbolize=0` and
|
||
resolve the reported `JaySynth.so+0x...` offsets with `addr2line -e
|
||
JaySynth.so <offset>` (or just `nm`/`objdump` if `addr2line` needs exact
|
||
offsets adjusted for the binary's load bias) instead of trusting ASan's own
|
||
backtrace; (2) rebuilding the ASan variant needs a real `git worktree`
|
||
avoided here — `sdk/juce/JUCE-3.1.1` is vendored as a zip+patch pair
|
||
extracted on demand, not a plain tracked directory, so a fresh worktree
|
||
doesn't have it. Simpler to `git stash` uncommitted work and build the
|
||
ASan variant directly on the branch with the fix under investigation.
|