Fixed #260 (lazy byte[] allocation for longer binary payloads)

FasterXML · Mar 20, 2021 · 5740026 · 5740026
1 parent 9b82247
commit 5740026
Show file tree

Hide file tree

Showing 8 changed files with 116 additions and 11 deletions.
diff --git a/cbor/src/main/java/com/fasterxml/jackson/dataformat/cbor/CBORParser.java b/cbor/src/main/java/com/fasterxml/jackson/dataformat/cbor/CBORParser.java
@@ -2558,7 +2558,7 @@ protected byte[] _finishLongContiguousBytes(final int expLen) throws IOException
                 int count = Math.min(avail, left);
                 bb.write(_inputBuffer, _inputPtr, count);
                 _inputPtr += count;
-                left -= count;        
+                left -= count;
             }
             return bb.toByteArray();
         }

diff --git a/cbor/src/test/java/com/fasterxml/jackson/dataformat/cbor/fuzz/Fuzz32173LongTextTest.java b/cbor/src/test/java/com/fasterxml/jackson/dataformat/cbor/fuzz/Fuzz32173LongTextTest.java
@@ -10,7 +10,7 @@ public class Fuzz32173LongTextTest extends CBORTestBase
 {
     private final ObjectMapper MAPPER = cborMapper();
 
-    public void testInvalidShortText() throws Exception
+    public void testTruncatedLongText() throws Exception
     {
         final byte[] input = new byte[] {
                 0x7A, // Text value

diff --git a/pom.xml b/pom.xml
@@ -84,6 +84,10 @@
             <excludes>
               <exclude>com/fasterxml/jackson/**/failing/*.java</exclude>
             </excludes>
+	    <!-- 20-Mar-2021, tatu: limit heap for tests to catch some
+		 of "too big allocation" cases
+	    -->
+            <argLine>-Xmx1024m</argLine>
           </configuration>
 	</plugin>
       </plugins>

diff --git a/release-notes/CREDITS-2.x b/release-notes/CREDITS-2.x
@@ -172,6 +172,8 @@ Fabian Meumertzheim (fmeum@github)
 * Reported #259: (cbor) Failed to handle case of alleged String with length of
   Integer.MAX_VALUE
  (2.12.3)
+* Reported #260: (smile) Allocate byte[] lazily for longer Smile binary data payloads
+ (2.12.3)
 * Reported #261 (cbor) CBORParser need to validate zero-length byte[] for BigInteger
  (2.12.3)
 

diff --git a/release-notes/VERSION-2.x b/release-notes/VERSION-2.x
@@ -18,6 +18,8 @@ Modules:
  (reported by Fabian M)
 #259: (cbor) Failed to handle case of alleged String with length of Integer.MAX_VALUE
  (reported by Fabian M)
+#260: (smile) Allocate byte[] lazily for longer Smile binary data payloads
+ (reported by Fabian M)
 #261 (cbor) CBORParser need to validate zero-length byte[] for BigInteger 
  (reported by Fabian M)
 

diff --git a/smile/src/main/java/com/fasterxml/jackson/dataformat/smile/SmileParser.java b/smile/src/main/java/com/fasterxml/jackson/dataformat/smile/SmileParser.java
@@ -8,6 +8,7 @@
 import com.fasterxml.jackson.core.*;
 import com.fasterxml.jackson.core.io.IOContext;
 import com.fasterxml.jackson.core.sym.ByteQuadsCanonicalizer;
+import com.fasterxml.jackson.core.util.ByteArrayBuilder;
 
 import static com.fasterxml.jackson.dataformat.smile.SmileConstants.BYTE_MARKER_END_OF_STRING;
 
@@ -239,9 +240,11 @@ private final byte _nextByteGuaranteed() throws IOException
     }
 
     protected final void _loadMoreGuaranteed() throws IOException {
-        if (!_loadMore()) { _reportInvalidEOF(); }
+        if (!_loadMore()) {
+            _reportInvalidEOF();
+        }
     }
-    
+
     protected final boolean _loadMore() throws IOException
     {
         //_currInputRowStart -= _inputEnd;
@@ -1895,7 +1898,7 @@ protected final void _finishToken() throws IOException
                 _binaryValue = _read7BitBinaryWithLength();
                 return;
             case 7: // binary, raw
-                _finishRawBinary();
+                _binaryValue = _finishRawBinary();
                 return;
             }
         }
@@ -2403,27 +2406,67 @@ private final void _decodeLongUnicode() throws IOException
         _textBuffer.setCurrentLength(outPtr);
     }
 
-    private final void _finishRawBinary() throws IOException
+    private final byte[] _finishRawBinary() throws IOException
     {
         int byteLen = _readUnsignedVInt();
-        _binaryValue = new byte[byteLen];
+
+        // 20-Mar-2021, tatu [dataformats-binary#260]: avoid eager allocation
+        //   for very large content
+        if (byteLen > LONGEST_NON_CHUNKED_BINARY) {
+            return _finishLongContiguousBytes(byteLen);
+        }
+
+        final int expLen = byteLen;
+        final byte[] b = new byte[byteLen];
+
         if (_inputPtr >= _inputEnd) {
+            if (!_loadMore()) {
+                _reportIncompleteBinaryRead(expLen, 0);
+            }
             _loadMoreGuaranteed();
         }
         int ptr = 0;
         while (true) {
             int toAdd = Math.min(byteLen, _inputEnd - _inputPtr);
-            System.arraycopy(_inputBuffer, _inputPtr, _binaryValue, ptr, toAdd);
+            System.arraycopy(_inputBuffer, _inputPtr, b, ptr, toAdd);
             _inputPtr += toAdd;
             ptr += toAdd;
             byteLen -= toAdd;
             if (byteLen <= 0) {
-                return;
+                return b;
+            }
+            if (!_loadMore()) {
+                _reportIncompleteBinaryRead(expLen, ptr);
             }
-            _loadMoreGuaranteed();
         }
     }
-
+
+    // @since 2.12.3
+    protected byte[] _finishLongContiguousBytes(final int expLen) throws IOException
+    {
+        int left = expLen;
+
+        // 20-Mar-2021, tatu: Let's NOT use recycled instance since we have much
+        //   longer content and there is likely less benefit of trying to recycle
+        //   segments
+        try (final ByteArrayBuilder bb = new ByteArrayBuilder(LONGEST_NON_CHUNKED_BINARY >> 1)) {
+            while (left > 0) {
+                int avail = _inputEnd - _inputPtr;
+                if (avail <= 0) {
+                    if (!_loadMore()) {
+                        _reportIncompleteBinaryRead(expLen, expLen-left);
+                    }
+                    avail = _inputEnd - _inputPtr;
+                }
+                int count = Math.min(avail, left);
+                bb.write(_inputBuffer, _inputPtr, count);
+                _inputPtr += count;
+                left -= count;
+            }
+            return bb.toByteArray();
+        }
+    }
+
     /*
     /**********************************************************
     /* Internal methods, skipping
@@ -2693,6 +2736,13 @@ protected void _reportInvalidOther(int mask, int ptr) throws JsonParseException
         _reportInvalidOther(mask);
     }
 
+    // @since 2.12.3
+    protected void _reportIncompleteBinaryRead(int expLen, int actLen) throws IOException
+    {
+        _reportInvalidEOF(String.format(" for Binary value: expected %d bytes, only found %d",
+                expLen, actLen), _currToken);
+    }
+
     /*
     /**********************************************************
     /* Internal methods, other

diff --git a/smile/src/main/java/com/fasterxml/jackson/dataformat/smile/SmileParserBase.java b/smile/src/main/java/com/fasterxml/jackson/dataformat/smile/SmileParserBase.java
@@ -23,6 +23,10 @@ public abstract class SmileParserBase extends ParserMinimalBase
 {
     protected final static String[] NO_STRINGS = new String[0];
 
+    // 2.12.3: [dataformats-binary#260] Avoid OOME/DoS for bigger binary;
+    //  read only up to 250k
+    protected final static int LONGEST_NON_CHUNKED_BINARY = 250_000;
+
     /*
     /**********************************************************
     /* Config

diff --git a/smile/src/test/java/com/fasterxml/jackson/dataformat/smile/fuzz/Fuzz32180BinaryTest.java b/smile/src/test/java/com/fasterxml/jackson/dataformat/smile/fuzz/Fuzz32180BinaryTest.java
@@ -0,0 +1,43 @@
+package com.fasterxml.jackson.dataformat.smile.fuzz;
+
+import java.io.ByteArrayInputStream;
+import java.util.Arrays;
+
+import com.fasterxml.jackson.core.io.JsonEOFException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+
+import com.fasterxml.jackson.dataformat.smile.BaseTestForSmile;
+
+// For [dataformats-binary#]
+public class Fuzz32180BinaryTest extends BaseTestForSmile
+{
+    private final ObjectMapper MAPPER = smileMapper();
+
+    // Payload:
+    public void testInvalidBinary() throws Exception
+    {
+        final byte[] input0 = new byte[] {
+                0x3A, 0x29, 0x0A, 0x00, // smile signature
+                (byte) 0xFD, // raw binary
+                0x0F, 0x7E, 0x20,
+                0x20, (byte) 0xFF, // 5 byte VInt for 0x7fe4083f (close to Integer.MAX_VALUE)
+                // and one byte of binary payload
+                0x00
+        };
+        // Let's expand slightly to avoid too early detection, ensure that we are
+        // not only checking completely missing payload or such
+        final byte[] input = Arrays.copyOf(input0, 65 * 1024);
+
+        try (ByteArrayInputStream bytes = new ByteArrayInputStream(input)) {
+            try {
+            /*JsonNode root =*/ MAPPER.readTree(bytes);
+            } catch (JsonEOFException e) {
+                verifyException(e, "Unexpected end-of-input for Binary value: expected 2145650751 bytes, only found 66550");
+            } catch (OutOfMemoryError e) {
+                // Just to make it easier to see on fail (not ideal but better than nothing)
+                e.printStackTrace();
+                throw e;
+            }
+        }
+    }
+}