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 <ma...@apache.org>
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: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
