From 38cc2edac3d4074feedd7662918667a7db3c350d Mon Sep 17 00:00:00 2001 From: PJ Fanning Date: Sun, 27 Oct 2024 16:34:40 +0000 Subject: [PATCH 1/6] handle int overflow issue that can cause infinite loops --- .../core/json/UTF8StreamJsonParser.java | 7 ++++-- .../async/NonBlockingUtf8JsonParserBase.java | 9 +++++-- .../jackson/core/util/CommonUtil.java | 25 +++++++++++++++++++ 3 files changed, 37 insertions(+), 4 deletions(-) create mode 100644 src/main/java/com/fasterxml/jackson/core/util/CommonUtil.java diff --git a/src/main/java/com/fasterxml/jackson/core/json/UTF8StreamJsonParser.java b/src/main/java/com/fasterxml/jackson/core/json/UTF8StreamJsonParser.java index 62aa0ed7af..b43ae40fb0 100644 --- a/src/main/java/com/fasterxml/jackson/core/json/UTF8StreamJsonParser.java +++ b/src/main/java/com/fasterxml/jackson/core/json/UTF8StreamJsonParser.java @@ -2544,7 +2544,9 @@ private final void _finishString2(char[] outBuf, int outPtr) outBuf = _textBuffer.finishCurrentSegment(); outPtr = 0; } - final int max = Math.min(_inputEnd, (ptr + (outBuf.length - outPtr))); + final int max = Math.min( + _inputEnd, + CommonUtil.addWithOverflowDefault(ptr, outBuf.length - outPtr)); while (ptr < max) { c = inputBuffer[ptr++] & 0xFF; if (codes[c] != 0) { @@ -2776,7 +2778,8 @@ protected JsonToken _handleApos() throws IOException } int max = _inputEnd; { - int max2 = _inputPtr + (outBuf.length - outPtr); + final int max2 = + CommonUtil.addWithOverflowDefault(_inputPtr, outBuf.length - outPtr); if (max2 < max) { max = max2; } diff --git a/src/main/java/com/fasterxml/jackson/core/json/async/NonBlockingUtf8JsonParserBase.java b/src/main/java/com/fasterxml/jackson/core/json/async/NonBlockingUtf8JsonParserBase.java index 0f9594dce4..d7af1a8fcb 100644 --- a/src/main/java/com/fasterxml/jackson/core/json/async/NonBlockingUtf8JsonParserBase.java +++ b/src/main/java/com/fasterxml/jackson/core/json/async/NonBlockingUtf8JsonParserBase.java @@ -6,6 +6,7 @@ import com.fasterxml.jackson.core.io.IOContext; import com.fasterxml.jackson.core.json.JsonReadFeature; import com.fasterxml.jackson.core.sym.ByteQuadsCanonicalizer; +import com.fasterxml.jackson.core.util.CommonUtil; import com.fasterxml.jackson.core.util.VersionUtil; import java.io.IOException; @@ -2554,7 +2555,9 @@ private final JsonToken _finishRegularString() throws IOException outBuf = _textBuffer.finishCurrentSegment(); outPtr = 0; } - final int max = Math.min(_inputEnd, (ptr + (outBuf.length - outPtr))); + final int max = Math.min( + _inputEnd, + CommonUtil.addWithOverflowDefault(ptr, outBuf.length - outPtr)); while (ptr < max) { c = getByteFromBuffer(ptr++) & 0xFF; if (codes[c] != 0) { @@ -2677,7 +2680,9 @@ private final JsonToken _finishAposString() throws IOException outBuf = _textBuffer.finishCurrentSegment(); outPtr = 0; } - final int max = Math.min(_inputEnd, (ptr + (outBuf.length - outPtr))); + final int max = Math.min( + _inputEnd, + CommonUtil.addWithOverflowDefault(ptr, outBuf.length - outPtr)); while (ptr < max) { c = getByteFromBuffer(ptr++) & 0xFF; if ((codes[c] != 0) && (c != INT_QUOTE)) { diff --git a/src/main/java/com/fasterxml/jackson/core/util/CommonUtil.java b/src/main/java/com/fasterxml/jackson/core/util/CommonUtil.java new file mode 100644 index 0000000000..e9d465e4be --- /dev/null +++ b/src/main/java/com/fasterxml/jackson/core/util/CommonUtil.java @@ -0,0 +1,25 @@ +package com.fasterxml.jackson.core.util; + +/** + * Internal Use Only. Helper class used some useful utility methods. + */ +public class CommonUtil { + + private CommonUtil() {} + + /** + * Internal Use Only. + *

+ * Method that will add two integers, and if result overflows, return + * {@link Integer#MAX_VALUE}. For performance reasons, does NOT check for + * the result being less than {@link Integer#MIN_VALUE}. + *

+ */ + public static int addWithOverflowDefault(final int a, final int b) { + final long result = (long) a + (long) b; + if (result > Integer.MAX_VALUE) { + return Integer.MAX_VALUE; + } + return (int) result; + } +} From 39ece3058829ccda0caf78f5d4faeb0d1111712d Mon Sep 17 00:00:00 2001 From: PJ Fanning Date: Sun, 27 Oct 2024 17:12:40 +0000 Subject: [PATCH 2/6] Create TestLargeString.java --- .../jackson/core/read/TestLargeString.java | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 src/test/java/com/fasterxml/jackson/core/read/TestLargeString.java diff --git a/src/test/java/com/fasterxml/jackson/core/read/TestLargeString.java b/src/test/java/com/fasterxml/jackson/core/read/TestLargeString.java new file mode 100644 index 0000000000..73d5bad743 --- /dev/null +++ b/src/test/java/com/fasterxml/jackson/core/read/TestLargeString.java @@ -0,0 +1,48 @@ +package com.fasterxml.jackson.core.read; + +import com.fasterxml.jackson.core.JUnit5TestBase; +import com.fasterxml.jackson.core.JsonFactory; +import com.fasterxml.jackson.core.JsonParser; +import com.fasterxml.jackson.core.StreamReadConstraints; +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +// https://github.com/FasterXML/jackson-core/pull/1350 +class TestLargeString extends JUnit5TestBase { + + // disabled because it takes too much memory to run + @Disabled + @Test + void testLargeStringDeserialization() throws Exception { + final int len = Integer.MAX_VALUE - 1024; + final byte[] largeByteString = makeLongByteString(len); + final JsonFactory f = JsonFactory.builder() + .streamReadConstraints(StreamReadConstraints.builder() + .maxStringLength(Integer.MAX_VALUE) + .build()) + .build(); + + try (JsonParser parser = f.createParser(largeByteString)) { + final String parsedString = parser.nextTextValue(); + assertEquals(len, parsedString.length()); + for (int i = 0; i < len; i++) { + assertEquals('a', parsedString.charAt(i)); + } + } + + } + + private byte[] makeLongByteString(int length) { + final byte[] result = new byte[length + 2]; + final byte b = 'a'; + final int last = length + 1; + result[0] = '\"'; + for (int i = 1; i < last; i++) { + result[i] = b; + } + result[last] = result[0]; + return result; + } +} From 3351b6a6b87c5fcfc9deeb3bc0e2e9100d621918 Mon Sep 17 00:00:00 2001 From: PJ Fanning Date: Sun, 27 Oct 2024 17:23:52 +0000 Subject: [PATCH 3/6] Update TestLargeString.java --- .../jackson/core/read/TestLargeString.java | 58 +++++++++---------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/src/test/java/com/fasterxml/jackson/core/read/TestLargeString.java b/src/test/java/com/fasterxml/jackson/core/read/TestLargeString.java index 73d5bad743..cbcafe787e 100644 --- a/src/test/java/com/fasterxml/jackson/core/read/TestLargeString.java +++ b/src/test/java/com/fasterxml/jackson/core/read/TestLargeString.java @@ -12,37 +12,37 @@ // https://github.com/FasterXML/jackson-core/pull/1350 class TestLargeString extends JUnit5TestBase { - // disabled because it takes too much memory to run - @Disabled - @Test - void testLargeStringDeserialization() throws Exception { - final int len = Integer.MAX_VALUE - 1024; - final byte[] largeByteString = makeLongByteString(len); - final JsonFactory f = JsonFactory.builder() - .streamReadConstraints(StreamReadConstraints.builder() - .maxStringLength(Integer.MAX_VALUE) - .build()) - .build(); + // disabled because it takes too much memory to run + @Disabled + @Test + void testLargeStringDeserialization() throws Exception { + final int len = Integer.MAX_VALUE - 1024; + final byte[] largeByteString = makeLongByteString(len); + final JsonFactory f = JsonFactory.builder() + .streamReadConstraints(StreamReadConstraints.builder() + .maxStringLength(Integer.MAX_VALUE) + .build()) + .build(); - try (JsonParser parser = f.createParser(largeByteString)) { - final String parsedString = parser.nextTextValue(); - assertEquals(len, parsedString.length()); - for (int i = 0; i < len; i++) { - assertEquals('a', parsedString.charAt(i)); - } - } + try (JsonParser parser = f.createParser(largeByteString)) { + final String parsedString = parser.nextTextValue(); + assertEquals(len, parsedString.length()); + for (int i = 0; i < len; i++) { + assertEquals('a', parsedString.charAt(i)); + } + } - } + } - private byte[] makeLongByteString(int length) { - final byte[] result = new byte[length + 2]; - final byte b = 'a'; - final int last = length + 1; - result[0] = '\"'; - for (int i = 1; i < last; i++) { - result[i] = b; + private byte[] makeLongByteString(int length) { + final byte[] result = new byte[length + 2]; + final byte b = 'a'; + final int last = length + 1; + result[0] = '\"'; + for (int i = 1; i < last; i++) { + result[i] = b; + } + result[last] = result[0]; + return result; } - result[last] = result[0]; - return result; - } } From 2832b8c6d97f2de79b3b058278cc8e36d5e72b4f Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Sun, 27 Oct 2024 18:47:39 -0700 Subject: [PATCH 4/6] Update release notes --- release-notes/CREDITS-2.x | 7 ++++++- release-notes/VERSION-2.x | 4 ++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/release-notes/CREDITS-2.x b/release-notes/CREDITS-2.x index b60d60bb62..c419898abf 100644 --- a/release-notes/CREDITS-2.x +++ b/release-notes/CREDITS-2.x @@ -433,4 +433,9 @@ Antonin Janec (@xtonic) * Contributed #1217: Optimize char comparison using bitwise OR (2.17.0) * Contributed #1218: Simplify Unicode surrogate pair conversion for generation - (2.17.0) + (2.17.0) + +Adam J. Shook (@adamjshook) + * Reported, suggested fix for #1352: Fix infinite loop due to integer overflow + when reading large strings + (2.17.3) diff --git a/release-notes/VERSION-2.x b/release-notes/VERSION-2.x index c4684c75d1..8088d98758 100644 --- a/release-notes/VERSION-2.x +++ b/release-notes/VERSION-2.x @@ -20,6 +20,10 @@ a pure JSON library. (contributed by @pjfanning) #1340: Missing `JsonFactory` "provides" SPI with JPMS in `jackson-core` module (contributed by @sdyura) +#1352: Fix infinite loop due to integer overflow when reading large strings + (reported by Adam J.S) + (fix contributed by @pjfanning) + 2.17.2 (05-Jul-2024) From b778083e9cb04e9c393c0dbae4e70aa14f6993c1 Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Sun, 27 Oct 2024 18:50:32 -0700 Subject: [PATCH 5/6] Test rename --- ...tLargeString.java => TestReadHumongousString.java} | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) rename src/test/java/com/fasterxml/jackson/core/read/{TestLargeString.java => TestReadHumongousString.java} (93%) diff --git a/src/test/java/com/fasterxml/jackson/core/read/TestLargeString.java b/src/test/java/com/fasterxml/jackson/core/read/TestReadHumongousString.java similarity index 93% rename from src/test/java/com/fasterxml/jackson/core/read/TestLargeString.java rename to src/test/java/com/fasterxml/jackson/core/read/TestReadHumongousString.java index cbcafe787e..f9633d7116 100644 --- a/src/test/java/com/fasterxml/jackson/core/read/TestLargeString.java +++ b/src/test/java/com/fasterxml/jackson/core/read/TestReadHumongousString.java @@ -1,17 +1,18 @@ package com.fasterxml.jackson.core.read; +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Test; + import com.fasterxml.jackson.core.JUnit5TestBase; import com.fasterxml.jackson.core.JsonFactory; import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.core.StreamReadConstraints; -import org.junit.jupiter.api.Disabled; -import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.assertEquals; -// https://github.com/FasterXML/jackson-core/pull/1350 -class TestLargeString extends JUnit5TestBase { - +// https://github.com/FasterXML/jackson-core/pull/1352 +class TestReadHumongousString extends JUnit5TestBase +{ // disabled because it takes too much memory to run @Disabled @Test From feb6ce8ac61887e2a5d84876888c6f8538a1428f Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Sun, 27 Oct 2024 19:59:11 -0700 Subject: [PATCH 6/6] Minor rewriting --- .../core/json/UTF8StreamJsonParser.java | 4 +-- .../async/NonBlockingUtf8JsonParserBase.java | 6 ++-- .../jackson/core/util/CommonUtil.java | 25 ----------------- .../core/util/InternalJacksonUtil.java | 25 +++++++++++++++++ .../core/read/TestReadHumongousString.java | 28 +++++++++++-------- 5 files changed, 47 insertions(+), 41 deletions(-) delete mode 100644 src/main/java/com/fasterxml/jackson/core/util/CommonUtil.java create mode 100644 src/main/java/com/fasterxml/jackson/core/util/InternalJacksonUtil.java diff --git a/src/main/java/com/fasterxml/jackson/core/json/UTF8StreamJsonParser.java b/src/main/java/com/fasterxml/jackson/core/json/UTF8StreamJsonParser.java index b43ae40fb0..b45143777b 100644 --- a/src/main/java/com/fasterxml/jackson/core/json/UTF8StreamJsonParser.java +++ b/src/main/java/com/fasterxml/jackson/core/json/UTF8StreamJsonParser.java @@ -2546,7 +2546,7 @@ private final void _finishString2(char[] outBuf, int outPtr) } final int max = Math.min( _inputEnd, - CommonUtil.addWithOverflowDefault(ptr, outBuf.length - outPtr)); + InternalJacksonUtil.addOverflowSafe(ptr, outBuf.length - outPtr)); while (ptr < max) { c = inputBuffer[ptr++] & 0xFF; if (codes[c] != 0) { @@ -2779,7 +2779,7 @@ protected JsonToken _handleApos() throws IOException int max = _inputEnd; { final int max2 = - CommonUtil.addWithOverflowDefault(_inputPtr, outBuf.length - outPtr); + InternalJacksonUtil.addOverflowSafe(_inputPtr, outBuf.length - outPtr); if (max2 < max) { max = max2; } diff --git a/src/main/java/com/fasterxml/jackson/core/json/async/NonBlockingUtf8JsonParserBase.java b/src/main/java/com/fasterxml/jackson/core/json/async/NonBlockingUtf8JsonParserBase.java index d7af1a8fcb..bbf4796635 100644 --- a/src/main/java/com/fasterxml/jackson/core/json/async/NonBlockingUtf8JsonParserBase.java +++ b/src/main/java/com/fasterxml/jackson/core/json/async/NonBlockingUtf8JsonParserBase.java @@ -6,7 +6,7 @@ import com.fasterxml.jackson.core.io.IOContext; import com.fasterxml.jackson.core.json.JsonReadFeature; import com.fasterxml.jackson.core.sym.ByteQuadsCanonicalizer; -import com.fasterxml.jackson.core.util.CommonUtil; +import com.fasterxml.jackson.core.util.InternalJacksonUtil; import com.fasterxml.jackson.core.util.VersionUtil; import java.io.IOException; @@ -2557,7 +2557,7 @@ private final JsonToken _finishRegularString() throws IOException } final int max = Math.min( _inputEnd, - CommonUtil.addWithOverflowDefault(ptr, outBuf.length - outPtr)); + InternalJacksonUtil.addOverflowSafe(ptr, outBuf.length - outPtr)); while (ptr < max) { c = getByteFromBuffer(ptr++) & 0xFF; if (codes[c] != 0) { @@ -2682,7 +2682,7 @@ private final JsonToken _finishAposString() throws IOException } final int max = Math.min( _inputEnd, - CommonUtil.addWithOverflowDefault(ptr, outBuf.length - outPtr)); + InternalJacksonUtil.addOverflowSafe(ptr, outBuf.length - outPtr)); while (ptr < max) { c = getByteFromBuffer(ptr++) & 0xFF; if ((codes[c] != 0) && (c != INT_QUOTE)) { diff --git a/src/main/java/com/fasterxml/jackson/core/util/CommonUtil.java b/src/main/java/com/fasterxml/jackson/core/util/CommonUtil.java deleted file mode 100644 index e9d465e4be..0000000000 --- a/src/main/java/com/fasterxml/jackson/core/util/CommonUtil.java +++ /dev/null @@ -1,25 +0,0 @@ -package com.fasterxml.jackson.core.util; - -/** - * Internal Use Only. Helper class used some useful utility methods. - */ -public class CommonUtil { - - private CommonUtil() {} - - /** - * Internal Use Only. - *

- * Method that will add two integers, and if result overflows, return - * {@link Integer#MAX_VALUE}. For performance reasons, does NOT check for - * the result being less than {@link Integer#MIN_VALUE}. - *

- */ - public static int addWithOverflowDefault(final int a, final int b) { - final long result = (long) a + (long) b; - if (result > Integer.MAX_VALUE) { - return Integer.MAX_VALUE; - } - return (int) result; - } -} diff --git a/src/main/java/com/fasterxml/jackson/core/util/InternalJacksonUtil.java b/src/main/java/com/fasterxml/jackson/core/util/InternalJacksonUtil.java new file mode 100644 index 0000000000..4b356552b5 --- /dev/null +++ b/src/main/java/com/fasterxml/jackson/core/util/InternalJacksonUtil.java @@ -0,0 +1,25 @@ +package com.fasterxml.jackson.core.util; + +/** + * Internal Use Only. Helper class used to contain some useful utility methods. + * + * @since 2.17.3 / 2.18.1 + */ +public abstract class InternalJacksonUtil { + /** + * Internal Use Only. + *

+ * Method that will add two non-negative integers, and if result overflows, return + * {@link Integer#MAX_VALUE}. For performance reasons, does NOT check for + * the result being less than {@link Integer#MIN_VALUE}, nor whether arguments + * are actually non-negative. + * This is usually used to implement overflow-safe bounds checking. + */ + public static int addOverflowSafe(final int base, final int length) { + int result = base + length; + if (result < 0) { + return Integer.MAX_VALUE; + } + return result; + } +} diff --git a/src/test/java/com/fasterxml/jackson/core/read/TestReadHumongousString.java b/src/test/java/com/fasterxml/jackson/core/read/TestReadHumongousString.java index f9633d7116..7158251373 100644 --- a/src/test/java/com/fasterxml/jackson/core/read/TestReadHumongousString.java +++ b/src/test/java/com/fasterxml/jackson/core/read/TestReadHumongousString.java @@ -2,19 +2,27 @@ import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; import com.fasterxml.jackson.core.JUnit5TestBase; import com.fasterxml.jackson.core.JsonFactory; import com.fasterxml.jackson.core.JsonParser; +import com.fasterxml.jackson.core.JsonToken; import com.fasterxml.jackson.core.StreamReadConstraints; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +import java.util.Arrays; +import java.util.concurrent.TimeUnit; // https://github.com/FasterXML/jackson-core/pull/1352 class TestReadHumongousString extends JUnit5TestBase { // disabled because it takes too much memory to run @Disabled + // Since we might get infinite loop: + @Timeout(value = 10, unit = TimeUnit.SECONDS, threadMode = Timeout.ThreadMode.SEPARATE_THREAD) @Test void testLargeStringDeserialization() throws Exception { final int len = Integer.MAX_VALUE - 1024; @@ -26,24 +34,22 @@ void testLargeStringDeserialization() throws Exception { .build(); try (JsonParser parser = f.createParser(largeByteString)) { - final String parsedString = parser.nextTextValue(); - assertEquals(len, parsedString.length()); - for (int i = 0; i < len; i++) { - assertEquals('a', parsedString.charAt(i)); - } + assertToken(JsonToken.VALUE_STRING, parser.nextToken()); + // Let's not construct String but just check that length is + // expected: this avoids having to allocate 4 gig more of heap + // for test -- should still trigger problem if fix not valid + assertEquals(len, parser.getTextLength()); + // TODO: could use streaming accessor (`JsonParser.getText(Writer)`) + assertNull(parser.nextToken()); } } private byte[] makeLongByteString(int length) { final byte[] result = new byte[length + 2]; - final byte b = 'a'; - final int last = length + 1; + Arrays.fill(result, (byte) 'a'); result[0] = '\"'; - for (int i = 1; i < last; i++) { - result[i] = b; - } - result[last] = result[0]; + result[length + 1] = '\"'; return result; } }