Fix OOM in DataformatAwareCatalogSnapshot deserialization of corrupt input

[ask-kamal-nayan]

Description

Problem

testDeserializationRejectsInvalidInput fails with OutOfMemoryError instead of the expected IOException when deserializing corrupt data.

Failing test: https://build.ci.opensearch.org/job/gradle-check/74559/testReport/junit/org.opensearch.index.engine.exec.coord/DataformatAwareCatalogSnapshotTests/testDeserializationRejectsInvalidInput/

When random/corrupt bytes are deserialized, readVInt() can produce an arbitrarily large segmentCount. This value is passed directly to
new ArrayList<>(segmentCount), which attempts to allocate a massive backing array and triggers an OOM. Since OutOfMemoryError is an Error (not Exception), it
escapes the catch block in deserializeFromString.

Fix

Validate segmentCount against in.available() before allocating the list. Each segment must occupy at least 1 byte when serialized, so a segment count exceeding
the remaining bytes in the stream is guaranteed to be invalid. This is consistent with how Lucene's SegmentInfos validates segment counts during deserialization.

Test

Ran testDeserializationRejectsInvalidInput with the original failing seed (37AB7097D08B22E2) and 100 iterations — all pass.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit f8d44e8)

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Incomplete Validation The validation checks segmentCount > in.available(), but in.available() returns the number of bytes available without blocking, which may not accurately reflect the total remaining bytes in all StreamInput implementations (e.g., network streams or compressed streams). This could cause valid inputs to be incorrectly rejected or still allow OOM in edge cases where available() returns a large value. Consider whether the underlying StreamInput implementation reliably supports available(), and if not, a tighter bound or a different approach (e.g., a hard cap on maximum allowed segment count) may be more robust. if (segmentCount < 0 || segmentCount > in.available()) { throw new IOException("Invalid segment count: " + segmentCount); } Negative Check Redundant The check segmentCount < 0 is redundant because readVInt() always returns a non-negative integer by definition (variable-length encoding of unsigned integers). While harmless, it may indicate a misunderstanding of the API. Consider removing it or adding a comment explaining why it is kept as a defensive check. if (segmentCount < 0 || segmentCount > in.available()) { throw new IOException("Invalid segment count: " + segmentCount);

[github-actions]

Persistent review updated to latest commit f0041cc

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to f8d44e8

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Replace unreliable available-bytes check with a fixed bound The in.available() method on a StreamInput may not reliably return the total number of remaining bytes, as it can return 0 or an estimate depending on the underlying stream implementation. This means the upper-bound check segmentCount > in.available() could incorrectly allow corrupt large values or incorrectly reject valid ones. Consider using a reasonable maximum constant (e.g., a configurable or hardcoded upper limit) instead of relying on in.available(). server/src/main/java/org/opensearch/index/engine/exec/coord/DataformatAwareCatalogSnapshot.java [88-90] -if (segmentCount < 0 || segmentCount > in.available()) { +if (segmentCount < 0 || segmentCount > MAX_SEGMENT_COUNT) { throw new IOException("Invalid segment count: " + segmentCount); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that in.available() may not reliably return the remaining bytes in a StreamInput, making the upper-bound check potentially unreliable. Using a fixed MAX_SEGMENT_COUNT constant would be more robust, though it requires defining an appropriate constant value.	Medium

---

Previous suggestions

Suggestions up to commit f8d44e8

Category	Suggestion	Impact
Possible issue	Use a meaningful max limit instead of available bytes The in.available() method returns the number of bytes available, not the number of segments. A single Segment object likely requires more than 1 byte to deserialize, so comparing segmentCount directly to in.available() is not a reliable upper bound and may allow OOM for large segment counts with many available bytes. Consider using a reasonable maximum constant (e.g., a configurable or hardcoded max segment limit) instead of in.available() as the upper bound. server/src/main/java/org/opensearch/index/engine/exec/coord/DataformatAwareCatalogSnapshot.java [88-90] -if (segmentCount < 0 || segmentCount > in.available()) { +if (segmentCount < 0 || segmentCount > MAX_SEGMENT_COUNT) { throw new IOException("Invalid segment count: " + segmentCount); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that in.available() returns bytes, not segment count, making the comparison unreliable as an upper bound. Using a MAX_SEGMENT_COUNT constant would be a more meaningful and safer guard against OOM attacks or corrupted data.	Medium

Suggestions up to commit f8d44e8

Category	Suggestion	Impact
Possible issue	Use a meaningful upper bound for segment count validation The in.available() method returns the number of bytes available, not the number of segments. A single Segment object likely requires more than 1 byte to deserialize, so comparing segmentCount directly against in.available() may allow unreasonably large allocations when segments are small or the stream reports a large byte count. Consider using a more conservative maximum (e.g., a fixed constant like MAX_SEGMENT_COUNT) or dividing in.available() by a minimum expected segment size in bytes. server/src/main/java/org/opensearch/index/engine/exec/coord/DataformatAwareCatalogSnapshot.java [88-90] -if (segmentCount < 0 || segmentCount > in.available()) { +if (segmentCount < 0 || segmentCount > MAX_SEGMENT_COUNT) { throw new IOException("Invalid segment count: " + segmentCount); } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that in.available() returns bytes, not segment count, making the comparison potentially misleading or ineffective as a guard. However, the improved code references MAX_SEGMENT_COUNT which is not defined in the PR, requiring additional changes not shown.	Low

Suggestions up to commit f8d44e8

Category	Suggestion	Impact
Possible issue	Use a meaningful max bound for validation The in.available() method returns the number of bytes available, not the number of segments. A single Segment object likely requires more than 1 byte to deserialize, so comparing segmentCount directly to in.available() is not a reliable upper bound and may allow OOM for large segment counts with many bytes available. Consider using a reasonable maximum constant (e.g., a configurable or hardcoded max segment count) instead of in.available(). server/src/main/java/org/opensearch/index/engine/exec/coord/DataformatAwareCatalogSnapshot.java [88-90] -if (segmentCount < 0 || segmentCount > in.available()) { +if (segmentCount < 0 || segmentCount > MAX_SEGMENT_COUNT) { throw new IOException("Invalid segment count: " + segmentCount); } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that in.available() returns bytes, not segment count, making the comparison unreliable as an upper bound. Using a defined MAX_SEGMENT_COUNT constant would be more semantically correct, though the suggested improved_code introduces an undefined MAX_SEGMENT_COUNT variable that would need to be declared elsewhere.	Low

Suggestions up to commit f0041cc

Category	Suggestion	Impact
Possible issue	Use a fixed maximum instead of available bytes The in.available() method returns the number of bytes available, not the number of segments. A single Segment object likely requires more than 1 byte to deserialize, so comparing segmentCount directly to in.available() is not a reliable upper bound and may allow OOM for large segment counts with many available bytes. Consider using a reasonable maximum segment count constant (e.g., a configurable or hardcoded upper limit) instead of relying on in.available(). server/src/main/java/org/opensearch/index/engine/exec/coord/DataformatAwareCatalogSnapshot.java [88-90] -if (segmentCount < 0 || segmentCount > in.available()) { +if (segmentCount < 0 || segmentCount > MAX_SEGMENT_COUNT) { throw new IOException("Invalid segment count: " + segmentCount); } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that in.available() returns bytes, not segment count, making the comparison unreliable as a guard against OOM. However, using a hardcoded MAX_SEGMENT_COUNT constant introduces its own issues without knowing the appropriate limit, and the current check still provides some protection against obviously malformed data.	Low

[github-actions]

❕ Gradle check result for f0041cc: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.66%. Comparing base (8bd7c98) to head (f8d44e8).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
...ine/exec/coord/DataformatAwareCatalogSnapshot.java	0.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21236      +/-   ##
============================================
+ Coverage     73.28%   73.66%   +0.37%     
- Complexity    73483    74100     +617     
============================================
  Files          5912     5936      +24     
  Lines        334943   335730     +787     
  Branches      48257    48395     +138     
============================================
+ Hits         245455   247299    +1844     
+ Misses        69899    69003     -896     
+ Partials      19589    19428     -161     

☔ 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.

[github-actions]

Persistent review updated to latest commit f8d44e8

[github-actions]

✅ Gradle check result for f8d44e8: SUCCESS

[github-actions]

Persistent review updated to latest commit f8d44e8

[github-actions]

❌ Gradle check result for f8d44e8: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit f8d44e8

[github-actions]

❌ Gradle check result for f8d44e8: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit f8d44e8

[github-actions]

✅ Gradle check result for f8d44e8: SUCCESS