From 5ee9f7ea9dede1d52ce252126ffa37b097348606 Mon Sep 17 00:00:00 2001 From: Tyler Gregg Date: Mon, 22 Apr 2024 16:57:44 -0700 Subject: [PATCH] Ensures that using a cursor after close has no effect. --- .../com/amazon/ion/impl/IonCursorBinary.java | 23 ++++++++++++++----- .../amazon/ion/impl/IonCursorBinaryTest.java | 20 +++++++++++++++- ...onReaderContinuableTopLevelBinaryTest.java | 18 +++++++++++++++ 3 files changed, 54 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/amazon/ion/impl/IonCursorBinary.java b/src/main/java/com/amazon/ion/impl/IonCursorBinary.java index 52401ae300..4887aca16f 100644 --- a/src/main/java/com/amazon/ion/impl/IonCursorBinary.java +++ b/src/main/java/com/amazon/ion/impl/IonCursorBinary.java @@ -92,7 +92,7 @@ private static class RefillableState { * READY state). This enables the reader to complete the previous IonCursor API invocation if it returned a * NEEDS_DATA event. */ - State state = State.READY; + State state; /** * The number of bytes that still need to be consumed from the input during a fill or seek operation. @@ -124,14 +124,20 @@ private static class RefillableState { */ int individualBytesSkippedWithoutBuffering = 0; - RefillableState(InputStream inputStream, int capacity, int maximumBufferSize) { + RefillableState(InputStream inputStream, int capacity, int maximumBufferSize, State initialState) { this.inputStream = inputStream; this.capacity = capacity; this.maximumBufferSize = maximumBufferSize; + this.state = initialState; } } + /** + * Dummy state that indicates the cursor has been terminated and that additional API calls will have no effect. + */ + private static final RefillableState TERMINATED_STATE = new RefillableState(null, -1, -1, State.TERMINATED); + /** * Stack to hold container info. Stepping into a container results in a push; stepping out results in a pop. */ @@ -249,7 +255,7 @@ private static class RefillableState { /** * Holds information necessary for reading from refillable input. Null if the cursor is byte-backed. */ - private final RefillableState refillableState; + private RefillableState refillableState; /** * Describes the byte at the `checkpoint` index. @@ -455,7 +461,8 @@ private static IonBufferConfiguration getFixedSizeConfigurationFor( refillableState = new RefillableState( inputStream, configuration.getInitialBufferSize(), - configuration.getMaximumBufferSize() + configuration.getMaximumBufferSize(), + State.READY ); registerOversizedValueHandler(configuration.getOversizedValueHandler()); } @@ -1898,12 +1905,15 @@ private boolean slowIsAwaitingMoreData() { * exceeds the maximum buffer size. */ void terminate() { - refillableState.state = State.TERMINATED; + refillableState = TERMINATED_STATE; + // Use a unified code path for all cursors after termination. This path forces a termination check before + // accessing the input stream or buffer. + isSlowMode = true; } @Override public void close() { - if (refillableState != null) { + if (refillableState != null && refillableState.inputStream != null) { try { refillableState.inputStream.close(); } catch (IOException e) { @@ -1913,6 +1923,7 @@ public void close() { buffer = null; containerStack = null; byteBuffer = null; + terminate(); } /* ---- End: version-agnostic parsing, utility, and public API methods ---- */ diff --git a/src/test/java/com/amazon/ion/impl/IonCursorBinaryTest.java b/src/test/java/com/amazon/ion/impl/IonCursorBinaryTest.java index 581289a78e..436bb0030a 100644 --- a/src/test/java/com/amazon/ion/impl/IonCursorBinaryTest.java +++ b/src/test/java/com/amazon/ion/impl/IonCursorBinaryTest.java @@ -1,6 +1,5 @@ // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 - package com.amazon.ion.impl; import com.amazon.ion.IonBufferConfiguration; @@ -31,6 +30,7 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; public class IonCursorBinaryTest { @@ -427,4 +427,22 @@ public void expectValueLargerThanMaxArraySizeToFailCleanly() { cursor.close(); } + + @ParameterizedTest(name = "constructFromBytes={0}") + @ValueSource(booleans = {true, false}) + public void expectUseAfterCloseToHaveNoEffect(boolean constructFromBytes) { + // Using the cursor after close should not fail with an obscure unchecked + // exception like NullPointerException, ArrayIndexOutOfBoundsException, etc. + IonCursorBinary cursor = initializeCursor( + constructFromBytes, + 0xE0, 0x01, 0x00, 0xEA, + 0x20 + ); + cursor.close(); + assertEquals(IonCursor.Event.NEEDS_DATA, cursor.nextValue()); + assertNull(cursor.getValueMarker().typeId); + assertEquals(IonCursor.Event.NEEDS_DATA, cursor.nextValue()); + assertNull(cursor.getValueMarker().typeId); + cursor.close(); + } } diff --git a/src/test/java/com/amazon/ion/impl/IonReaderContinuableTopLevelBinaryTest.java b/src/test/java/com/amazon/ion/impl/IonReaderContinuableTopLevelBinaryTest.java index 476b667cd3..1080dc3962 100644 --- a/src/test/java/com/amazon/ion/impl/IonReaderContinuableTopLevelBinaryTest.java +++ b/src/test/java/com/amazon/ion/impl/IonReaderContinuableTopLevelBinaryTest.java @@ -4009,4 +4009,22 @@ public void corruptEachByteThrowsIonException(boolean constructFromBytes) { corruptEachByteThrowsIonException(constructFromBytes, false, true); corruptEachByteThrowsIonException(constructFromBytes, false, false); } + + @ParameterizedTest(name = "constructFromBytes={0}") + @ValueSource(booleans = {true, false}) + public void expectUseAfterCloseToHaveNoEffect(boolean constructFromBytes) throws Exception { + // Using the cursor after close should not fail with an obscure unchecked + // exception like NullPointerException, ArrayIndexOutOfBoundsException, etc. + reader = readerFor( + constructFromBytes, + 0xE0, 0x01, 0x00, 0xEA, + 0x20 + ); + reader.close(); + assertNull(reader.next()); + assertNull(reader.getType()); + assertNull(reader.next()); + assertNull(reader.getType()); + reader.close(); + } }