From 170e0f792bd18ff031677890ba2fe50eb7a376c1 Mon Sep 17 00:00:00 2001
From: Mark Thomas <markt@apache.org>
Date: Tue, 29 Mar 2022 19:15:37 +0100
Subject: [PATCH] Improve the recycling of Processor objects to make it more
 robust.

---
 java/org/apache/coyote/AbstractProtocol.java  | 32 ++++++++++---------
 .../tomcat/util/net/SocketWrapperBase.java    | 17 +++++++---
 webapps/docs/changelog.xml                    |  4 +++
 3 files changed, 33 insertions(+), 20 deletions(-)

Index: apache-tomcat-9.0.43-src/java/org/apache/coyote/AbstractProtocol.java
===================================================================
--- apache-tomcat-9.0.43-src.orig/java/org/apache/coyote/AbstractProtocol.java
+++ apache-tomcat-9.0.43-src/java/org/apache/coyote/AbstractProtocol.java
@@ -794,7 +794,11 @@ public abstract class AbstractProtocol<S
 
             S socket = wrapper.getSocket();
 
-            Processor processor = (Processor) wrapper.getCurrentProcessor();
+            // We take complete ownership of the Processor inside of this method to ensure
+            // no other thread can release it while we're using it. Whatever processor is
+            // held by this variable will be associated with the SocketWrapper before this
+            // method returns.
+            Processor processor = (Processor) wrapper.takeCurrentProcessor();
             if (getLog().isDebugEnabled()) {
                 getLog().debug(sm.getString("abstractConnectionHandler.connectionsGet",
                         processor, socket));
@@ -879,9 +883,6 @@ public abstract class AbstractProtocol<S
                 processor.setSslSupport(
                         wrapper.getSslSupport(getProtocol().getClientCertProvider()));
 
-                // Associate the processor with the connection
-                wrapper.setCurrentProcessor(processor);
-
                 SocketState state = SocketState.CLOSED;
                 do {
                     state = processor.process(wrapper, status);
@@ -901,8 +902,6 @@ public abstract class AbstractProtocol<S
                                 release(processor);
                                 // Create the upgrade processor
                                 processor = upgradeProtocol.getProcessor(wrapper, getProtocol().getAdapter());
-                                // Associate with the processor with the connection
-                                wrapper.setCurrentProcessor(processor);
                             } else {
                                 if (getLog().isDebugEnabled()) {
                                     getLog().debug(sm.getString(
@@ -922,8 +921,6 @@ public abstract class AbstractProtocol<S
                                 getLog().debug(sm.getString("abstractConnectionHandler.upgradeCreate",
                                         processor, wrapper));
                             }
-                            // Associate with the processor with the connection
-                            wrapper.setCurrentProcessor(processor);
                             // Initialise the upgrade handler (which may trigger
                             // some IO using the new protocol which is why the lines
                             // above are necessary)
@@ -961,8 +958,8 @@ public abstract class AbstractProtocol<S
                 } else if (state == SocketState.OPEN) {
                     // In keep-alive but between requests. OK to recycle
                     // processor. Continue to poll for the next request.
-                    wrapper.setCurrentProcessor(null);
                     release(processor);
+                    processor = null;
                     wrapper.registerReadInterest();
                 } else if (state == SocketState.SENDFILE) {
                     // Sendfile in progress. If it fails, the socket will be
@@ -987,8 +984,7 @@ public abstract class AbstractProtocol<S
                     // Connection closed. OK to recycle the processor.
                     // Processors handling upgrades require additional clean-up
                     // before release.
-                    wrapper.setCurrentProcessor(null);
-                    if (processor.isUpgrade()) {
+                    if (processor != null && processor.isUpgrade()) {
                         UpgradeToken upgradeToken = processor.getUpgradeToken();
                         HttpUpgradeHandler httpUpgradeHandler = upgradeToken.getHttpUpgradeHandler();
                         InstanceManager instanceManager = upgradeToken.getInstanceManager();
@@ -1009,7 +1005,13 @@ public abstract class AbstractProtocol<S
                             }
                         }
                     }
+
                     release(processor);
+                    processor = null;
+                }
+
+                if (processor != null) {
+                    wrapper.setCurrentProcessor(processor);
                 }
                 return state;
             } catch(java.net.SocketException e) {
@@ -1047,7 +1049,6 @@ public abstract class AbstractProtocol<S
 
             // Make sure socket/processor is removed from the list of current
             // connections
-            wrapper.setCurrentProcessor(null);
             release(processor);
             return SocketState.CLOSED;
         }
@@ -1081,7 +1082,9 @@ public abstract class AbstractProtocol<S
 
         /**
          * Expected to be used by the handler once the processor is no longer
-         * required.
+         * required. Care must be taken to ensure that this method is only
+         * called once per processor, after the request processing has
+         * completed.
          *
          * @param processor Processor being released (that was associated with
          *                  the socket)
@@ -1119,8 +1122,7 @@ public abstract class AbstractProtocol<S
          */
         @Override
         public void release(SocketWrapperBase<S> socketWrapper) {
-            Processor processor = (Processor) socketWrapper.getCurrentProcessor();
-            socketWrapper.setCurrentProcessor(null);
+            Processor processor = (Processor) socketWrapper.takeCurrentProcessor();
             release(processor);
         }
 
Index: apache-tomcat-9.0.43-src/java/org/apache/tomcat/util/net/SocketWrapperBase.java
===================================================================
--- apache-tomcat-9.0.43-src.orig/java/org/apache/tomcat/util/net/SocketWrapperBase.java
+++ apache-tomcat-9.0.43-src/java/org/apache/tomcat/util/net/SocketWrapperBase.java
@@ -29,6 +29,7 @@ import java.util.concurrent.RejectedExec
 import java.util.concurrent.Semaphore;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
 
 import org.apache.juli.logging.Log;
 import org.apache.juli.logging.LogFactory;
@@ -104,10 +105,12 @@ public abstract class SocketWrapperBase<
     protected volatile OperationState<?> writeOperation = null;
 
     /**
-     * The org.apache.coyote.Processor instance currently associated
-     * with the wrapper.
+     * The org.apache.coyote.Processor instance currently associated with the
+     * wrapper. Only populated when required to maintain wrapper<->Processor
+     * mapping between calls to
+     * {@link AbstractEndpoint.Handler#process(SocketWrapperBase, SocketEvent)}.
      */
-    protected Object currentProcessor = null;
+    private final AtomicReference<Object> currentProcessor = new AtomicReference<>();
 
     public SocketWrapperBase(E socket, AbstractEndpoint<E,?> endpoint) {
         this.socket = socket;
@@ -134,11 +137,15 @@ public abstract class SocketWrapperBase<
     }
 
     public Object getCurrentProcessor() {
-        return currentProcessor;
+        return currentProcessor.get();
     }
 
     public void setCurrentProcessor(Object currentProcessor) {
-        this.currentProcessor = currentProcessor;
+        this.currentProcessor.set(currentProcessor);
+    }
+
+    public Object takeCurrentProcessor() {
+        return currentProcessor.getAndSet(null);
     }
 
     /**
Index: apache-tomcat-9.0.43-src/webapps/docs/changelog.xml
===================================================================
--- apache-tomcat-9.0.43-src.orig/webapps/docs/changelog.xml
+++ apache-tomcat-9.0.43-src/webapps/docs/changelog.xml
@@ -485,6 +485,10 @@
         Fix a concurrency issue in the NIO connector that could cause newly
         created connections to be removed from the poller. (markt)
       </fix>
+      <fix>
+        Improve the recycling of Processor objects to make it more robust.
+        (markt)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Jasper">