Fix out-of-bounds write in FastFourierTransform constructor for order=0#2597
Merged
Conversation
The constructor allocated cs_ and sn_ with size 'order' and then unconditionally wrote to cs_[order-1] and sn_[order-1]. When 'order' is 0 (e.g. when callers chain FastFourierTransform::min_order(1), which returns 0), both vectors are empty and the indexing wraps to size_t(-1), producing an out-of-bounds write that segfaults under debug-iterator-checked standard libraries. A transform of size 1 == 1<<0 reduces to a copy and needs no twiddle factors, so skip the setup when order is 0. The transform loop body 'for (s = 1; s <= order; ++s)' already handles the order==0 case correctly.
|
Thanks for opening this pull request! It might take a while before we look at it, so don't worry if there seems to be no feedback. We'll get to it. |
Owner
|
Thanks! Can you add a small test case? |
Contributor
Author
i added a small test |
Owner
|
Great, thanks. |
|
Congratulations on your first merged pull request! |
FastFourierTransform constructor for order=0
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.
Summary
The
FastFourierTransform(std::size_t order)constructor inql/math/fastfouriertransform.hppallocatescs_andsn_with sizeorderand then unconditionally writes tocs_[order - 1]andsn_[order - 1]. Whenorder == 0both vectors are empty and theindexing wraps to
size_t(-1), producing an out-of-bounds heap write.How to reproduce
FastFourierTransform::min_order(1)returns0, so a caller thatchains it straight into the constructor for a single-element transform
hits the bug:
With a debug-checked standard library (
-D_LIBCPP_DEBUG=1/-D_GLIBCXX_DEBUG) this segfaults (exit 139). Under release builds itsilently scribbles past the end of the allocator's bookkeeping for the
empty
std::vectorstorage.Affected lines (pre-patch):
The fix
A transform of length
1 == (1 << 0)reduces to a copy and needs notwiddle factors. The existing transform loop body
for (s = 1; s <= order; ++s) { ... }already does the right thingfor
order == 0(it iterates zero times). The only piece that misbehavesis the precomputation in the constructor, so early-return there when
order == 0. The change is local to the callee and does not affect anyother code path.
Test / build evidence
Built and ran a small program that exercises both the previously-crashing
order == 0path and a regression case fororder == 3:Compiled with
-D_LIBCPP_DEBUG=1to catch the OOB write; the unpatchedheader exits 139, the patched header exits 0 and produces the expected
DFT bin values (first bin == sum of inputs).