#8993: Disallow creating new chunks with an empty ByteBuffer to make…

… Chunk.isTerminal() contract stricter

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/ResponseNotifier.java

@@ -369,7 +369,7 @@ private void onChunk(Content.Chunk chunk)
                 if (LOG.isDebugEnabled())
                     LOG.debug("Registering content in multiplexed content source #{} that contains {}", index, currentChunk);
                 if (currentChunk == null || currentChunk == AlreadyReadChunk.INSTANCE)
-                    this.chunk = Content.Chunk.slice(chunk);
+                    this.chunk = chunk.slice();
                 else if (!currentChunk.isLast())
                     throw new IllegalStateException("Cannot overwrite chunk");
                 onDemandCallback();

jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/internal/HttpReceiverOverHTTP2.java

@@ -80,10 +80,14 @@ public Content.Chunk read(boolean fillInterestIfNeeded)
             return null;
         }
         DataFrame frame = data.frame();
-        boolean last = frame.remaining() == 0 && frame.isEndStream();
-        if (last)
+        boolean terminal = !frame.getData().hasRemaining() && frame.isEndStream();
+        if (terminal)
+        {
+            data.release();
             responseSuccess(getHttpExchange(), null);
-        return Content.Chunk.from(frame.getData(), last, data);
+            return Content.Chunk.EOF;
+        }
+        return Content.Chunk.from(frame.getData(), terminal, data);
     }
 
     @Override

jetty-core/jetty-http3/jetty-http3-client-transport/src/main/java/org/eclipse/jetty/http3/client/transport/internal/HttpReceiverOverHTTP3.java

@@ -64,10 +64,14 @@ public Content.Chunk read(boolean fillInterestIfNeeded)
             return null;
         }
         ByteBuffer byteBuffer = data.getByteBuffer();
-        boolean last = !byteBuffer.hasRemaining() && data.isLast();
-        if (last)
+        boolean terminal = !byteBuffer.hasRemaining() && data.isLast();
+        if (terminal)
+        {
+            data.release();
             responseSuccess(getHttpExchange(), null);
-        return Content.Chunk.from(byteBuffer, last, data);
+            return Content.Chunk.EOF;
+        }
+        return Content.Chunk.from(byteBuffer, terminal, data);
     }
 
     @Override

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

@@ -437,6 +437,8 @@ public interface Chunk extends Retainable
         /**
          * <p>Creates a Chunk with the given ByteBuffer.</p>
          * <p>The returned Chunk must be {@link #release() released}.</p>
+         * <p>New chunks with an empty {@code byteBuffer} and {@code last}
+         * set to {@code true} cannot be created.</p>
          *
          * @param byteBuffer the ByteBuffer with the bytes of this Chunk
          * @param last whether the Chunk is the last one
@@ -463,6 +465,8 @@ static Chunk from(ByteBuffer byteBuffer, boolean last, Runnable releaser)
         /**
          * <p>Creates a last/non-last Chunk with the given ByteBuffer.</p>
          * <p>The returned Chunk must be {@link #release() released}.</p>
+         * <p>New chunks with an empty {@code byteBuffer} and {@code last}
+         * set to {@code true} cannot be created.</p>
          *
          * @param byteBuffer the ByteBuffer with the bytes of this Chunk
          * @param last whether the Chunk is the last one
@@ -478,6 +482,8 @@ static Chunk from(ByteBuffer byteBuffer, boolean last, Consumer<ByteBuffer> rele
          * <p>Creates a last/non-last Chunk with the given ByteBuffer, linked to the given Retainable.</p>
          * <p>The {@link #retain()} and {@link #release()} methods of this Chunk will delegate to the
          * given Retainable.</p>
+         * <p>New chunks with an empty {@code byteBuffer} and {@code last}
+         * set to {@code true} cannot be created.</p>
          *
          * @param byteBuffer the ByteBuffer with the bytes of this Chunk
          * @param last whether the Chunk is the last one
@@ -539,28 +545,6 @@ static Chunk next(Chunk chunk)
             return null;
         }
 
-        /**
-         * <p>Creates a chunk that is a slice of the given chunk.</p>
-         * <p>
-         *  The chunk slice has a byte buffer that is a slice of the original chunk's byte buffer, the last flag
-         *  copied over and the original chunk used as the retainable of the chunk slice.
-         *  Note that after this method returns, an extra Chunk instance refers to the same byte buffer,
-         *  so the original chunk is retained and is used as the {@link Retainable} for the returned chunk.
-         * </p>
-         * <p>Passing a null chunk returns null.</p>
-         * <p>Passing a terminal chunk returns it.</p>
-         *
-         * @param chunk the chunk to slice.
-         * @return the chunk slice.
-         */
-        static Chunk slice(Chunk chunk)
-        {
-            if (chunk == null || chunk.isTerminal())
-                return chunk;
-            chunk.retain();
-            return Chunk.from(chunk.getByteBuffer().slice(), chunk.isLast(), chunk);
-        }
-
         /**
          * @return the ByteBuffer of this Chunk
          */
