Skip to content

Fix wavelet coefficient buffer under allocation in five bindings - #850

Open
adityasingh2400 wants to merge 4 commits into
brainflow-dev:masterfrom
adityasingh2400:fix-wavelet-buffer-underallocation
Open

Fix wavelet coefficient buffer under allocation in five bindings#850
adityasingh2400 wants to merge 4 commits into
brainflow-dev:masterfrom
adityasingh2400:fix-wavelet-buffer-underallocation

Conversation

@adityasingh2400

Copy link
Copy Markdown

perform_wavelet_transform writes wt->outlength doubles into the output buffer the caller supplies. The safe upper bound, taken from the wavelib sources, is data_len + 2 * decomposition_level * (40 + 1). That is exactly what cpp_package/src/data_filter.cpp, DataFilter.java, data_filter.rs, the Swift DataFilter.swift and the internal caller at src/data_handler/data_handler.cpp:1557 all allocate.

Five bindings dropped the decomposition_level factor and allocate only data_len + 2 * (40 + 1): python data_filter.py:881, C# data_filter.cs:354 and :1118, node data_filter.ts:494, matlab DataFilter.m:189 and julia data_filter.jl:300. The R package calls through the python one, so it inherits the same buffer. Short wavelets stay under the smaller bound and hide the problem, but the longer filters do not, and the transform writes past the end of the buffer.

I measured the real write count by handing the C function a hugely oversized sentinel filled buffer and finding the highest index it touched, across 53 combinations of wavelet, length and level. The decomposition_level bound was never exceeded, 0 of 53. The smaller bound was exceeded in 6 of 53. The worst case is db15 with a 1024 sample input at level 5, which writes 1167 doubles into a 1106 slot buffer, 61 doubles or 488 bytes past the end. db15 already overflows at level 3, so this is a function of the wavelet filter length as well as the level, not level alone.

To confirm it is a genuine out of bounds write rather than an accounting mismatch, I mapped the buffer so its final byte is the final byte of a mapped page with a PROT_NONE guard page immediately after, then called perform_wavelet_transform through the built libDataHandler. At the old size of 1106 slots the process takes a hard fault, SIGBUS, exit status 138. At 1434 slots, which is what the corrected python formula gives for that case, it completes normally and reports 1167 coefficients.

There is a second, quieter symptom. Each wrapper slices the buffer to sum(lengths), and when that total exceeds the buffer the slice silently clamps, so the caller gets a short coefficient vector. Through the public python API on 8 cases, 6 came back truncated before the change, for example db15 at level 5 returning 1106 of 1167 coefficients. The inverse transform then read past the end of that short array. After the change all 8 return the full vector and round trip to a maximum absolute error of 1.8e-15.

The fix restores the decomposition_level factor in all five bindings. The C# overload taking a 2d array also sized itself from data.Length, the total element count of the matrix, while passing data.GetLength (1) as the actual data_len, so I switched it to GetLength (1) to match what the C side is told and is now correct for a single row matrix too.

Verified on macOS arm64 against a fresh cmake .. && make -j8 build. The nine synthetic board examples CI runs all pass: denoising.py, signal_filtering.py, transforms.py, downsampling.py, band_power.py, band_power_all.py, windowing.py, ica.py and csp.py. transforms.py is the one that exercises the wavelet path. The C# package builds with dotnet build -c Release, 0 warnings and 0 errors, and the node package typechecks with tsc --noEmit, exit 0. I have no julia or matlab toolchain locally, so those two are one line changes reviewed against the working java and rust versions rather than executed. There is no assertion based test surface for these functions, the existing coverage is demo scripts that plot rather than assert, so I could not extend one, but I am glad to add a db15 round trip case if you would like it covered in CI.

perform_wavelet_transform writes wt->outlength doubles into the caller
supplied buffer. The safe upper bound taken from the wavelib sources is
data_len + 2 * decomposition_level * (40 + 1), which is what the C++
binding, the java binding, the rust binding, the swift binding and the
internal caller in data_handler.cpp all use.

The python, C#, node, matlab and julia bindings dropped the
decomposition_level factor and allocated data_len + 2 * (40 + 1). With
longer wavelet filters the transform then writes past the end of the
buffer. Measured with db15, a 1024 sample input and level 5 writes 1167
doubles into a 1106 slot buffer, 488 bytes past the end. A guard page
placed immediately after the buffer turns that into a hard fault.

Because each wrapper then slices the buffer to sum(lengths), which is
larger than the buffer itself, the coefficient vector handed back to the
caller was also silently truncated, and the inverse transform read past
the end of that short array on the way back.

Restore the decomposition_level factor in all five. The C# overload that
takes a 2d array also sized itself from data.Length, the total element
count, rather than the row length it actually passes as data_len, so use
GetLength (1) there to match.

Copy link
Copy Markdown
Member

The allocation fixes agree with the native upper bound and with the bindings that already include decomposition_level. The guard-page and round-trip reproductions are convincing.

Because this is a demonstrated out-of-bounds write across five public bindings, I consider automated regression coverage necessary before merge. Please add at least a db15/level-5 case that asserts the returned coefficient count equals sum(lengths) and that inverse transform round-trips. Ideally, run it for Python plus one statically typed binding. The normal MATLAB and Julia CI jobs should also complete successfully because those two changes were not executed locally.

Longer term, centralizing or exposing the required output-size calculation would help prevent the bindings from diverging again.

Asserts the returned coefficient count equals sum(lengths) for db15 at
every level up to 5, and that the inverse transform round-trips. The old
fixed headroom allocated 1106 doubles where the native side writes 1167.
@adityasingh2400

Copy link
Copy Markdown
Author

Added python_package/examples/tests/wavelet_buffer_size.py and wired it into run_unix.yml.

The db15 level-5 case asserts len(coeffs) == sum(lengths) and that the inverse transform round-trips to the original signal. It also loops levels 1 through 5, since the point is that the headroom has to scale with the level rather than being fixed. On the unfixed binding it fails with (1106, 1167), so 61 doubles or 488 bytes past the end of the array.

Two things I did not do. I covered python only, not a statically typed binding. The C# and java suites build and run separately from the python examples, so that is a larger change than this PR, and I would rather add it where you think it belongs than guess. I also did not execute the matlab or julia changes locally, same as before, so those still rest on their CI jobs.

On centralizing the size calculation, I agree that is the real fix. It would need a native helper returning the required output length so every binding asks instead of each recomputing it. Happy to do that as a follow-up if you want it, though it is an API addition rather than a bug fix so it seemed wrong to fold into this one.

@adityasingh2400

Copy link
Copy Markdown
Author

Follow-up: I had only wired the new test into run_unix.yml. The existing python DSP tests also run in run_windows.yml, so I added it there too, matching the shell: cmd style used by the downsampling.py step. run_alpine.yml does not run any python example tests, so I left it alone.

Both workflow files still parse.

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.

2 participants