[tomcat] branch 9.0.x updated: Fix bz 65137 Don't corrupt response on early termination

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

[tomcat] branch 9.0.x updated: Fix bz 65137 Don't corrupt response on early termination

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

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


The following commit(s) were added to refs/heads/9.0.x by this push:
     new e6efb7f  Fix bz 65137 Don't corrupt response on early termination
e6efb7f is described below

commit e6efb7ffe662c6cca4f3e6024d8065976e605f64
Author: Mark Thomas <[hidden email]>
AuthorDate: Fri Feb 19 15:33:56 2021 +0000

    Fix bz 65137 Don't corrupt response on early termination
   
    Ensure that a response is not corrupted as well as incomplete if the
    connection is closed before the response is fully written due to a write
    timeout.
   
    https://bz.apache.org/bugzilla/show_bug.cgi?id=65137
---
 java/org/apache/tomcat/util/net/AprEndpoint.java   | 22 +++++++++++++++++++++-
 .../tomcat/util/net/NioBlockingSelector.java       | 20 +++++++++++++++++++-
 .../apache/tomcat/util/net/NioSelectorPool.java    | 20 +++++++++++++++++++-
 .../apache/tomcat/util/net/SocketWrapperBase.java  |  2 ++
 webapps/docs/changelog.xml                         |  5 +++++
 5 files changed, 66 insertions(+), 3 deletions(-)

diff --git a/java/org/apache/tomcat/util/net/AprEndpoint.java b/java/org/apache/tomcat/util/net/AprEndpoint.java
index cb67e07..887a1d5 100644
--- a/java/org/apache/tomcat/util/net/AprEndpoint.java
+++ b/java/org/apache/tomcat/util/net/AprEndpoint.java
@@ -2290,6 +2290,25 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
 
 
         private void doWriteInternal(ByteBuffer from) throws IOException {
+
+            if (previousIOException != null) {
+                /*
+                 * Socket has previously seen an IOException on write.
+                 *
+                 * Blocking writes assume that buffer is always fully written so
+                 * there is no code checking for incomplete writes, retaining
+                 * the unwritten data and attempting to write it as part of a
+                 * subsequent write call.
+                 *
+                 * Because of the above, when an IOException is triggered we
+                 * need so skip subsequent attempts to write as otherwise it
+                 * will appear to the client as if some data was dropped just
+                 * before the connection is lost. It is better if the client
+                 * just sees the dropped connection.
+                 */
+                throw new IOException(previousIOException);
+            }
+
             int thisTime;
 
             do {
@@ -2326,8 +2345,9 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
                     // 10053 on Windows is connection aborted
                     throw new EOFException(sm.getString("socket.apr.clientAbort"));
                 } else if (thisTime < 0) {
-                    throw new IOException(sm.getString("socket.apr.write.error",
+                    previousIOException = new IOException(sm.getString("socket.apr.write.error",
                             Integer.valueOf(-thisTime), getSocket(), this));
+                    throw previousIOException;
                 }
             } while ((thisTime > 0 || getBlockingStatus()) && from.hasRemaining());
 
diff --git a/java/org/apache/tomcat/util/net/NioBlockingSelector.java b/java/org/apache/tomcat/util/net/NioBlockingSelector.java
index eb8d511..67d6a53 100644
--- a/java/org/apache/tomcat/util/net/NioBlockingSelector.java
+++ b/java/org/apache/tomcat/util/net/NioBlockingSelector.java
@@ -91,6 +91,23 @@ public class NioBlockingSelector {
             reference = new KeyReference();
         }
         NioSocketWrapper att = (NioSocketWrapper) key.attachment();
+        if (att.previousIOException != null) {
+            /*
+             * Socket has previously seen an IOException on write.
+             *
+             * Blocking writes assume that buffer is always fully written so
+             * there is no code checking for incomplete writes, retaining
+             * the unwritten data and attempting to write it as part of a
+             * subsequent write call.
+             *
+             * Because of the above, when an IOException is triggered we
+             * need so skip subsequent attempts to write as otherwise it
+             * will appear to the client as if some data was dropped just
+             * before the connection is lost. It is better if the client
+             * just sees the dropped connection.
+             */
+            throw new IOException(att.previousIOException);
+        }
         int written = 0;
         boolean timedout = false;
         int keycount = 1; //assume we can write
@@ -131,7 +148,8 @@ public class NioBlockingSelector {
                 }
             }
             if (timedout) {
-                throw new SocketTimeoutException();
+                att.previousIOException = new SocketTimeoutException();
+                throw att.previousIOException;
             }
         } finally {
             poller.remove(att, SelectionKey.OP_WRITE);
diff --git a/java/org/apache/tomcat/util/net/NioSelectorPool.java b/java/org/apache/tomcat/util/net/NioSelectorPool.java
index ea365d9..e3d88f6 100644
--- a/java/org/apache/tomcat/util/net/NioSelectorPool.java
+++ b/java/org/apache/tomcat/util/net/NioSelectorPool.java
@@ -151,6 +151,23 @@ public class NioSelectorPool {
         if (shared) {
             return blockingSelector.write(buf, socket, writeTimeout);
         }
+        if (socket.getSocketWrapper().previousIOException != null) {
+            /*
+             * Socket has previously seen an IOException on write.
+             *
+             * Blocking writes assume that buffer is always fully written so
+             * there is no code checking for incomplete writes, retaining
+             * the unwritten data and attempting to write it as part of a
+             * subsequent write call.
+             *
+             * Because of the above, when an IOException is triggered we
+             * need so skip subsequent attempts to write as otherwise it
+             * will appear to the client as if some data was dropped just
+             * before the connection is lost. It is better if the client
+             * just sees the dropped connection.
+             */
+            throw new IOException(socket.getSocketWrapper().previousIOException);
+        }
         SelectionKey key = null;
         int written = 0;
         boolean timedout = false;
@@ -191,7 +208,8 @@ public class NioSelectorPool {
                 }
             }
             if (timedout) {
-                throw new SocketTimeoutException();
+                socket.getSocketWrapper().previousIOException = new SocketTimeoutException();
+                throw socket.getSocketWrapper().previousIOException;
             }
         } finally {
             if (key != null) {
diff --git a/java/org/apache/tomcat/util/net/SocketWrapperBase.java b/java/org/apache/tomcat/util/net/SocketWrapperBase.java
index 7f8020c..07580f9 100644
--- a/java/org/apache/tomcat/util/net/SocketWrapperBase.java
+++ b/java/org/apache/tomcat/util/net/SocketWrapperBase.java
@@ -51,6 +51,8 @@ public abstract class SocketWrapperBase<E> {
     private volatile long readTimeout = -1;
     private volatile long writeTimeout = -1;
 
+    protected volatile IOException previousIOException = null;
+
     private volatile int keepAliveLeft = 100;
     private volatile boolean upgraded = false;
     private boolean secure = false;
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index c5dd99f..5f1ee2b 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -138,6 +138,11 @@
         entire request body and the server is ready the request body using
         non-blocking I/O. (markt)
       </fix>
+      <fix>
+        <bug>65137</bug>: Ensure that a response is not corrupted as well as
+        incomplete if the connection is closed before the response is fully
+        written due to a write timeout. (markt)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Web applications">


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]