@@ -571,6 +555,23 @@ static Chunk slice(Chunk chunk)
          */
         boolean isLast();
 
+        /**
+         * <p>Returns a new {@code Chunk} whose {@code ByteBuffer} is a slice of the
+         * {@code ByteBuffer} of the source {@code Chunk}.</p>
+         * <p>The returned {@code Chunk} retains the source {@code Chunk} and it is linked
+         * to it via {@link #from(ByteBuffer, boolean, Retainable)}.</p>
+         *
+         * @return a new {@code Chunk} retained from the source {@code Chunk} with a slice
+         * of the source {@code Chunk}'s {@code ByteBuffer}
+         */
+        default Chunk slice()
+        {
+            if (isTerminal())
+                return this;
+            retain();
+            return from(getByteBuffer().slice(), isLast(), this);
+        }
+
         /**
          * <p>Returns a new {@code Chunk} whose {@code ByteBuffer} is a slice, with the given
          * position and limit, of the {@code ByteBuffer} of the source {@code Chunk}.</p>
@@ -649,20 +650,27 @@ default int skip(int length)
 
         /**
          * <p>Returns whether this Chunk is a <em>terminal</em> chunk.</p>
-         * <p>A terminal chunk is either an {@link Error error chunk},
-         * or a Chunk that {@link #isLast()} is true and has no remaining
-         * bytes.</p>
+         * <p>A terminal chunk is a Chunk that {@link #isLast()} is true
+         * and has no remaining bytes.</p>
+         * <p><em>Terminal</em> chunks cannot be lifecycled using the
+         * {@link Retainable} contract like other chunks: they always throw
+         * {@link UnsupportedOperationException} on {@link #retain()} and
+         * always return {@code true} on {@link #release()}. As such, they
+         * cannot contain a recyclable buffer and calling their
+         * {@link #release()} method isn't necessary, although harmless.
+         * </p>
          *
          * @return whether this Chunk is a terminal chunk
          */
         default boolean isTerminal()
         {
-            return this instanceof Error || isLast() && !hasRemaining();
+            return isLast() && !hasRemaining();
         }
 
         /**
          * <p>A chunk that wraps a failure.</p>
-         * <p>Error Chunks are always last and have no bytes to read.</p>
+         * <p>Error Chunks are always last and have no bytes to read,
+         * as such they are <em>terminal</em> Chunks.</p>
          *
          * @see #from(Throwable)
          */

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ByteBufferContentSource.java

@@ -78,7 +78,8 @@ public Content.Chunk read()
             if (last)
                 terminated = Content.Chunk.EOF;
         }
-        return Content.Chunk.from(buffer, last);
+        boolean terminal = !buffer.hasRemaining() && last;
+        return terminal ? Content.Chunk.EOF : Content.Chunk.from(buffer, last);
     }
 
     @Override

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ByteBufferChunk.java

@@ -24,15 +24,15 @@
 
 public abstract class ByteBufferChunk implements Content.Chunk
 {
-    public static final ByteBufferChunk EMPTY = new ByteBufferChunk(BufferUtil.EMPTY_BUFFER, false)
+    public static final ByteBufferChunk EMPTY = new ByteBufferChunk(true, BufferUtil.EMPTY_BUFFER, false)
     {
         @Override
         public String toString()
         {
             return "%s[EMPTY]".formatted(ByteBufferChunk.class.getSimpleName());
         }
     };
-    public static final ByteBufferChunk EOF = new ByteBufferChunk(BufferUtil.EMPTY_BUFFER, true)
+    public static final ByteBufferChunk EOF = new ByteBufferChunk(true, BufferUtil.EMPTY_BUFFER, true)
     {
         @Override
         public String toString()
@@ -46,6 +46,13 @@ public String toString()
 
     public ByteBufferChunk(ByteBuffer byteBuffer, boolean last)
     {
+        this(false, byteBuffer, last);
+    }
+
+    private ByteBufferChunk(boolean isConstant, ByteBuffer byteBuffer, boolean last)
+    {
+        if (!isConstant && !byteBuffer.hasRemaining() && last)
+            throw new IllegalArgumentException("Empty ByteBuffer instances with last=true are forbidden, use the EOF constant instead.");
         this.byteBuffer = Objects.requireNonNull(byteBuffer);
         this.last = last;
     }