Fix CachingHttpContentFactory$CachedHttpContent already released buffer (#12704)

#12681 atomically retain the buffer with a check that it still is in the cache, otherwise delegate to the wrapped content

Signed-off-by: Ludovic Orban <[email protected]>

Author: lorban

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

@@ -242,7 +242,6 @@ public HttpContent getContent(String path) throws IOException
         if (!isCacheable(httpContent))
             return httpContent;
 
-        // The re-mapping function may be run multiple times by compute.
         AtomicBoolean added = new AtomicBoolean();
         cachingHttpContent = _cache.computeIfAbsent(path, key ->
         {
@@ -366,19 +365,39 @@ public String getKey()
         @Override
         public void writeTo(Content.Sink sink, long offset, long length, Callback callback)
         {
+            boolean retained = false;
             try
             {
-                _buffer.retain();
-                sink.write(true, BufferUtil.slice(_buffer.getByteBuffer(), (int)offset, (int)length), Callback.from(_buffer::release, callback));
+                retained = tryRetain();
+                if (retained)
+                    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);
             }
             catch (Throwable x)
             {
-                // BufferUtil.slice() may fail if offset and/or length are out of bounds.
-                _buffer.release();
+                // BufferUtil.slice() may fail if offset and/or length are out of bounds,
+                // Math.toIntExact may fail too if offset or length are > Integer.MAX_VALUE.
+                if (retained)
+                    release();
                 callback.failed(x);
             }
         }
 
+        /**
+         * Atomically checks that this content still is in cache (so it hasn't been released yet and is still usable) and retain
+         * its internal buffer if it is.
+         * @return true if this content can be used and has been retained, false otherwise.
+         */
+        private boolean tryRetain()
+        {
+            return _cache.computeIfPresent(_cacheKey, (s, cachingHttpContent) ->
+            {
+                _buffer.retain();
+                return cachingHttpContent;
+            }) != null;
+        }
+
         @Override
         public void release()
         {

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

@@ -0,0 +1,101 @@
+//
+// ========================================================================
+// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others.
+//
+// This program and the accompanying materials are made available under the
+// terms of the Eclipse Public License v. 2.0 which is available at
+// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0
+// which is available at https://www.apache.org/licenses/LICENSE-2.0.
+//
+// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
+// ========================================================================
+//
+
+package org.eclipse.jetty.http.content;
+
+import java.io.ByteArrayOutputStream;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+
+import org.eclipse.jetty.http.MimeTypes;
+import org.eclipse.jetty.io.ArrayByteBufferPool;
+import org.eclipse.jetty.io.ByteBufferPool;
+import org.eclipse.jetty.io.Content;
+import org.eclipse.jetty.toolchain.test.jupiter.WorkDir;
+import org.eclipse.jetty.toolchain.test.jupiter.WorkDirExtension;
+import org.eclipse.jetty.util.Blocker;
+import org.eclipse.jetty.util.resource.ResourceFactory;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+
+@ExtendWith(WorkDirExtension.class)
+public class CachingHttpContentFactoryTest
+{
+    public WorkDir workDir;
+    private ArrayByteBufferPool.Tracking trackingPool;
+    private ByteBufferPool.Sized sizedPool;
+
+    @BeforeEach
+    public void setUp()
+    {
+        trackingPool = new ArrayByteBufferPool.Tracking();
+        sizedPool = new ByteBufferPool.Sized(trackingPool);
+    }
+
+    @AfterEach
+    public void tearDown()
+    {
+        assertThat("Leaks: " + trackingPool.dumpLeaks(), trackingPool.getLeaks().size(), is(0));
+    }
+
+    @Test
+    public void testWriteEvictedContent() throws Exception
+    {
+        Path file = Files.writeString(workDir.getEmptyPathDir().resolve("file.txt"), "0123456789abcdefghijABCDEFGHIJ");
+        ResourceHttpContentFactory resourceHttpContentFactory = new ResourceHttpContentFactory(ResourceFactory.root().newResource(file.getParent()), MimeTypes.DEFAULTS, sizedPool);
+        CachingHttpContentFactory cachingHttpContentFactory = new CachingHttpContentFactory(resourceHttpContentFactory, sizedPool);
+
+        HttpContent content = cachingHttpContentFactory.getContent("file.txt");
+
+        // Empty the cache so 'content' gets released.
+        cachingHttpContentFactory.flushCache();
+
+        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        try (Blocker.Callback cb = Blocker.callback())
+        {
+            content.writeTo(Content.Sink.from(baos), 0L, -1L, cb);
+            cb.block();
+        }
+        assertThat(baos.toString(StandardCharsets.UTF_8), is("0123456789abcdefghijABCDEFGHIJ"));
+    }
+
+    @Test
+    public void testEvictBetweenWriteToAndSinkWrite() throws Exception
+    {
+        Path file = Files.writeString(workDir.getEmptyPathDir().resolve("file.txt"), "0123456789abcdefghijABCDEFGHIJ");
+        ResourceHttpContentFactory resourceHttpContentFactory = new ResourceHttpContentFactory(ResourceFactory.root().newResource(file.getParent()), MimeTypes.DEFAULTS, sizedPool);
+        CachingHttpContentFactory cachingHttpContentFactory = new CachingHttpContentFactory(resourceHttpContentFactory, sizedPool);
+
+        HttpContent content = cachingHttpContentFactory.getContent("file.txt");
+
+        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        try (Blocker.Callback cb = Blocker.callback())
+        {
+            content.writeTo((last, byteBuffer, callback) ->
+            {
+                // Empty the cache so 'content' gets released.
+                cachingHttpContentFactory.flushCache();
+
+                Content.Sink.from(baos).write(last, byteBuffer, callback);
+            }, 0L, -1L, cb);
+            cb.block();
+        }
+        assertThat(baos.toString(StandardCharsets.UTF_8), is("0123456789abcdefghijABCDEFGHIJ"));
+    }
+}