Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a _Private_IonConstants.ARRAY_MAXIMUM_SIZE #708

Merged
merged 5 commits into from
Jan 30, 2024

Conversation

jobarr-amzn
Copy link
Contributor

Description of changes:

Arrays in Java must be indexed by integers (JLS 10.4), but the true maximum array size may be less than Integer.MAX_VALUE. True maximum array size is a JVM implementation detail, and varies by type and by JVM. We have this pinned at Integer.MAX_VALUE - 8 because that's a fairly common value in the JDK itself, in classes such as ConcurrentHashMap, InputStream, Hashtable, ByteArrayChannel, etc.

In testing against a variety of JVMs and types the smallest maximum size I've seen is Integer.MAX_VALUE - 2, and the smallest maximum size I've seen reported is Integer.MAX_VALUE - 6

This change adds a constant _Private_IonConstants.ARRAY_MAXIMUM_SIZE and uses the new constant where appropriate.

Need for this safeguard was exposed by another bug which caused excessive buffering in this chunk of IonCursorBinary:

int maximumFreeSpace = refillableState.maximumBufferSize;
int startOffset = (int) offset;
if (minimumNumberOfBytesRequired > maximumFreeSpace) {
refillableState.isSkippingCurrentValue = true;
return false;
}
long shortfall = minimumNumberOfBytesRequired - refillableState.capacity;
if (shortfall > 0) {
int newSize = (int) Math.min(Math.max(refillableState.capacity * 2, nextPowerOfTwo((int) (refillableState.capacity + shortfall))), maximumFreeSpace);
byte[] newBuffer = new byte[newSize];

While that is still under investigation, this at least should prevent Requested array size exceeds VM limit.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

* Use the new constant where appropriate

Arrays in Java must be indexed by integers (JLS 10.4), but the true maximum
array size may be less than `Integer.MAX_VALUE`. True maximum array size is a
JVM implementation detail, and varies by type and by JVM.
We have this pinned at `Integer.MAX_VALUE - 8` because that's a fairly common
value in the JDK itself, in classes such as `ConcurrentHashMap`, `InputStream`,
`Hashtable`, `ByteArrayChannel`, etc.

In testing against a variety of JVMs and types the smallest maximum size I've
seen is `Integer.MAX_VALUE - 2`, and the smallest maximum size I've seen
reported is `Integer.MAX_VALUE - 6`
@jobarr-amzn jobarr-amzn requested a review from tgregg January 26, 2024 19:33
Copy link

codecov bot commented Jan 26, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (3c1b6b1) 67.23% compared to head (a4bf716) 67.25%.

Files Patch % Lines
src/main/java/com/amazon/ion/apps/BaseApp.java 0.00% 1 Missing ⚠️
...c/main/java/com/amazon/ion/impl/BlockedBuffer.java 0.00% 1 Missing ⚠️
...java/com/amazon/ion/impl/IonReaderTextSystemX.java 0.00% 0 Missing and 1 partial ⚠️
.../main/java/com/amazon/ion/impl/_Private_Utils.java 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #708      +/-   ##
============================================
+ Coverage     67.23%   67.25%   +0.02%     
- Complexity     5484     5488       +4     
============================================
  Files           159      159              
  Lines         23025    23030       +5     
  Branches       4126     4128       +2     
============================================
+ Hits          15481    15489       +8     
+ Misses         6262     6260       -2     
+ Partials       1282     1281       -1     

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

Copy link
Contributor

@tgregg tgregg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@jobarr-amzn jobarr-amzn merged commit 88035b9 into master Jan 30, 2024
21 of 35 checks passed
@jobarr-amzn jobarr-amzn deleted the array-maximum-size branch January 30, 2024 17:50
tgregg pushed a commit that referenced this pull request Feb 8, 2024
* Add a _Private_IonConstants.ARRAY_MAXIMUM_SIZE

* Use the new constant where appropriate

Arrays in Java must be indexed by integers (JLS 10.4), but the true maximum
array size may be less than `Integer.MAX_VALUE`. True maximum array size is a
JVM implementation detail, and varies by type and by JVM.
We have this pinned at `Integer.MAX_VALUE - 8` because that's a fairly common
value in the JDK itself, in classes such as `ConcurrentHashMap`, `InputStream`,
`Hashtable`, `ByteArrayChannel`, etc.

In testing against a variety of JVMs and types the smallest maximum size I've
seen is `Integer.MAX_VALUE - 2`, and the smallest maximum size I've seen
reported is `Integer.MAX_VALUE - 6`

* Add test coverage for maximum buffer size

* Add more test coverage for maximum buffer size

* Ensure that a modeled exception is thrown
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.

2 participants