fix(FIR): size q15 state buffer for ARM_MATH_LOOPUNROLL, not just ARM_MATH_DSP#317
Merged
christophe0606 merged 2 commits intoJul 17, 2026
Conversation
…_MATH_DSP arm_fir_init_q15() only allocates the larger (numTaps+blockSize) state buffer when ARM_MATH_DSP is defined, otherwise it allocates the smaller (numTaps+blockSize-1). arm_fir_q15()'s non-MVE processing function has a separate ARM_MATH_LOOPUNROLL-gated code path (4-sample-at-a-time, using read_q15x2_ia()/__SMLALD()/__PKHBT(), which all have portable non-hardware-DSP fallback implementations and so do not themselves require ARM_MATH_DSP) that reads up to index numTaps+blockSize-1 from the state buffer - one element past what's allocated when ARM_MATH_LOOPUNROLL is defined without ARM_MATH_DSP, an out-of-bounds read. Verified with a standalone host reproduction of the exact read_q15x2_ia/__SMLALD/__PKHBT access pattern, placing the state buffer immediately before a PAGE_NOACCESS guard page: with the current (numTaps+blockSize-1)-sized buffer the reproduction crashes with an access violation; sized to (numTaps+blockSize) it completes cleanly. Fix: allocate the larger buffer whenever ARM_MATH_DSP *or* ARM_MATH_LOOPUNROLL is defined, matching what arm_fir_q15()'s loop-unrolled path actually needs regardless of whether hardware DSP instructions or their software fallback are in use. Fixes ARM-software#305 Signed-off-by: 94xhn <87560781+94xhn@users.noreply.github.com>
christophe0606
requested changes
Jul 16, 2026
christophe0606
left a comment
Contributor
There was a problem hiding this comment.
I have added a few comments to address.
arm_fir_q15 only exercises the extra state slot in its ARM_MATH_LOOPUNROLL path. Restrict the initialization rule and documentation to that tested configuration so ARM_MATH_DSP alone keeps the normal state size. Constraint: ARM_MATH_DSP is not a tested arm_fir_q15 execution path Rejected: Keep the combined macro guard | overstates the implementation and test coverage Confidence: high Scope-risk: narrow Directive: Tie q15 state sizing to the paths actually compiled in arm_fir_q15 Tested: MinGW and WSL full CMSISDSP builds 518/518; strict host FIR harness; WSL ASan Not-tested: ARM_MATH_DSP-only path, per maintainer direction
christophe0606
approved these changes
Jul 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #305.
Problem
arm_fir_init_q15()only allocates the larger(numTaps+blockSize)state buffer whenARM_MATH_DSPis defined; otherwise it allocates the smaller(numTaps+blockSize-1), per the function's own documentation.arm_fir_q15()'s non-MVE processing function has a separateARM_MATH_LOOPUNROLL-gated code path (processes 4 samples at a time usingread_q15x2_ia()/__SMLALD()/__PKHBT()) that reads up to state-buffer indexnumTaps+blockSize-1- one element past what's allocated wheneverARM_MATH_LOOPUNROLLis defined withoutARM_MATH_DSP. This is a real, reachable configuration:read_q15x2_ia()/__SMLALD()/__PKHBT()all have portable non-hardware-DSP fallback implementations (inInclude/arm_math_memory.h/Include/dsp/none.h), soARM_MATH_LOOPUNROLLdoesn't itself requireARM_MATH_DSPto compile or run - it just happens that most real projects define both together.Verification
I extracted the exact access pattern of
arm_fir_q15()'s loop-unrolled scalar path (read_q15x2_ia/__SMLALD/__SMLALDX/__PKHBT, matching the portable fallback definitions) into a standalone host reproduction, and placed the state buffer immediately before aPAGE_NOACCESSguard page (VirtualAlloc) so any read past the documented allocation size faults immediately instead of silently touching adjacent heap memory:(numTaps+blockSize-1)-sized buffer (the un-fixed!ARM_MATH_DSPsizing): the reproduction crashes with an access violation (STATUS_ACCESS_VIOLATION,0xC0000005).(numTaps+blockSize)-sized buffer (theARM_MATH_DSP-defined sizing): the reproduction completes cleanly.This confirms the out-of-bounds read is real and reachable, not just a theoretical pointer-arithmetic concern.
Fix
Changed
arm_fir_init_q15()'s sizing condition from#if defined(ARM_MATH_DSP)to#if defined(ARM_MATH_DSP) || defined(ARM_MATH_LOOPUNROLL), so the larger buffer is allocated whenever the loop-unrolled path is actually in use, regardless of whether hardware DSP instructions or their software fallback are what's actually executing it. Updated the docstring to match.Scope note
I checked the q31/q7/f32 FIR init functions for the same pattern -
arm_fir_init_q31.candarm_fir_init_f32.chave additional, different conditional sizing logic (not a simpleARM_MATH_DSPgate), andarm_fir_init_q7.cdoesn't have thisARM_MATH_DSP-gated branch at all - so I did not attempt to extend this same fix to them without separately, individually verifying each one, which is outside the scope of this specific issue.Disclosure
Generative AI (Claude) was used to help investigate this issue, implement, and verify this fix. All changes were reviewed by me before submission.