[tomcat] 04/05: Fix BZ 64771. Support calls to SocketInputStream.isReady()

[markt]

This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch 8.5.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git

commit ea8ffba3fc4740201eb93321eb42bb684959cf34
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Mon Mar 22 21:56:44 2021 +0000

    Fix BZ 64771. Support calls to SocketInputStream.isReady()
    
    Prior to this fix, concurrent calls to isReady() could corrupt the input
    buffer. The fix is modelled on the similar changes made for
    SocketOutputStream.isReady()
---
 java/org/apache/coyote/Request.java | 60 ++++++++++++++++++++++++++++++++++---
 webapps/docs/changelog.xml          |  5 ++++
 2 files changed, 61 insertions(+), 4 deletions(-)

diff --git a/java/org/apache/coyote/Request.java 
b/java/org/apache/coyote/Request.java
index 8f8554e..9611cf8 100644
--- a/java/org/apache/coyote/Request.java
+++ b/java/org/apache/coyote/Request.java
@@ -166,7 +166,18 @@ public final class Request {
      */
     private Exception errorException = null;
 
+    /*
+     * State for non-blocking output is maintained here as it is the one point
+     * easily reachable from the CoyoteInputStream and the CoyoteAdapter which
+     * both need access to state.
+     */
     volatile ReadListener listener;
+    // Ensures listener is only fired after a call is isReady()
+    private boolean fireListener = false;
+    // Tracks read registration to prevent duplicate registrations
+    private boolean registeredForRead = false;
+    // Lock used to manage concurrent access to above flags
+    private final Object nonBlockingStateLock = new Object();
 
     public ReadListener getReadListener() {
         return listener;
@@ -201,6 +212,14 @@ public final class Request {
         // has been finished will register the socket for read interest and 
that
         // is not required.
         if (!isFinished() && isReady()) {
+            synchronized (nonBlockingStateLock) {
+                // Ensure we don't get multiple read registrations
+                registeredForRead = true;
+                // Need to set the fireListener flag otherwise when the
+                // container tries to trigger onDataAvailable, nothing will
+                // happen
+                fireListener = true;
+            }
             action(ActionCode.DISPATCH_READ, null);
             if (!ContainerThreadMarker.isContainerThread()) {
                 // Not on a container thread so need to execute the dispatch
@@ -210,13 +229,42 @@ public final class Request {
     }
 
     public boolean isReady() {
-        AtomicBoolean result = new AtomicBoolean();
-        action(ActionCode.NB_READ_INTEREST, result);
-        return result.get();
+        // Assume read is not possible
+        boolean ready = false;
+        synchronized (nonBlockingStateLock) {
+            if (registeredForRead) {
+                fireListener = true;
+                return false;
+            }
+            ready = checkRegisterForRead();
+            fireListener = !ready;
+        }
+        return ready;
+    }
+
+    private boolean checkRegisterForRead() {
+        AtomicBoolean ready = new AtomicBoolean(false);
+        synchronized (nonBlockingStateLock) {
+            if (!registeredForRead) {
+                action(ActionCode.NB_READ_INTEREST, ready);
+                registeredForRead = !ready.get();
+            }
+        }
+        return ready.get();
     }
 
     public void onDataAvailable() throws IOException {
-        listener.onDataAvailable();
+        boolean fire = false;
+        synchronized (nonBlockingStateLock) {
+            registeredForRead = false;
+            if (fireListener) {
+                fireListener = false;
+                fire = true;
+            }
+        }
+        if (fire) {
+            listener.onDataAvailable();
+        }
     }
 
 
@@ -743,6 +791,10 @@ public final class Request {
         attributes.clear();
 
         listener = null;
+        synchronized (nonBlockingStateLock) {
+            fireListener = false;
+            registeredForRead = false;
+        }
         allDataReadEventSent.set(false);
 
         startTime = -1;
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index cb7bc06..5493b1a6 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -136,6 +136,11 @@
         present. (markt)
       </scode>
       <fix>
+        <bug>64771</bug>: Prevent concurrent calls to
+        <code>ServletInputStream.isReady()</code> corrupting the input buffer.
+        (markt)
+      </fix>
+      <fix>
         <bug>65179</bug>: Ensure that the connection level flow control window
         from the client to the server is updated when handling DATA frames
         received for completed streams else the flow control window  may become

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org