Skip to content

fix int overflow in subarray bounds check in verifyValues and setData#323

Merged
aherbert merged 3 commits into
apache:masterfrom
dxbjavid:verifyvalues-overflow
Jun 1, 2026
Merged

fix int overflow in subarray bounds check in verifyValues and setData#323
aherbert merged 3 commits into
apache:masterfrom
dxbjavid:verifyvalues-overflow

Conversation

@dxbjavid
Copy link
Copy Markdown
Contributor

@dxbjavid dxbjavid commented Jun 1, 2026

Noticed that verifyValues checks begin + length > values.length with int arithmetic after only validating that begin and length are non-negative. With large begin/length the sum overflows to a negative int, the check passes, and the helper reports an out-of-range subarray as valid: MathArrays.verifyValues(new double[10], 1, Integer.MAX_VALUE, false) returns true instead of throwing. AbstractUnivariateStatistic.setData has the same check and then trips a raw ArrayIndexOutOfBoundsException in the arraycopy. Widening the sum to long closes the bypass.

@aherbert
Copy link
Copy Markdown
Contributor

aherbert commented Jun 1, 2026

Thanks for the bug report. If the exception is triggered, the exception still contains the integer value of start + length (which may overflow). Can you also change this to a long so the exception message is correct.

Can you add a unit test that fails without the changes. Thank you.

@dxbjavid
Copy link
Copy Markdown
Contributor Author

dxbjavid commented Jun 1, 2026

Good catch on the exception value. Widened begin + length to long in the message args for both verifyValues and setData, so it now reports 2147483648 instead of the wrapped int.

Added testVerifyValuesOverflow, which calls verifyValues(testArray, 1, Integer.MAX_VALUE). Before the fix the int sum wraps negative, the bounds check is skipped, and it returns true instead of throwing, so the test fails. It also asserts getArgument() is the correct long value.

@aherbert
Copy link
Copy Markdown
Contributor

aherbert commented Jun 1, 2026

Can you also add a test for the AbstractUnivariateStatistic. Thanks.

@dxbjavid
Copy link
Copy Markdown
Contributor Author

dxbjavid commented Jun 1, 2026

Added AbstractUnivariateStatisticTest.testSetDataOverflow. It calls setData(new double[10], 1, Integer.MAX_VALUE) on a minimal concrete subclass; before the fix the int sum wraps negative so the bounds check is skipped and arraycopy throws a raw AIOOBE, and it also asserts getArgument() is the correct long value.

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 87.15%. Comparing base (f554608) to head (abaf6ec).
⚠️ Report is 319 commits behind head on master.

Files with missing lines Patch % Lines
.../stat/descriptive/AbstractUnivariateStatistic.java 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #323      +/-   ##
============================================
+ Coverage     86.54%   87.15%   +0.60%     
+ Complexity     9787       89    -9698     
============================================
  Files           532      499      -33     
  Lines         35516    33459    -2057     
  Branches       6194     5833     -361     
============================================
- Hits          30738    29161    -1577     
+ Misses         3518     3174     -344     
+ Partials       1260     1124     -136     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@aherbert aherbert merged commit 2e251dd into apache:master Jun 1, 2026
8 of 12 checks passed
@aherbert
Copy link
Copy Markdown
Contributor

aherbert commented Jun 1, 2026

Thank you for your contribution.

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.

3 participants