Fixes a bug that caused the binary reader not to fail cleanly when parsing incomplete containers in certain cases.

[tgregg]

Issue #, if available:
FasterXML/jackson-dataformats-binary#473

Description of changes:
Before this fix, the added test expectIncompleteContainerToFailCleanlyAfterFieldSid would fail with ArrayIndexOutOfBoundsException. The other two tests succeeded before and after the fix; I just wanted to be sure these cases were covered.

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3c1b6b1) 67.23% compared to head (8b5c43e) 67.25%.
Report is 5 commits behind head on master.

❗ Current head 8b5c43e differs from pull request most recent head 08752c2. Consider uploading reports for the commit 08752c2 to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #710      +/-   ##
============================================
+ Coverage     67.23%   67.25%   +0.01%     
- Complexity     5484     5487       +3     
============================================
  Files           159      159              
  Lines         23025    23027       +2     
  Branches       4126     4127       +1     
============================================
+ Hits          15481    15486       +5     
+ Misses         6262     6259       -3     
  Partials       1282     1282              

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

[zslayton]

Would

...exceeds the number of bytes remaining in the container.

be more accurate?

[jobarr-amzn]

It took me a while to understand why you made this suggestion. It's because we're in an unchecked method, which means that we already believe we have enough data to finish the container. Therefore if peekIndex >= limit we're definitely past the container bounds because implicitly parent.endIndex < limit?

Another thing strikes me about this though- we check the same condition peekIndex >= limit on line 1551, and from my read the only thing that can change that condition in between there and here is this clause of uncheckedNextContainedToken():

        } else if (parent.typeId.type == IonType.STRUCT) {
            if (minorVersion == 0) {
                byte b = buffer[(int) peekIndex++];
                if (b < 0) {
                    fieldSid = (b & LOWER_SEVEN_BITS_BITMASK);
                } else {
                    fieldSid = (int) uncheckedReadVarUInt_1_0(b);
                }
            } else {
                uncheckedReadFieldName_1_1();
            }
        }

We must be in minorVersion == 0, and I note that uncheckedReadVarUInt_1_0(b) already contains a malformed data check. Does that mean that this error condition is discovered only when we've just read a 1-byte VarUInt field name, in the if (b < 0) { case above? In that case, should this error check go there, in uncheckedNextContainedToken()?

[zslayton]

This code appears in the else branch which handles values below the top level:

if (parent == null) { // Depth 0
  // ...
} else {
  // the new code
}

I wasn't certain whether limit was the end of the stream's data or the end of the container's data in the buffer. Based on the location of the code, the latter seemed plausible.

Another thing strikes me about this though- we check the same condition peekIndex >= limit on line 1551, and from my read the only thing that can change that condition in between there and here is this clause of uncheckedNextContainedToken():

I wondered if reset() might affect peekIndex or limit, but didn't investigate further.

[tgregg]

Would

...exceeds the number of bytes remaining in the container.

be more accurate?

Yes, this is more accurate.

We must be in minorVersion == 0, and I note that uncheckedReadVarUInt_1_0(b) already contains a malformed data check. Does that mean that this error condition is discovered only when we've just read a 1-byte VarUInt field name, in the if (b < 0) { case above? In that case, should this error check go there, in uncheckedNextContainedToken()?

I preferred to put this check in the proposed location, rather than in uncheckedNextContainedToken(), because the check protects the line that immediately follows (the access to the buffer at peekIndex). Note: uncheckedReadVarUInt_1_0 performs the check before each byte it consumes, but the added check protects access to the first byte after the field name.