Skip to content

[BUGFIX] Split WaveNet::process() calls larger than the max buffer size - #1

Draft
squatchlr wants to merge 2 commits into
squatchfrom
submit/oversized
Draft

squatchlr wants to merge 2 commits into
squatchfrom
submit/oversized

Conversation

@squatchlr

Copy link
Copy Markdown
Collaborator

Related: sdatkinson#303

Summary

WaveNet::process() sizes every buffer for the maxBufferSize given to
Reset(), and only asserts that a call fits (model.cpp:827). In a release
build, a larger call writes past the condition array (:820), every layer's
conv, mixin and activation buffers, and the conv ring buffers whose lookback is
shorter than the excess. ASan reports a heap-buffer-overflow for every generic
WaveNet in example_models/.

This PR processes such a call as consecutive calls of at most mMaxBufferSize
frames. The chunk pointers are sized in the constructor, so nothing allocates,
and state carries over between chunks. The recursive call is qualified, so a
subclass override isn't re-entered. The layer-array loop moves unchanged into
_process_layer_arrays(), which keeps process() at its current complexity.

flowchart LR
    accTitle: WaveNet::process with the fix
    accDescr: A call larger than the maximum buffer size is split into consecutive calls of at most that size; each call then runs the unchanged path.
    n_call["WaveNet::process(input, output, n)"] --> n_fits{"n > mMaxBufferSize?"}
    n_fits -- no --> n_run["condition array, layer arrays, head<br/>(unchanged)"]
    n_fits -- yes --> n_split["_process_in_chunks()<br/>calls of at most mMaxBufferSize<br/>no allocation"]
    n_split -- each chunk --> n_call
Loading

Split rather than grow or throw. Growing would allocate on the audio thread
and clear every history: after one such call, wavenet_a1_standard.nam's
output was off by up to 0.57. SequentialModel throws instead. If you'd prefer
that here, it's a few lines and the test adapts. We chose splitting because a
throw on the audio thread usually takes the host down.

sdatkinson#303. A host that sends more frames than it reserved gets a working A2 model
(the A2 fast path grows) and a corrupted heap with the next A1 model. That
matches the report, but it depends on the reporter's host, so this doesn't
claim to close it. Details are on the issue.

Tests

tools/test/test_wavenet/test_oversized_blocks.cpp covers four WaveNets:
wavenet.nam, wavenet_a2_feature_test.nam, wavenet_condition_dsp.nam and a
2-in/2-out model built in the test. Each gets a 32-frame reservation and
100-frame calls, and the output must equal 32/32/32/4-frame calls exactly, with
no allocation. On main it fails at
Assertion failed: (num_frames <= mMaxBufferSize), function process, file model.cpp, line 827.
With -DNDEBUG under ASan it fails at the overflow:

AddressSanitizer on main
ERROR: AddressSanitizer: heap-buffer-overflow
WRITE of size 4 thread T0
    #0 nam::wavenet::WaveNet::_set_condition_array(double**, int) model.cpp:820
    #1 nam::wavenet::WaveNet::process(double**, double**, int) model.cpp:830
    ...
    #6 test_wavenet::test_oversized_blocks::test_oversized_call_matches_consecutive_calls() test_oversized_blocks.cpp:112
is located 0 bytes after 128-byte region
allocated by thread T0 here:
    ...
    #7 nam::wavenet::WaveNet::SetMaxBufferSize(int) model.cpp:691
    #8 nam::DSP::Reset(double, int) dsp.cpp:136

Verification

CI's steps pass on AppleClang 21 and on Linux clang 18.1.3 with both libstdc++
and libc++. run_tests passes on GCC 14.2 (Pi 5), with
-DNAM_ENABLE_A2_FAST=OFF, and under ASan+UBSan. Within the reservation,
output is bit-identical to main.

Details
  • AppleClang 21, macOS arm64: Debug, and Debug with NAM_USE_INLINE_GEMM,
    each running run_tests, benchmodel (wavenet.nam, lstm.nam) and
    render. run_tests with -DNAM_ENABLE_A2_FAST=OFF, and with
    -O1 -DNDEBUG -fsanitize=address,undefined.
  • Linux x86-64, clang 18.1.3 (Ubuntu 24.04 packages): CI's four
    configurations: libstdc++ and -DNAM_USE_LIBCXX_LINUX=ON, each plain and
    with inline GEMM.
  • GCC 14.2, Debian 13, Raspberry Pi 5: run_tests, plain and with inline
    GEMM.
  • Bit-identity with main at -O3: 36 byte-compared renders. They cover
    every example model through render (the slimmables at both sizes), and the
    A2 fast and generic paths at 64-, 7- and 61-frame blocks on a 128-frame
    reservation.
  • Oversized calls under ASan with -DNDEBUG: every example WaveNet, and
    the A1 model from [BUG] Loading certain A1 models after A2 models can cause audio crackling / dropouts sdatkinson/NeuralAmpModelerCore#303, are clean.
  • Cost: one integer comparison per call when the call fits. An earlier
    revision with the same check measured within noise on an idle Pi 5 (n=5).
    This commit was not timed separately.

Compatibility

Calls within the reservation take the same path, with bit-identical output.
Larger calls were undefined behaviour in release builds; they are now chunked.

Not covered

Every WaveNet buffer holds the maximum buffer size given to Reset(), and
process() only asserted that a call fits. With asserts compiled out, a
larger call wrote past the end of the condition array, every layer's
conv, mixin and activation buffers and the conv ring buffers.
AddressSanitizer reports a heap-buffer-overflow in _set_condition_array
for each generic WaveNet in example_models/. The A2 fast path grows its
buffers for the same call instead, so a host that under-reserves works
with A2 models and corrupts the heap with A1 models (see sdatkinson#303).

Process such a call as consecutive calls of at most the maximum buffer
size. That keeps the model's state and allocates nothing: the
per-channel chunk pointers are sized at construction. Growing the
buffers would also avoid the overflow, but it allocates on the audio
thread and SetMaxBufferSize() reinitialises every history, which changed
the output of wavenet_a1_standard.nam by up to 0.57 after the call. The
layer-array loop moves unchanged into _process_layer_arrays() so that
process() does not become more complex.

The new test renders four WaveNets (plain, A2-era layer features, a
condition DSP, and two in/two out) in 100-frame calls with a 32-frame
maximum buffer size, and requires the output to equal that of the host
making those calls in 32/32/32/4-frame pieces, with no allocation.
Without the fix it stops at the assert in WaveNet::process(), or, with
-DNDEBUG under AddressSanitizer, at the heap-buffer-overflow.
…k head

- _process_in_chunks advances by each chunk's length, so the offset never
  passes num_frames and cannot overflow int for calls within one buffer of
  INT_MAX frames. No test can make such a call; this is by inspection.
- The oversized-call test adds a two-channel WaveNet with a post-stack head,
  so the chunked path through the head is checked against consecutive calls
  and for allocations too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant