Fix wavelet coefficient buffer under allocation in five bindings - #850
Fix wavelet coefficient buffer under allocation in five bindings#850adityasingh2400 wants to merge 4 commits into
Conversation
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.
|
The allocation fixes agree with the native upper bound and with the bindings that already include 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 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.
|
Added The db15 level-5 case asserts 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. |
|
Follow-up: I had only wired the new test into Both workflow files still parse. |
perform_wavelet_transformwriteswt->outlengthdoubles into the output buffer the caller supplies. The safe upper bound, taken from the wavelib sources, isdata_len + 2 * decomposition_level * (40 + 1). That is exactly whatcpp_package/src/data_filter.cpp,DataFilter.java,data_filter.rs, the SwiftDataFilter.swiftand the internal caller atsrc/data_handler/data_handler.cpp:1557all allocate.Five bindings dropped the
decomposition_levelfactor and allocate onlydata_len + 2 * (40 + 1): pythondata_filter.py:881, C#data_filter.cs:354and:1118, nodedata_filter.ts:494, matlabDataFilter.m:189and juliadata_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_levelbound 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_NONEguard page immediately after, then calledperform_wavelet_transformthrough the builtlibDataHandler. 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_levelfactor in all five bindings. The C# overload taking a 2d array also sized itself fromdata.Length, the total element count of the matrix, while passingdata.GetLength (1)as the actualdata_len, so I switched it toGetLength (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 -j8build. 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.pyandcsp.py.transforms.pyis the one that exercises the wavelet path. The C# package builds withdotnet build -c Release, 0 warnings and 0 errors, and the node package typechecks withtsc --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.