[tomcat] branch 8.5.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 8.5.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 8.5.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git


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

commit af7b667a7b7e7f367325b95a36b3b4d6399a6d1c
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.
    Back-porting also trigger a fair amount of clean-up to align the 8.5.x
    code with the 9.0.x code
   
    https://bz.apache.org/bugzilla/show_bug.cgi?id=65137
---
 java/org/apache/tomcat/util/net/AprEndpoint.java   |  22 +++-
 .../tomcat/util/net/NioBlockingSelector.java       |  82 +++++++++----
 java/org/apache/tomcat/util/net/NioChannel.java    |   8 +-
 .../apache/tomcat/util/net/NioSelectorPool.java    | 131 ++++++++++++++-------
 .../apache/tomcat/util/net/SocketWrapperBase.java  |   2 +
 webapps/docs/changelog.xml                         |   5 +
 6 files changed, 180 insertions(+), 70 deletions(-)

diff --git a/java/org/apache/tomcat/util/net/AprEndpoint.java b/java/org/apache/tomcat/util/net/AprEndpoint.java
index b0aacf5..b03c797 100644
--- a/java/org/apache/tomcat/util/net/AprEndpoint.java
+++ b/java/org/apache/tomcat/util/net/AprEndpoint.java
@@ -2384,6 +2384,25 @@ public class AprEndpoint extends AbstractEndpoint<Long> implements SNICallBack {
 
 
         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 {
@@ -2420,8 +2439,9 @@ public class AprEndpoint extends AbstractEndpoint<Long> implements SNICallBack {
                     // 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 286c06f..5aaa90e 100644
--- a/java/org/apache/tomcat/util/net/NioBlockingSelector.java
+++ b/java/org/apache/tomcat/util/net/NioBlockingSelector.java
@@ -49,9 +49,6 @@ public class NioBlockingSelector {
     protected Selector sharedSelector;
 
     protected BlockPoller poller;
-    public NioBlockingSelector() {
-
-    }
 
     public void open(Selector selector) {
         sharedSelector = selector;
@@ -63,7 +60,7 @@ public class NioBlockingSelector {
     }
 
     public void close() {
-        if (poller!=null) {
+        if (poller != null) {
             poller.disable();
             poller.interrupt();
             poller = null;
@@ -74,10 +71,11 @@ public class NioBlockingSelector {
      * Performs a blocking write using the bytebuffer for data to be written
      * If the <code>selector</code> parameter is null, then it will perform a busy write that could
      * take up a lot of CPU cycles.
+     *
      * @param buf ByteBuffer - the buffer containing the data, we will write as long as <code>(buf.hasRemaining()==true)</code>
      * @param socket SocketChannel - the socket to write data to
      * @param writeTimeout long - the timeout for this write operation in milliseconds, -1 means no timeout
-     * @return int - returns the number of bytes written
+     * @return the number of bytes written
      * @throws EOFException if write returns -1
      * @throws SocketTimeoutException if the write times out
      * @throws IOException if an IO Exception occurs in the underlying socket logic
@@ -85,22 +83,42 @@ public class NioBlockingSelector {
     public int write(ByteBuffer buf, NioChannel socket, long writeTimeout)
             throws IOException {
         SelectionKey key = socket.getIOChannel().keyFor(socket.getPoller().getSelector());
-        if ( key == null ) throw new IOException("Key no longer registered");
+        if (key == null) {
+            throw new IOException("Key no longer registered");
+        }
         KeyReference reference = keyReferenceStack.pop();
         if (reference == null) {
             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
         long time = System.currentTimeMillis(); //start the timeout timer
         try {
-            while ( (!timedout) && buf.hasRemaining()) {
+            while (!timedout && buf.hasRemaining()) {
                 if (keycount > 0) { //only write if we were registered for a write
                     int cnt = socket.write(buf); //write the data
-                    if (cnt == -1)
+                    if (cnt == -1) {
                         throw new EOFException();
+                    }
                     written += cnt;
                     if (cnt > 0) {
                         time = System.currentTimeMillis(); //reset our timeout timer
@@ -108,8 +126,10 @@ public class NioBlockingSelector {
                     }
                 }
                 try {
-                    if ( att.getWriteLatch()==null || att.getWriteLatch().getCount()==0) att.startWriteLatch(1);
-                    poller.add(att,SelectionKey.OP_WRITE,reference);
+                    if (att.getWriteLatch() == null || att.getWriteLatch().getCount() == 0) {
+                        att.startWriteLatch(1);
+                    }
+                    poller.add(att, SelectionKey.OP_WRITE, reference);
                     if (writeTimeout < 0) {
                         att.awaitWriteLatch(Long.MAX_VALUE,TimeUnit.MILLISECONDS);
                     } else {
@@ -118,23 +138,26 @@ public class NioBlockingSelector {
                 } catch (InterruptedException ignore) {
                     // Ignore
                 }
-                if ( att.getWriteLatch()!=null && att.getWriteLatch().getCount()> 0) {
+                if (att.getWriteLatch() != null && att.getWriteLatch().getCount() > 0) {
                     //we got interrupted, but we haven't received notification from the poller.
                     keycount = 0;
-                }else {
+                } else {
                     //latch countdown has happened
                     keycount = 1;
                     att.resetWriteLatch();
                 }
 
-                if (writeTimeout > 0 && (keycount == 0))
+                if (writeTimeout > 0 && (keycount == 0)) {
                     timedout = (System.currentTimeMillis() - time) >= writeTimeout;
-            } //while
-            if (timedout)
-                throw new SocketTimeoutException();
+                }
+            }
+            if (timedout) {
+                att.previousIOException = new SocketTimeoutException();
+                throw att.previousIOException;
+            }
         } finally {
-            poller.remove(att,SelectionKey.OP_WRITE);
-            if (timedout && reference.key!=null) {
+            poller.remove(att, SelectionKey.OP_WRITE);
+            if (timedout && reference.key != null) {
                 poller.cancelKey(reference.key);
             }
             reference.key = null;
@@ -147,17 +170,20 @@ public class NioBlockingSelector {
      * Performs a blocking read using the bytebuffer for data to be read
      * If the <code>selector</code> parameter is null, then it will perform a busy read that could
      * take up a lot of CPU cycles.
+     *
      * @param buf ByteBuffer - the buffer containing the data, we will read as until we have read at least one byte or we timed out
      * @param socket SocketChannel - the socket to write data to
      * @param readTimeout long - the timeout for this read operation in milliseconds, -1 means no timeout
-     * @return int - returns the number of bytes read
+     * @return the number of bytes read
      * @throws EOFException if read returns -1
      * @throws SocketTimeoutException if the read times out
      * @throws IOException if an IO Exception occurs in the underlying socket logic
      */
     public int read(ByteBuffer buf, NioChannel socket, long readTimeout) throws IOException {
         SelectionKey key = socket.getIOChannel().keyFor(socket.getPoller().getSelector());
-        if ( key == null ) throw new IOException("Key no longer registered");
+        if (key == null) {
+            throw new IOException("Key no longer registered");
+        }
         KeyReference reference = keyReferenceStack.pop();
         if (reference == null) {
             reference = new KeyReference();
@@ -168,7 +194,7 @@ public class NioBlockingSelector {
         int keycount = 1; //assume we can read
         long time = System.currentTimeMillis(); //start the timeout timer
         try {
-            while(!timedout) {
+            while (!timedout) {
                 if (keycount > 0) { //only read if we were registered for a read
                     read = socket.read(buf);
                     if (read != 0) {
@@ -176,7 +202,9 @@ public class NioBlockingSelector {
                     }
                 }
                 try {
-                    if ( att.getReadLatch()==null || att.getReadLatch().getCount()==0) att.startReadLatch(1);
+                    if (att.getReadLatch()==null || att.getReadLatch().getCount()==0) {
+                        att.startReadLatch(1);
+                    }
                     poller.add(att,SelectionKey.OP_READ, reference);
                     if (readTimeout < 0) {
                         att.awaitReadLatch(Long.MAX_VALUE, TimeUnit.MILLISECONDS);
@@ -194,14 +222,16 @@ public class NioBlockingSelector {
                     keycount = 1;
                     att.resetReadLatch();
                 }
-                if (readTimeout >= 0 && (keycount == 0))
+                if (readTimeout >= 0 && (keycount == 0)) {
                     timedout = (System.currentTimeMillis() - time) >= readTimeout;
-            } //while
-            if (timedout)
+                }
+            }
+            if (timedout) {
                 throw new SocketTimeoutException();
+            }
         } finally {
             poller.remove(att,SelectionKey.OP_READ);
-            if (timedout && reference.key!=null) {
+            if (timedout && reference.key != null) {
                 poller.cancelKey(reference.key);
             }
             reference.key = null;
diff --git a/java/org/apache/tomcat/util/net/NioChannel.java b/java/org/apache/tomcat/util/net/NioChannel.java
index adaa39f..14b364d 100644
--- a/java/org/apache/tomcat/util/net/NioChannel.java
+++ b/java/org/apache/tomcat/util/net/NioChannel.java
@@ -32,8 +32,6 @@ import org.apache.tomcat.util.res.StringManager;
  * Base class for a SocketChannel wrapper used by the endpoint.
  * This way, logic for an SSL socket channel remains the same as for
  * a non SSL, making sure we don't need to code for any exception cases.
- *
- * @version 1.0
  */
 public class NioChannel implements ByteChannel, ScatteringByteChannel, GatheringByteChannel {
 
@@ -62,6 +60,12 @@ public class NioChannel implements ByteChannel, ScatteringByteChannel, Gathering
         bufHandler.reset();
     }
 
+    /**
+     * @return the socketWrapper
+     */
+    SocketWrapperBase<NioChannel> getSocketWrapper() {
+        return socketWrapper;
+    }
 
     void setSocketWrapper(SocketWrapperBase<NioChannel> socketWrapper) {
         this.socketWrapper = socketWrapper;
diff --git a/java/org/apache/tomcat/util/net/NioSelectorPool.java b/java/org/apache/tomcat/util/net/NioSelectorPool.java
index aa282cf..73d84c1 100644
--- a/java/org/apache/tomcat/util/net/NioSelectorPool.java
+++ b/java/org/apache/tomcat/util/net/NioSelectorPool.java
@@ -56,8 +56,7 @@ public class NioSelectorPool {
     protected boolean enabled = true;
     protected AtomicInteger active = new AtomicInteger(0);
     protected AtomicInteger spare = new AtomicInteger(0);
-    protected ConcurrentLinkedQueue<Selector> selectors =
-            new ConcurrentLinkedQueue<>();
+    protected ConcurrentLinkedQueue<Selector> selectors = new ConcurrentLinkedQueue<>();
 
     protected Selector getSharedSelector() throws IOException {
         if (SHARED && SHARED_SELECTOR == null) {
@@ -72,28 +71,32 @@ public class NioSelectorPool {
     }
 
     public Selector get() throws IOException{
-        if ( SHARED ) {
+        if (SHARED) {
             return getSharedSelector();
         }
-        if ( (!enabled) || active.incrementAndGet() >= maxSelectors ) {
-            if ( enabled ) active.decrementAndGet();
+        if ((!enabled) || active.incrementAndGet() >= maxSelectors) {
+            if (enabled) {
+                active.decrementAndGet();
+            }
             return null;
         }
         Selector s = null;
         try {
-            s = selectors.size()>0?selectors.poll():null;
+            s = selectors.size() > 0 ? selectors.poll() : null;
             if (s == null) {
                 s = Selector.open();
+            } else {
+                spare.decrementAndGet();
             }
-            else spare.decrementAndGet();
-
-        }catch (NoSuchElementException x ) {
+        } catch (NoSuchElementException x) {
             try {
                 s = Selector.open();
             } catch (IOException iox) {
             }
         } finally {
-            if ( s == null ) active.decrementAndGet();//we were unable to find a selector
+            if (s == null) {
+                active.decrementAndGet();// we were unable to find a selector
+            }
         }
         return s;
     }
@@ -101,25 +104,32 @@ public class NioSelectorPool {
 
 
     public void put(Selector s) throws IOException {
-        if ( SHARED ) return;
-        if ( enabled ) active.decrementAndGet();
-        if ( enabled && (maxSpareSelectors==-1 || spare.get() < Math.min(maxSpareSelectors,maxSelectors)) ) {
+        if (SHARED) {
+            return;
+        }
+        if (enabled) {
+            active.decrementAndGet();
+        }
+        if (enabled && (maxSpareSelectors == -1 || spare.get() < Math.min(maxSpareSelectors, maxSelectors))) {
             spare.incrementAndGet();
             selectors.offer(s);
+        } else {
+            s.close();
         }
-        else s.close();
     }
 
     public void close() throws IOException {
         enabled = false;
         Selector s;
-        while ( (s = selectors.poll()) != null ) s.close();
+        while ((s = selectors.poll()) != null) {
+            s.close();
+        }
         spare.set(0);
         active.set(0);
-        if (blockingSelector!=null) {
+        if (blockingSelector != null) {
             blockingSelector.close();
         }
-        if ( SHARED && getSharedSelector()!=null ) {
+        if (SHARED && getSharedSelector() != null) {
             getSharedSelector().close();
             SHARED_SELECTOR = null;
         }
@@ -156,17 +166,36 @@ public class NioSelectorPool {
         if ( SHARED && block ) {
             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;
         int keycount = 1; //assume we can write
         long time = System.currentTimeMillis(); //start the timeout timer
         try {
-            while ( (!timedout) && buf.hasRemaining() ) {
+            while ((!timedout) && buf.hasRemaining()) {
                 int cnt = 0;
                 if ( keycount > 0 ) { //only write if we were registered for a write
                     cnt = socket.write(buf); //write the data
-                    if (cnt == -1) throw new EOFException();
+                    if (cnt == -1) {
+                        throw new EOFException();
+                    }
 
                     written += cnt;
                     if (cnt > 0) {
@@ -175,21 +204,29 @@ public class NioSelectorPool {
                     }
                     if (cnt==0 && (!block)) break; //don't block
                 }
-                if ( selector != null ) {
+                if (selector != null) {
                     //register OP_WRITE to the selector
-                    if (key==null) key = socket.getIOChannel().register(selector, SelectionKey.OP_WRITE);
-                    else key.interestOps(SelectionKey.OP_WRITE);
-                    if (writeTimeout==0) {
+                    if (key == null) {
+                        key = socket.getIOChannel().register(selector, SelectionKey.OP_WRITE);
+                    } else {
+                        key.interestOps(SelectionKey.OP_WRITE);
+                    }
+                    if (writeTimeout == 0) {
                         timedout = buf.hasRemaining();
-                    } else if (writeTimeout<0) {
+                    } else if (writeTimeout < 0) {
                         keycount = selector.select();
                     } else {
                         keycount = selector.select(writeTimeout);
                     }
                 }
-                if (writeTimeout > 0 && (selector == null || keycount == 0) ) timedout = (System.currentTimeMillis()-time)>=writeTimeout;
-            }//while
-            if ( timedout ) throw new SocketTimeoutException();
+                if (writeTimeout > 0 && (selector == null || keycount == 0)) {
+                    timedout = (System.currentTimeMillis() - time) >= writeTimeout;
+                }
+            }
+            if (timedout) {
+                socket.getSocketWrapper().previousIOException = new SocketTimeoutException();
+                throw socket.getSocketWrapper().previousIOException;
+            }
         } finally {
             if (key != null) {
                 key.cancel();
@@ -215,6 +252,7 @@ public class NioSelectorPool {
     public int read(ByteBuffer buf, NioChannel socket, Selector selector, long readTimeout) throws IOException {
         return read(buf,socket,selector,readTimeout,true);
     }
+
     /**
      * Performs a read using the bytebuffer for data to be read and a selector to register for events should
      * you have the block=true.
@@ -240,9 +278,9 @@ public class NioSelectorPool {
         int keycount = 1; //assume we can write
         long time = System.currentTimeMillis(); //start the timeout timer
         try {
-            while ( (!timedout) ) {
+            while (!timedout) {
                 int cnt = 0;
-                if ( keycount > 0 ) { //only read if we were registered for a read
+                if (keycount > 0) { //only read if we were registered for a read
                     cnt = socket.read(buf);
                     if (cnt == -1) {
                         if (read == 0) {
@@ -252,27 +290,38 @@ public class NioSelectorPool {
                     }
                     read += cnt;
                     if (cnt > 0) continue; //read some more
-                    if (cnt==0 && (read>0 || (!block) ) ) break; //we are done reading
+                    if (cnt == 0 && (read > 0 || (!block))) {
+                        break; //we are done reading
+                    }
                 }
-                if ( selector != null ) {//perform a blocking read
+                if (selector != null) {//perform a blocking read
                     //register OP_WRITE to the selector
-                    if (key==null) key = socket.getIOChannel().register(selector, SelectionKey.OP_READ);
-                    else key.interestOps(SelectionKey.OP_READ);
-                    if (readTimeout==0) {
-                        timedout = (read==0);
-                    } else if (readTimeout<0) {
+                    if (key == null) {
+                        key = socket.getIOChannel().register(selector, SelectionKey.OP_READ);
+                    } else {
+                        key.interestOps(SelectionKey.OP_READ);
+                    }
+                    if (readTimeout == 0) {
+                        timedout = (read == 0);
+                    } else if (readTimeout < 0) {
                         keycount = selector.select();
                     } else {
                         keycount = selector.select(readTimeout);
                     }
                 }
-                if (readTimeout > 0 && (selector == null || keycount == 0) ) timedout = (System.currentTimeMillis()-time)>=readTimeout;
-            }//while
-            if ( timedout ) throw new SocketTimeoutException();
+                if (readTimeout > 0 && (selector == null || keycount == 0)) {
+                    timedout = (System.currentTimeMillis() - time) >= readTimeout;
+                }
+            }
+            if (timedout) {
+                throw new SocketTimeoutException();
+            }
         } finally {
             if (key != null) {
                 key.cancel();
-                if (selector != null) selector.selectNow();//removes the key from this selector
+                if (selector != null) {
+                    selector.selectNow();//removes the key from this selector
+                }
             }
         }
         return read;
@@ -317,4 +366,4 @@ public class NioSelectorPool {
     public AtomicInteger getSpare() {
         return spare;
     }
-}
\ No newline at end of file
+}
diff --git a/java/org/apache/tomcat/util/net/SocketWrapperBase.java b/java/org/apache/tomcat/util/net/SocketWrapperBase.java
index dd1da55..17a0b4c 100644
--- a/java/org/apache/tomcat/util/net/SocketWrapperBase.java
+++ b/java/org/apache/tomcat/util/net/SocketWrapperBase.java
@@ -49,6 +49,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 764f813..9f32463 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -131,6 +131,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]