Refine offset, length size handling in Content.Sources (#13690)

Fix #14685 by handling zero length resources
* Propose the checkOffsetLengthSize contract
* More forgiving offsetLengthSize contract
* Update jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java

Co-authored-by: Ludovic Orban <[email protected]>

---------

Signed-off-by: Lachlan Roberts <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Co-authored-by: Lachlan Roberts <[email protected]>
Co-authored-by: Ludovic Orban <[email protected]>

Author: 3 people

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java

@@ -354,7 +354,9 @@ public final Content.Source createContentSource()
         @Override
         public Content.Source newContentSource(ByteBufferPool.Sized bufferPool, long offset, long length)
         {
-            return newContentSource();
+            // We call the deprecated newContentSource() to support existing subclasses.
+            // All current implementations of Part do override newContentSource(ByteBufferPool.Sized, long, long).
+            return Content.Source.from(newContentSource(), offset, length);
         }
 
         public long getLength()
@@ -499,10 +501,19 @@ public ByteBufferPart(String name, String fileName, HttpFields fields, List<Byte
             this.content = content;
         }
 
+        @Override
+        public long getLength()
+        {
+            long length = 0;
+            for (ByteBuffer b : content)
+                length += BufferUtil.length(b);
+            return length;
+        }
+
         @Override
         public Content.Source newContentSource(ByteBufferPool.Sized bufferPool, long offset, long length)
         {
-            return new ByteBufferContentSource(content);
+            return new ByteBufferContentSource(content, offset, length);
         }
 
         @Override
@@ -535,9 +546,21 @@ public ChunksPart(String name, String fileName, HttpFields fields, List<Content.
             content.forEach(Content.Chunk::retain);
         }
 
+        @Override
+        public long getLength()
+        {
+            long length = 0;
+            for (Content.Chunk c : content)
+                length += c.size();
+            return length;
+        }
+
         @Override
         public Content.Source newContentSource(ByteBufferPool.Sized bufferPool, long offset, long length)
         {
+            long size = getLength();
+            length = TypeUtil.checkOffsetLengthSize(offset, length, size);
+
             try (AutoLock ignored = lock.lock())
             {
                 if (closed)
@@ -556,7 +579,7 @@ public Content.Source newContentSource(ByteBufferPool.Sized bufferPool, long off
                 ChunksContentSource newContentSource = new ChunksContentSource(chunks);
                 chunks.forEach(Content.Chunk::release);
                 contentSources.add(newContentSource);
-                return newContentSource;
+                return Content.Source.from(newContentSource, offset, length);
             }
         }
 
@@ -650,12 +673,19 @@ public ContentSourcePart(String name, String fileName, HttpFields fields, Conten
             this.content = Objects.requireNonNull(content);
         }
 
+        @Override
+        public long getLength()
+        {
+            return content.getLength();
+        }
+
         @Override
         public Content.Source newContentSource(ByteBufferPool.Sized bufferPool, long offset, long length)
         {
+            length = TypeUtil.checkOffsetLengthSize(offset, length, content.getLength());
             Content.Source c = content;
             content = null;
-            return c;
+            return Content.Source.from(c, offset, length);
         }
 
         @Override

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/content/CachingHttpContentFactory.java

@@ -22,6 +22,7 @@
 import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.atomic.AtomicReference;
 
 import org.eclipse.jetty.http.CompressedContentFormat;
 import org.eclipse.jetty.http.HttpField;
@@ -36,6 +37,7 @@
 import org.eclipse.jetty.util.Callback;
 import org.eclipse.jetty.util.NanoTime;
 import org.eclipse.jetty.util.StringUtil;
+import org.eclipse.jetty.util.TypeUtil;
 import org.eclipse.jetty.util.resource.Resource;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -300,7 +302,7 @@ protected interface CachingHttpContent extends HttpContent
 
     protected class CachedHttpContent extends HttpContent.Wrapper implements CachingHttpContent
     {
-        private final RetainableByteBuffer _buffer;
+        private final AtomicReference<RetainableByteBuffer> _buffer = new AtomicReference<>();
         private final String _cacheKey;
         private final HttpField _etagField;
         private volatile long _lastAccessed;
@@ -333,7 +335,7 @@ public CachedHttpContent(String key, HttpContent httpContent)
                 throw new IllegalArgumentException("Resource is too large: length " + contentLengthValue + " > " + _maxCachedFileSize);
 
             // Read the content into memory
-            _buffer = IOResources.toRetainableByteBuffer(httpContent.getResource(), _bufferPool);
+            _buffer.set(IOResources.toRetainableByteBuffer(httpContent.getResource(), _bufferPool));
 
             _characterEncoding = httpContent.getCharacterEncoding();
             _compressedFormats = httpContent.getPreCompressedContentFormats();
@@ -364,12 +366,20 @@ public String getKey()
         @Override
         public void writeTo(Content.Sink sink, long offset, long length, Callback callback)
         {
+            RetainableByteBuffer buffer = _buffer.get();
+            if (buffer == null)
+            {
+                super.writeTo(sink, offset, length, callback);
+                return;
+            }
+
             boolean retained = false;
             try
             {
+                length = TypeUtil.checkOffsetLengthSize(offset, length, buffer.remaining());
                 retained = tryRetain();
                 if (retained)
-                    sink.write(true, BufferUtil.slice(_buffer.getByteBuffer(), Math.toIntExact(offset), Math.toIntExact(length)), Callback.from(this::release, callback));
+                    sink.write(true, BufferUtil.slice(buffer.getByteBuffer(), Math.toIntExact(offset), Math.toIntExact(length)), Callback.from(this::release, callback));
                 else
                     getWrapped().writeTo(sink, offset, length, callback);
             }
@@ -392,15 +402,18 @@ private boolean tryRetain()
         {
             return _cache.computeIfPresent(_cacheKey, (s, cachingHttpContent) ->
             {
-                _buffer.retain();
+                RetainableByteBuffer buffer = _buffer.get();
+                if (buffer == null)
+                    return null;
+                buffer.retain();
                 return cachingHttpContent;
             }) != null;
         }
 
         @Override
         public void release()
         {
-            _buffer.release();
+            _buffer.getAndUpdate(buffer -> (buffer != null && buffer.release()) ? null : buffer);
         }
 
         @Override
@@ -436,7 +449,7 @@ public HttpField getContentLength()
         @Override
         public long getContentLengthValue()
         {
-            return _buffer.remaining();
+            return _contentLength.getLongValue();
         }
 
         @Override

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/content/FileMappingHttpContentFactory.java

@@ -24,6 +24,7 @@
 import org.eclipse.jetty.util.BufferUtil;
 import org.eclipse.jetty.util.Callback;
 import org.eclipse.jetty.util.IteratingNestedCallback;
+import org.eclipse.jetty.util.TypeUtil;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -110,6 +111,7 @@ public void writeTo(Content.Sink sink, long offset, long length, Callback callba
         {
             try
             {
+                length = TypeUtil.checkOffsetLengthSize(offset, length, _buffer.remaining());
                 sink.write(true, BufferUtil.slice(_buffer, Math.toIntExact(offset), Math.toIntExact(length)), callback);
             }
             catch (Throwable x)
@@ -175,10 +177,7 @@ public void writeTo(Content.Sink sink, long offset, long length, Callback callba
         {
             try
             {
-                if (offset > getContentLengthValue())
-                    throw new IllegalArgumentException("Offset outside of mapped file range");
-                if (length > -1 && length + offset > getContentLengthValue())
-                    throw new IllegalArgumentException("Offset / length outside of mapped file range");
+                length = TypeUtil.checkOffsetLengthSize(offset, length, _contentLengthValue);
 
                 int beginIndex = Math.toIntExact(offset / maxBufferSize);
                 int firstOffset = Math.toIntExact(offset % maxBufferSize);

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/content/HttpContent.java

@@ -119,6 +119,7 @@ public interface HttpContent
      * @param sink the sink to write to.
      * @param offset the offset byte of the resource to start from.
      * @param length the length of the resource's contents to copy, -1 for the full length.
+     *               If the length is longer than the available content, the write is truncated to the available length.
      * @param callback the callback to notify when writing is done.
      */
     void writeTo(Content.Sink sink, long offset, long length, Callback callback);

jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/content/FileMappingHttpContentFactoryTest.java

@@ -90,12 +90,14 @@ public void testMultiBufferFileMappedOffsetAndLength() throws Exception
 
         HttpContent content = fileMappingHttpContentFactory.getContent("file.txt");
 
-        assertThrows(IllegalArgumentException.class, () -> writeToString(content, 0, 31));
-        assertThrows(IllegalArgumentException.class, () -> writeToString(content, 30, 1));
-        assertThrows(IllegalArgumentException.class, () -> writeToString(content, 31, 0));
+        assertThrows(IllegalArgumentException.class, () -> writeToString(content, -1, 0));
+        assertThrows(IndexOutOfBoundsException.class, () -> writeToString(content, 31, 1));
 
+        assertThat(writeToString(content, 0, 100), is("0123456789abcdefghijABCDEFGHIJ"));
+        assertThat(writeToString(content, 0, 31), is("0123456789abcdefghijABCDEFGHIJ"));
         assertThat(writeToString(content, 0, 30), is("0123456789abcdefghijABCDEFGHIJ"));
         assertThat(writeToString(content, 29, 1), is("J"));
+        assertThat(writeToString(content, 30, 1), is(""));
         assertThat(writeToString(content, 0, 0), is(""));
         assertThat(writeToString(content, 10, 0), is(""));
         assertThat(writeToString(content, 15, 0), is(""));

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java

@@ -42,6 +42,7 @@
 import org.eclipse.jetty.io.internal.ContentCopier;
 import org.eclipse.jetty.io.internal.ContentSourceByteBuffer;
 import org.eclipse.jetty.io.internal.ContentSourceConsumer;
+import org.eclipse.jetty.io.internal.ContentSourceRange;
 import org.eclipse.jetty.io.internal.ContentSourceRetainableByteBuffer;
 import org.eclipse.jetty.io.internal.ContentSourceString;
 import org.eclipse.jetty.util.Blocker;
@@ -169,9 +170,13 @@ interface Factory
              * Creates a new {@link Content.Source}.
              *
              * @param bufferPool the {@link ByteBufferPool.Sized} to get buffers from. {@code null} means allocate new buffers as needed.
-             * @param offset the offset byte of the resource to start from.
+             * @param offset the offset byte of the content to start from.
+             *               Must be greater than or equal to 0 and less than the content length (if known).
              * @param length the length of the content to make available, -1 for the full length.
+             *               If the size of the content is known, the length may be truncated to the content size minus the offset.
              * @return a {@link Content.Source}.
+             * @throws IndexOutOfBoundsException if the offset or length are out of range.
+             * @see TypeUtil#checkOffsetLengthSize(long, long, long)
              */
             Content.Source newContentSource(ByteBufferPool.Sized bufferPool, long offset, long length);
         }
@@ -198,16 +203,42 @@ static Content.Source from(Path path)
 
         /**
          * Create a {@code Content.Source} from a {@link Path}.
+         *
          * @param path The {@link Path}s to use as the source.
-         * @param offset The offset in bytes from which to start the source
-         * @param length The length in bytes of the source.
-         * @return A {@code Content.Source}
+         * @param offset the offset byte of the content to start from.
+         *               Must be greater than or equal to 0 and less than the content length (if known).
+         * @param length the length of the content to make available, -1 for the full length.
+         *               If the size of the content is known, the length may be truncated to the content size minus the offset.
+         * @return a {@link Content.Source}.
+         * @throws IndexOutOfBoundsException if the offset or length are out of range.
+         * @see TypeUtil#checkOffsetLengthSize(long, long, long)
          */
         static Content.Source from(Path path, long offset, long length)
         {
             return from(null, path, offset, length);
         }
 
+        /**
+         * Wrap a {@link Content.Source} to make it appear as a sub-range of the original.
+         *
+         * @param source The {@link Content.Source} to wrap.
+         * @param offset the offset byte of the content to start from.
+         *               Must be greater than or equal to 0 and less than the content length (if known).
+         * @param length the length of the content to make available, -1 for the full length.
+         *               If the size of the content is known, the length may be truncated to the content size minus the offset.
+         * @return a {@link Content.Source}.
+         * @throws IndexOutOfBoundsException if the offset or length are out of range.
+         * @see TypeUtil#checkOffsetLengthSize(long, long, long)
+         */
+        static Content.Source from(Content.Source source, long offset, long length)
+        {
+            // If the offset and length include the full content, then do not wrap.
+            if (offset == 0 && (length == -1 || length == source.getLength()))
+                return source;
+
+            return new ContentSourceRange(source, offset, length);
+        }
+
         /**
          * Create a {@code Content.Source} from a {@link Path}.
          * @param byteBufferPool The {@link org.eclipse.jetty.io.ByteBufferPool.Sized} to use for any internal buffers.
@@ -223,9 +254,13 @@ static Content.Source from(ByteBufferPool.Sized byteBufferPool, Path path)
          * Create a {@code Content.Source} from a {@link Path}.
          * @param byteBufferPool The {@link org.eclipse.jetty.io.ByteBufferPool.Sized} to use for any internal buffers.
          * @param path The {@link Path}s to use as the source.
-         * @param offset The offset in bytes from which to start the source
-         * @param length The length in bytes of the source, -1 for the full length.
-         * @return A {@code Content.Source}
+         * @param offset the offset byte of the content to start from.
+         *               Must be greater than or equal to 0 and less than the content length (if known).
+         * @param length the length of the content to make available, -1 for the full length.
+         *               If the size of the content is known, the length may be truncated to the content size minus the offset.
+         * @return a {@link Content.Source}.
+         * @throws IndexOutOfBoundsException if the offset or length are out of range.
+         * @see TypeUtil#checkOffsetLengthSize(long, long, long)
          */
         static Content.Source from(ByteBufferPool.Sized byteBufferPool, Path path, long offset, long length)
         {
@@ -247,9 +282,13 @@ static Content.Source from(ByteBufferPool.Sized byteBufferPool, ByteChannel byte
          * Create a {@code Content.Source} from a {@link ByteChannel}.
          * @param byteBufferPool The {@link org.eclipse.jetty.io.ByteBufferPool.Sized} to use for any internal buffers.
          * @param seekableByteChannel The {@link ByteChannel}s to use as the source.
-         * @param offset The offset in bytes from which to start the source
-         * @param length The length in bytes of the source.
-         * @return A {@code Content.Source}
+         * @param offset the offset byte of the content to start from.
+         *               Must be greater than or equal to 0 and less than the content length (if known).
+         * @param length the length of the content to make available, -1 for the full length.
+         *               If the size of the content is known, the length may be truncated to the content size minus the offset.
+         * @return a {@link Content.Source}.
+         * @throws IndexOutOfBoundsException if the offset or length are out of range.
+         * @see TypeUtil#checkOffsetLengthSize(long, long, long)
          */
         static Content.Source from(ByteBufferPool.Sized byteBufferPool, SeekableByteChannel seekableByteChannel, long offset, long length)
         {
@@ -276,9 +315,13 @@ static Content.Source from(ByteBufferPool.Sized byteBufferPool, InputStream inpu
          * Create a {@code Content.Source} from an {@link InputStream}.
          * @param byteBufferPool The {@link org.eclipse.jetty.io.ByteBufferPool.Sized} to use for any internal buffers.
          * @param inputStream The {@link InputStream}s to use as the source.
-         * @param offset The offset in bytes from which to start the source
-         * @param length The number of bytes to read from the source, or -1 to read to the end of the stream
-         * @return A {@code Content.Source}
+         * @param offset the offset byte of the resource to start from.
+         *               Must be greater than or equal to 0 and less than the resource size (if known).
+         * @param length the length of the content to make available, or -1 for the full length available.
+         *               The length may be truncated if the stream ends sooner.
+         * @return a {@link Content.Source}.
+         * @throws IndexOutOfBoundsException if the offset or length are out of range.
+         * @see TypeUtil#checkOffsetLengthSize(long, long, long)
          */
         static Content.Source from(ByteBufferPool.Sized byteBufferPool, InputStream inputStream, long offset, long length)
         {
@@ -961,6 +1004,19 @@ static Chunk from(ByteBuffer byteBuffer, boolean last)
             return last ? EOF : EMPTY;
         }
 
+        /**
+         * <p>Creates a Chunk with the given RetainableByteBuffer</p>
+         * <p>The returned Chunk is not {@link #retain() retained} and {@link #release() releasing it
+         * will release the passed buffer}.</p>
+         * @param buffer the RetainableByteBuffer to use to back the returned Chunk
+         * @param last whether the Chunk is the last one
+         * @return a buffer as a Chunk
+         */
+        static Chunk from(RetainableByteBuffer buffer, boolean last)
+        {
+            return new ByteBufferChunk.WithRetainableByteBuffer(buffer, last);
+        }
+
         /**
          * <p>Creates a Chunk with the given ByteBuffer.</p>
          * <p>The returned Chunk must be {@link #release() released}.</p>