Add regression suite testing JaySynth's TODO.md findings via testhost
tools/testhost/tests/run_tests.sh dynamically exercises the tracked
hardening findings using testhost itself - no GUI, sub-second wall
time:
- oversized_blocksize (Critical #5): --blockSize 16384 under a
timeout, catching either the overflow or the deadlock the original
bug could cause.
- nrpn_out_of_range/nrpn_in_range (Critical #1): JSON scenarios
sending the 5-message MIDI NRPN CC sequence, once targeting the
max 14-bit ID (16383) and once a legitimate in-range ID (500), so
a regression that made the bounds check too aggressive would also
show up.
- patch_save_load_roundtrip / bank_save_load_roundtrip: save a
distinctive continuous parameter value, reload in a fresh process,
check it round-trips within tolerance - covers the
setCurrentProgramStateInformation no-op bug and the
patchDecodeXml/patchImportXml fixes.
- stability_sweep: loads every bundled .fxp under extras/sounds/ and
renders a held note through each, as a broad crash/NaN net.
Added --printParams to main.cpp (prints every parameter's index and
value) to make the round-trip tests possible - this was also already
a noted nice-to-have in testhost/TODO.md.
While building the oversized_blocksize test, found and fixed (on the
hardening branch, commit d713ca5) a second, previously-unknown bug:
VCF_CalcCoeff_LPF/_HPF/_BPF advance the 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 overflowed it for any block between 4097 and 8192 samples.
Found via bisecting the crashing block size then confirming with an
ASan build; see tests/README.md's debugging-notes section for exactly
how, including the ASan symbolizer hang encountered along the way and
the workaround.
Also discovered (documented in tools/testhost/TODO.md's Investigate
section, not fixed - out of scope): JaySynth's compiled VST2 wrapper
never advertises chunk support to the host, so real .fxp/.fxb sample
files have no effect when loaded through the generic AudioProcessor
state API. The round-trip tests work around this by using testhost's
own save output as the fixture rather than an external sample file.
All 10 checks pass against the current hardening-branch build.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011dhtwRLARk4eiPngcQykLJ
This commit is contained in:
@@ -0,0 +1,122 @@
|
||||
# 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). Uses `testhost`'s own save→load cycle as the fixture,
|
||||
not an externally-provided `.fxp`/`.fxb` — see the chunk-format note below
|
||||
for why.
|
||||
|
||||
**`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.
|
||||
|
||||
## Two things discovered while building this suite that are worth knowing
|
||||
|
||||
**The VCF coefficient-buffer bug above** (already fixed, see TODO.md).
|
||||
|
||||
**JaySynth's compiled VST2 wrapper never advertises chunk support to the
|
||||
host**, so loading the bundled sample `.fxp` files through `testhost`'s
|
||||
generic `--patch` (i.e. through `AudioProcessor::setCurrentProgramState-
|
||||
Information`, exactly what any real VST2 host uses) has **zero effect** —
|
||||
confirmed by diffing `--printParams` output with and without `--patch`.
|
||||
Those sample files are legitimately in VST2's opaque-chunk ("FPCh") format
|
||||
(verified via hex dump: correct `CcnK`/`FPCh`/`Jsy1` header), but JUCE
|
||||
3.1.1's plugin-side VST wrapper (`sdk/juce/JUCE-3.1.1/modules/
|
||||
juce_audio_plugin_client/VST/juce_VST_Wrapper.cpp`, compiled into
|
||||
`JaySynth.so`) never sets the VST2 `effFlagsProgramChunks` flag on the
|
||||
`AEffect` struct, so JUCE's own host-side `VSTPluginInstance::usesChunks()`
|
||||
sees false and `setChunkData()` silently no-ops — for both loading *and*
|
||||
saving. This is **not** one of the tracked `TODO.md` findings and this
|
||||
suite doesn't attempt to fix it (it would mean patching JUCE's own VST
|
||||
wrapper, a materially bigger and riskier change). It's why
|
||||
`stability_sweep` only checks "doesn't crash," not "changes the patch," and
|
||||
why the round-trip tests use `testhost`'s own save output as the fixture
|
||||
rather than a real sample file — `testhost`'s save path has the same
|
||||
`usesChunks()`-is-false behaviour, so save and load are at least
|
||||
consistent with *each other*, even though neither talks to the external
|
||||
`.fxp` files' actual chunk data. Tracked as a discovered issue 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.
|
||||
Reference in New Issue
Block a user