fix: avoid out-of-bounds read of the SMAPE weight array#753
Open
dnplkndll wants to merge 1 commit into
Open
Conversation
weight[] has MAXBUCKETS (500) entries but is indexed weight[count - i] where count is the history-bucket count (~520 weekly / ~3650 daily over the default 10-yr horizon), reading past the array and corrupting SMAPE method selection. Add a clamping smapeWeight() accessor; weights decay exponentially so weight[>=500] ~= 0, making the clamp behaviour-preserving. Also fix a second initializer that filled only 300 of the 500 entries. Adds test/forecast_11 (weekly history >500 buckets; the overflow is flagged by an ASan build).
jdetaeye
added a commit
that referenced
this pull request
Jun 16, 2026
jdetaeye
reviewed
Jun 16, 2026
| // entries above the old hardcoded bound stay stale when this setter runs. | ||
| weight[0] = 1.0; | ||
| for (int i = 0; i < 299; ++i) | ||
| for (int i = 0; i < MAXBUCKETS - 1; ++i) |
Member
There was a problem hiding this comment.
This correction is absolutely correct. An oversight when the hardcoded limit was replaced with this constant.
jdetaeye
reviewed
Jun 16, 2026
| if (idx < 0) idx = 0; | ||
| if (idx >= MAXBUCKETS) idx = MAXBUCKETS - 1; | ||
| return weight[idx]; | ||
| } |
Member
There was a problem hiding this comment.
Not sure whether this change is worth picking up.
-
It's extremely rare we need more than 500 buckets of demand history. You already indicate that the weight of these buckets is close to 0. In functional terms: "what happened 500 time buckets ago has very little impact on the forecast we compute for the next forecast buckets"
-
While the forecast calculations are fast, this will have a (minimal) performance impact.
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
2 participants
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.
weight[](MAXBUCKETS = 500 entries) is indexedweight[count - i]wherecountis the history-bucket count (~520 weekly / ~3650 daily over the default 10-year horizon), reading past the array and corrupting SMAPE method selection.Adds a clamping
smapeWeight()accessor — behaviour-preserving since weights decay exponentially (weight[>=500] ≈ 0) — fixes a second initializer that filled only 300 of the 500 entries, and addstest/forecast_11(the overflow is flagged by an AddressSanitizer build).