From 93016eb03e7901a4d3023d9352be7622dffd958a Mon Sep 17 00:00:00 2001 From: Arthur Chan Date: Wed, 27 Dec 2023 08:02:33 +0000 Subject: [PATCH 1/5] Fix for 432: Wrap AssertionError Signed-off-by: Arthur Chan --- .../jackson/dataformat/ion/IonParser.java | 73 ++++++++++++++++--- .../ion/fuzz/Fuzz432_65273_AssertionTest.java | 34 +++++++++ ion/src/test/resources/data/fuzz-65273.ion | 1 + 3 files changed, 96 insertions(+), 12 deletions(-) create mode 100644 ion/src/test/java/com/fasterxml/jackson/dataformat/ion/fuzz/Fuzz432_65273_AssertionTest.java create mode 100644 ion/src/test/resources/data/fuzz-65273.ion diff --git a/ion/src/main/java/com/fasterxml/jackson/dataformat/ion/IonParser.java b/ion/src/main/java/com/fasterxml/jackson/dataformat/ion/IonParser.java index 479fba48a..6864a3a71 100644 --- a/ion/src/main/java/com/fasterxml/jackson/dataformat/ion/IonParser.java +++ b/ion/src/main/java/com/fasterxml/jackson/dataformat/ion/IonParser.java @@ -278,9 +278,10 @@ public String getText() throws IOException // trying to get the text for a symbol id that cannot be resolved. // stringValue() has an assert statement which could throw an throw _constructError(e.getMessage(), e); - } catch (AssertionError e) { + } catch (AssertionError | NullPointerException e) { // AssertionError if we're trying to get the text with a symbol // id less than or equals to 0. + // NullPointerException may also be thrown on invalid data String msg = e.getMessage(); if (msg == null) { msg = "UNKNOWN ROOT CAUSE"; @@ -331,34 +332,59 @@ public int getTextOffset() throws IOException { @Override public BigInteger getBigIntegerValue() throws IOException { - return _reader.bigIntegerValue(); + _verifyIsNumberToken(); + try { + return _reader.bigIntegerValue(); + } catch (IonException e) { + return _reportCorruptNumber(e); + } } @Override public BigDecimal getDecimalValue() throws IOException { - return _reader.bigDecimalValue(); + + _verifyIsNumberToken(); + try { + return _reader.bigDecimalValue(); + } catch (IonException e) { + return _reportCorruptNumber(e); + } } @Override public double getDoubleValue() throws IOException { + _verifyIsNumberToken(); return _reader.doubleValue(); } @Override public float getFloatValue() throws IOException { + _verifyIsNumberToken(); return (float) _reader.doubleValue(); } @Override public int getIntValue() throws IOException { + _verifyIsNumberToken(); return _reader.intValue(); } @Override public long getLongValue() throws IOException { + _verifyIsNumberToken(); return _reader.longValue(); } + // @since 2.17 + private void _verifyIsNumberToken() throws IOException + { + if (_currToken != JsonToken.VALUE_NUMBER_INT && _currToken != JsonToken.VALUE_NUMBER_FLOAT) { + // Same as `ParserBase._parseNumericValue()` exception: + _reportError("Current token (%s) not numeric, can not use numeric value accessors", + _currToken); + } + } + @Override public NumberType getNumberType() throws IOException { @@ -549,13 +575,20 @@ public JsonToken nextToken() throws IOException try { type = _reader.next(); } catch (IonException e) { - _wrapError(e.getMessage(), e); - - // [dataformats-binary#420]: IonJava leaks IOOBEs so: + return _reportCorruptContent(e); } catch (IndexOutOfBoundsException e) { - _wrapError(String.format("Corrupt content to decode; underlying failure: (%s) %s", - e.getClass().getName(), e.getMessage()), - e); + // [dataformats-binary#420]: IonJava leaks IOOBEs so: + return _reportCorruptContent(e); + } catch (AssertionError e) { + // [dataformats-binary#432]: + // AssertionError if we're trying to get the text with a symbol + // id less than or equals to 0. + String msg = e.getMessage(); + if (msg == null) { + msg = "UNKNOWN ROOT CAUSE"; + } + return _reportCorruptContent( + _constructError("Internal `IonReader` error: "+msg, e)); } if (type == null) { if (_parsingContext.inRoot()) { // EOF? @@ -572,13 +605,15 @@ public JsonToken nextToken() throws IOException boolean inStruct = !_parsingContext.inRoot() && _reader.isInStruct(); // (isInStruct can return true for the first value read if the reader // was created from an IonValue that has a parent container) + final String name; try { // getFieldName() can throw an UnknownSymbolException if the text of the // field name symbol cannot be resolved. - _parsingContext.setCurrentName(inStruct ? _reader.getFieldName() : null); - } catch (UnknownSymbolException e) { - _wrapError(e.getMessage(), e); + name = inStruct ? _reader.getFieldName() : null; + } catch (IonException e) { + return _reportCorruptContent(e); } + _parsingContext.setCurrentName(name); JsonToken t = _tokenFromType(type); // and return either field name first if (inStruct) { @@ -692,6 +727,20 @@ protected void _handleEOF() throws JsonParseException } } + private T _reportCorruptContent(Exception e) throws IOException + { + final String msg = String.format("Corrupt content to decode; underlying failure: (%s) %s", + e.getClass().getName(), e.getMessage()); + throw _constructError(msg, e); + } + + private T _reportCorruptNumber(Exception e) throws IOException + { + final String msg = String.format("Corrupt Number value to decode; underlying failure: (%s) %s", + e.getClass().getName(), e.getMessage()); + throw _constructError(msg, e); + } + @Override public void overrideCurrentName(String name) { try { diff --git a/ion/src/test/java/com/fasterxml/jackson/dataformat/ion/fuzz/Fuzz432_65273_AssertionTest.java b/ion/src/test/java/com/fasterxml/jackson/dataformat/ion/fuzz/Fuzz432_65273_AssertionTest.java new file mode 100644 index 000000000..c7cc4b665 --- /dev/null +++ b/ion/src/test/java/com/fasterxml/jackson/dataformat/ion/fuzz/Fuzz432_65273_AssertionTest.java @@ -0,0 +1,34 @@ +package com.fasterxml.jackson.dataformat.ion.fuzz; + +import java.io.InputStream; + +import org.hamcrest.Matchers; +import org.junit.Test; + +import com.fasterxml.jackson.core.JsonParser; +import com.fasterxml.jackson.core.JsonToken; +import com.fasterxml.jackson.core.exc.StreamReadException; +import com.fasterxml.jackson.dataformat.ion.*; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; + +// [dataformats-binary#4432 +public class Fuzz432_65273_AssertionTest +{ + private final IonFactory factory = + IonFactory.builderForTextualWriters().build(); + + @Test + public void testFuzz65273() throws Exception { + try (InputStream in = getClass().getResourceAsStream("/data/fuzz-65273.ion")) { + try (JsonParser p = factory.createParser(in)) { + p.nextToken(); + } + fail("Should not pass (invalid content)"); + } catch (StreamReadException e) { + assertThat(e.getMessage(), Matchers.containsString("Corrupt content to decode; underlying failure")); + } + } +} diff --git a/ion/src/test/resources/data/fuzz-65273.ion b/ion/src/test/resources/data/fuzz-65273.ion new file mode 100644 index 000000000..c148654b0 --- /dev/null +++ b/ion/src/test/resources/data/fuzz-65273.ion @@ -0,0 +1 @@ +$ion_symbol_table::{imports:$0//// / ÐÊÊÊt \ No newline at end of file From 78dea2555b9083181ae51c67ddf0fecf64fc7ce6 Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Fri, 29 Dec 2023 18:08:38 -0800 Subject: [PATCH 2/5] Minor tweak: simplify error handling for corrupt content --- .../jackson/dataformat/ion/IonParser.java | 36 ++++++++----------- .../ion/fuzz/Fuzz417_64721InvalidIonTest.java | 2 +- .../ion/fuzz/Fuzz424_65065_65126NPETest.java | 2 +- 3 files changed, 16 insertions(+), 24 deletions(-) diff --git a/ion/src/main/java/com/fasterxml/jackson/dataformat/ion/IonParser.java b/ion/src/main/java/com/fasterxml/jackson/dataformat/ion/IonParser.java index fbbe545d3..e984a5f88 100644 --- a/ion/src/main/java/com/fasterxml/jackson/dataformat/ion/IonParser.java +++ b/ion/src/main/java/com/fasterxml/jackson/dataformat/ion/IonParser.java @@ -273,20 +273,16 @@ public String getText() throws IOException case VALUE_STRING: try { return _reader.stringValue(); - } catch (UnknownSymbolException e) { + } catch (UnknownSymbolException // stringValue() will throw an UnknownSymbolException if we're // trying to get the text for a symbol id that cannot be resolved. // stringValue() has an assert statement which could throw an - throw _constructError(e.getMessage(), e); - } catch (AssertionError | NullPointerException e) { + | AssertionError | NullPointerException e // AssertionError if we're trying to get the text with a symbol // id less than or equals to 0. // NullPointerException may also be thrown on invalid data - String msg = e.getMessage(); - if (msg == null) { - msg = "UNKNOWN ROOT CAUSE"; - } - throw _constructError("Internal `IonReader` error: "+msg, e); + ) { + _reportCorruptContent(e); } case VALUE_NUMBER_INT: case VALUE_NUMBER_FLOAT: @@ -577,19 +573,11 @@ public JsonToken nextToken() throws IOException } catch (IonException e) { return _reportCorruptContent(e); - } catch (IndexOutOfBoundsException e) { + } catch (IndexOutOfBoundsException | AssertionError e) { // [dataformats-binary#420]: IonJava leaks IOOBEs so: + // [dataformats-binary#432]: AssertionError if we're trying to get the text + // with a symbol id less than or equals to 0. return _reportCorruptContent(e); - } catch (AssertionError e) { - // [dataformats-binary#432]: - // AssertionError if we're trying to get the text with a symbol - // id less than or equals to 0. - String msg = e.getMessage(); - if (msg == null) { - msg = "UNKNOWN ROOT CAUSE"; - } - return _reportCorruptContent( - _constructError("Internal `IonReader` error: "+msg, e)); } if (type == null) { if (_parsingContext.inRoot()) { // EOF? @@ -728,14 +716,18 @@ protected void _handleEOF() throws JsonParseException } } - private T _reportCorruptContent(Exception e) throws IOException + private T _reportCorruptContent(Throwable e) throws IOException { + String origMsg = e.getMessage(); + if (origMsg == null) { + origMsg = "[no exception message]"; + } final String msg = String.format("Corrupt content to decode; underlying failure: (%s) %s", - e.getClass().getName(), e.getMessage()); + e.getClass().getName(), origMsg); throw _constructError(msg, e); } - private T _reportCorruptNumber(Exception e) throws IOException + private T _reportCorruptNumber(Throwable e) throws IOException { final String msg = String.format("Corrupt Number value to decode; underlying failure: (%s) %s", e.getClass().getName(), e.getMessage()); diff --git a/ion/src/test/java/com/fasterxml/jackson/dataformat/ion/fuzz/Fuzz417_64721InvalidIonTest.java b/ion/src/test/java/com/fasterxml/jackson/dataformat/ion/fuzz/Fuzz417_64721InvalidIonTest.java index b51e4a460..724a98d81 100644 --- a/ion/src/test/java/com/fasterxml/jackson/dataformat/ion/fuzz/Fuzz417_64721InvalidIonTest.java +++ b/ion/src/test/java/com/fasterxml/jackson/dataformat/ion/fuzz/Fuzz417_64721InvalidIonTest.java @@ -27,7 +27,7 @@ public void testFuzz64721AssertionException() throws Exception { mapper.readValue("$0/", EnumFuzz.class); fail("Should not pass (invalid content)"); } catch (StreamReadException e) { - assertThat(e.getMessage(), Matchers.containsString("Internal `IonReader` error")); + assertThat(e.getMessage(), Matchers.containsString("Corrupt content to decode")); } } } diff --git a/ion/src/test/java/com/fasterxml/jackson/dataformat/ion/fuzz/Fuzz424_65065_65126NPETest.java b/ion/src/test/java/com/fasterxml/jackson/dataformat/ion/fuzz/Fuzz424_65065_65126NPETest.java index e526cb0c9..aa15caf2e 100644 --- a/ion/src/test/java/com/fasterxml/jackson/dataformat/ion/fuzz/Fuzz424_65065_65126NPETest.java +++ b/ion/src/test/java/com/fasterxml/jackson/dataformat/ion/fuzz/Fuzz424_65065_65126NPETest.java @@ -23,7 +23,7 @@ public void testFuzz65065() throws Exception { MAPPER.readTree(new ByteArrayInputStream(bytes)); fail("Should not pass (invalid content)"); } catch (StreamReadException e) { - assertThat(e.getMessage(), Matchers.containsString("Internal `IonReader` error")); + assertThat(e.getMessage(), Matchers.containsString("Corrupt content to decode")); } } From 7ad8d74cb2fce6adf36e2760fa3c7871307a8675 Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Fri, 29 Dec 2023 18:10:26 -0800 Subject: [PATCH 3/5] Yet more error handling tweaking --- .../com/fasterxml/jackson/dataformat/ion/IonParser.java | 2 +- .../dataformat/ion/fuzz/Fuzz432_65273_AssertionTest.java | 6 ++---- .../jackson/dataformat/ion/fuzz/Fuzz_65062_VarintTest.java | 2 +- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/ion/src/main/java/com/fasterxml/jackson/dataformat/ion/IonParser.java b/ion/src/main/java/com/fasterxml/jackson/dataformat/ion/IonParser.java index e984a5f88..7afabd9ce 100644 --- a/ion/src/main/java/com/fasterxml/jackson/dataformat/ion/IonParser.java +++ b/ion/src/main/java/com/fasterxml/jackson/dataformat/ion/IonParser.java @@ -722,7 +722,7 @@ private T _reportCorruptContent(Throwable e) throws IOException if (origMsg == null) { origMsg = "[no exception message]"; } - final String msg = String.format("Corrupt content to decode; underlying failure: (%s) %s", + final String msg = String.format("Corrupt content to decode; underlying `IonReader` error: (%s) %s", e.getClass().getName(), origMsg); throw _constructError(msg, e); } diff --git a/ion/src/test/java/com/fasterxml/jackson/dataformat/ion/fuzz/Fuzz432_65273_AssertionTest.java b/ion/src/test/java/com/fasterxml/jackson/dataformat/ion/fuzz/Fuzz432_65273_AssertionTest.java index c7cc4b665..e81e881d6 100644 --- a/ion/src/test/java/com/fasterxml/jackson/dataformat/ion/fuzz/Fuzz432_65273_AssertionTest.java +++ b/ion/src/test/java/com/fasterxml/jackson/dataformat/ion/fuzz/Fuzz432_65273_AssertionTest.java @@ -6,15 +6,13 @@ import org.junit.Test; import com.fasterxml.jackson.core.JsonParser; -import com.fasterxml.jackson.core.JsonToken; import com.fasterxml.jackson.core.exc.StreamReadException; import com.fasterxml.jackson.dataformat.ion.*; import static org.hamcrest.MatcherAssert.assertThat; -import static org.junit.Assert.assertEquals; import static org.junit.Assert.fail; -// [dataformats-binary#4432 +// [dataformats-binary#4432] public class Fuzz432_65273_AssertionTest { private final IonFactory factory = @@ -28,7 +26,7 @@ public void testFuzz65273() throws Exception { } fail("Should not pass (invalid content)"); } catch (StreamReadException e) { - assertThat(e.getMessage(), Matchers.containsString("Corrupt content to decode; underlying failure")); + assertThat(e.getMessage(), Matchers.containsString("Corrupt content to decode; underlying")); } } } diff --git a/ion/src/test/java/com/fasterxml/jackson/dataformat/ion/fuzz/Fuzz_65062_VarintTest.java b/ion/src/test/java/com/fasterxml/jackson/dataformat/ion/fuzz/Fuzz_65062_VarintTest.java index 8ecc5863f..770afab37 100644 --- a/ion/src/test/java/com/fasterxml/jackson/dataformat/ion/fuzz/Fuzz_65062_VarintTest.java +++ b/ion/src/test/java/com/fasterxml/jackson/dataformat/ion/fuzz/Fuzz_65062_VarintTest.java @@ -33,7 +33,7 @@ public void testFuzz65062_Varint() throws Exception { fail("Should not pass (invalid content)"); } catch (StreamReadException e) { // 21-Dec-2023, tatu: Not 100% sure why we won't get Number-specific fail but: - assertThat(e.getMessage(), Matchers.containsString("Corrupt content to decode; underlying failure")); + assertThat(e.getMessage(), Matchers.containsString("Corrupt content to decode; underlying")); } } } From dd4a3d7240c87882ea4dbe3d941a564d11026c51 Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Fri, 29 Dec 2023 18:12:17 -0800 Subject: [PATCH 4/5] ... --- .../fasterxml/jackson/dataformat/ion/IonParser.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/ion/src/main/java/com/fasterxml/jackson/dataformat/ion/IonParser.java b/ion/src/main/java/com/fasterxml/jackson/dataformat/ion/IonParser.java index 7afabd9ce..cf7a727a6 100644 --- a/ion/src/main/java/com/fasterxml/jackson/dataformat/ion/IonParser.java +++ b/ion/src/main/java/com/fasterxml/jackson/dataformat/ion/IonParser.java @@ -282,7 +282,7 @@ public String getText() throws IOException // id less than or equals to 0. // NullPointerException may also be thrown on invalid data ) { - _reportCorruptContent(e); + return _reportCorruptContent(e); } case VALUE_NUMBER_INT: case VALUE_NUMBER_FLOAT: @@ -722,15 +722,19 @@ private T _reportCorruptContent(Throwable e) throws IOException if (origMsg == null) { origMsg = "[no exception message]"; } - final String msg = String.format("Corrupt content to decode; underlying `IonReader` error: (%s) %s", + final String msg = String.format("Corrupt content to decode; underlying `IonReader` problem: (%s) %s", e.getClass().getName(), origMsg); throw _constructError(msg, e); } private T _reportCorruptNumber(Throwable e) throws IOException { - final String msg = String.format("Corrupt Number value to decode; underlying failure: (%s) %s", - e.getClass().getName(), e.getMessage()); + String origMsg = e.getMessage(); + if (origMsg == null) { + origMsg = "[no exception message]"; + } + final String msg = String.format("Corrupt Number value to decode; underlying `IonReader` problem: (%s) %s", + e.getClass().getName(), origMsg); throw _constructError(msg, e); } From d4964e412489a591c239c5ad780c9733680214be Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Fri, 29 Dec 2023 18:53:00 -0800 Subject: [PATCH 5/5] Update release notes --- release-notes/CREDITS-2.x | 3 +++ release-notes/VERSION-2.x | 2 ++ 2 files changed, 5 insertions(+) diff --git a/release-notes/CREDITS-2.x b/release-notes/CREDITS-2.x index 45dad8e85..e7246ea0c 100644 --- a/release-notes/CREDITS-2.x +++ b/release-notes/CREDITS-2.x @@ -292,3 +292,6 @@ Arthur Chan (@arthurscchan) (2.17.0) * Contributed #426: (smile) `SmileParser` throws unexpected IOOBE for corrupt content (2.17.0) + * Contributed #432 (ion) More methods from `IonReader` could throw an unexpected + `AssertionError` + (2.17.0) diff --git a/release-notes/VERSION-2.x b/release-notes/VERSION-2.x index 11879fe3e..c9254de34 100644 --- a/release-notes/VERSION-2.x +++ b/release-notes/VERSION-2.x @@ -26,6 +26,8 @@ Active maintainers: (fix contributed by Arthur C) #426: (smile) `SmileParser` throws unexpected IOOBE for corrupt content (fix contributed by Arthur C) +#432: (ion) More methods from `IonReader` could throw an unexpected `AssertionError` + (fix contributed by Arthur C) -(ion) Update `com.amazon.ion:ion-java` to 1.11.0 (from 1.10.5) 2.16.1 (24-Dec-2023)