On tomcat FTBFS.
Hi,
I don't think the link you gave on commit [fe932dd39d] is the reason for
FTBFS. I tried building on a VM that matches the certificate date and it
was successful. I also tried disabling all ssl related tests and was fine.
While doing these all I found TestSendFile test is the culprit. In
CVE-2017-5647 security patch a good amount of changes is applied for
SendFile*.java and *Nio2*.java. These are mostly about conditions on how
long the socket of sendfile keep active and to take away from it. But I
couldn't see any those change in its test file. Please take a look on
the attached patch. :)
--abhijith
From: Markus Koschany <apo@debian.org>
Date: Sat, 15 Apr 2017 17:25:03 +0200
Subject: CVE-2017-5647
Bug-Debian: https://bugs.debian.org/860068
Origin: http://svn.apache.org/r1788999
---
java/org/apache/coyote/AbstractProtocol.java | 7 +-
.../apache/coyote/http11/Http11AprProcessor.java | 38 +++++++----
.../apache/coyote/http11/Http11Nio2Processor.java | 11 +++-
.../apache/coyote/http11/Http11NioProcessor.java | 26 ++++++--
java/org/apache/tomcat/util/net/AprEndpoint.java | 49 +++++++++-----
java/org/apache/tomcat/util/net/Nio2Endpoint.java | 25 ++++---
java/org/apache/tomcat/util/net/NioEndpoint.java | 76 ++++++++++++----------
.../tomcat/util/net/SendfileKeepAliveState.java | 39 +++++++++++
java/org/apache/tomcat/util/net/SendfileState.java | 37 +++++++++++
9 files changed, 222 insertions(+), 86 deletions(-)
create mode 100644 java/org/apache/tomcat/util/net/SendfileKeepAliveState.java
create mode 100644 java/org/apache/tomcat/util/net/SendfileState.java
diff --git a/java/org/apache/coyote/AbstractProtocol.java b/java/org/apache/coyote/AbstractProtocol.java
index 9886cef..cabfbf6 100644
--- a/java/org/apache/coyote/AbstractProtocol.java
+++ b/java/org/apache/coyote/AbstractProtocol.java
@@ -710,10 +710,9 @@ public abstract class AbstractProtocol<S> implements ProtocolHandler,
release(wrapper, processor, false, true);
} else if (state == SocketState.SENDFILE) {
// Sendfile in progress. If it fails, the socket will be
- // closed. If it works, the socket will be re-added to the
- // poller
- connections.remove(socket);
- release(wrapper, processor, false, false);
+ // closed. If it works, the socket either be added to the
+ // poller (or equivalent) to await more data or processed
+ // if there are any pipe-lined requests remaining.
} else if (state == SocketState.UPGRADED) {
// Don't add sockets back to the poller if this was a
// non-blocking write otherwise the poller may trigger
diff --git a/java/org/apache/coyote/http11/Http11AprProcessor.java b/java/org/apache/coyote/http11/Http11AprProcessor.java
index e4ecd1a..a08da6f 100644
--- a/java/org/apache/coyote/http11/Http11AprProcessor.java
+++ b/java/org/apache/coyote/http11/Http11AprProcessor.java
@@ -37,6 +37,7 @@ import org.apache.tomcat.util.ExceptionUtils;
import org.apache.tomcat.util.net.AbstractEndpoint.Handler.SocketState;
import org.apache.tomcat.util.net.AprEndpoint;
import org.apache.tomcat.util.net.SSLSupport;
+import org.apache.tomcat.util.net.SendfileKeepAliveState;
import org.apache.tomcat.util.net.SocketStatus;
import org.apache.tomcat.util.net.SocketWrapper;
@@ -197,22 +198,31 @@ public class Http11AprProcessor extends AbstractHttp11Processor<Long> {
// Do sendfile as needed: add socket to sendfile and end
if (sendfileData != null && !getErrorState().isError()) {
sendfileData.socket = socketWrapper.getSocket().longValue();
- sendfileData.keepAlive = keepAlive;
- if (!((AprEndpoint)endpoint).getSendfile().add(sendfileData)) {
- // Didn't send all of the data to sendfile.
- if (sendfileData.socket == 0) {
- // The socket is no longer set. Something went wrong.
- // Close the connection. Too late to set status code.
- if (log.isDebugEnabled()) {
- log.debug(sm.getString(
- "http11processor.sendfile.error"));
- }
- setErrorState(ErrorState.CLOSE_NOW, null);
+ if (keepAlive) {
+ if (getInputBuffer().available() == 0) {
+ sendfileData.keepAliveState = SendfileKeepAliveState.OPEN;
} else {
- // The sendfile Poller will add the socket to the main
- // Poller once sendfile processing is complete
- sendfileInProgress = true;
+ sendfileData.keepAliveState = SendfileKeepAliveState.PIPELINED;
+ }
+ } else {
+ sendfileData.keepAliveState = SendfileKeepAliveState.NONE;
+ }
+ switch (((AprEndpoint)endpoint).getSendfile().add(sendfileData)) {
+ case DONE:
+ return false;
+ case PENDING:
+ // The sendfile Poller will add the socket to the main
+ // Poller once sendfile processing is complete
+ sendfileInProgress = true;
+ return true;
+ case ERROR:
+ // Something went wrong.
+ // Close the connection. Too late to set status code.
+ if (log.isDebugEnabled()) {
+ log.debug(sm.getString(
+ "http11processor.sendfile.error"));
}
+ setErrorState(ErrorState.CLOSE_NOW, null);
return true;
}
}
diff --git a/java/org/apache/coyote/http11/Http11Nio2Processor.java b/java/org/apache/coyote/http11/Http11Nio2Processor.java
index 6abd200..d13931a 100644
--- a/java/org/apache/coyote/http11/Http11Nio2Processor.java
+++ b/java/org/apache/coyote/http11/Http11Nio2Processor.java
@@ -35,6 +35,7 @@ import org.apache.tomcat.util.net.Nio2Channel;
import org.apache.tomcat.util.net.Nio2Endpoint;
import org.apache.tomcat.util.net.SSLSupport;
import org.apache.tomcat.util.net.SecureNio2Channel;
+import org.apache.tomcat.util.net.SendfileKeepAliveState;
import org.apache.tomcat.util.net.SocketStatus;
import org.apache.tomcat.util.net.SocketWrapper;
@@ -279,7 +280,15 @@ public class Http11Nio2Processor extends AbstractHttp11Processor<Nio2Channel> {
// Do sendfile as needed: add socket to sendfile and end
if (sendfileData != null && !getErrorState().isError()) {
((Nio2Endpoint.Nio2SocketWrapper) socketWrapper).setSendfileData(sendfileData);
- sendfileData.keepAlive = keepAlive;
+ if (keepAlive) {
+ if (getInputBuffer().available() == 0) {
+ sendfileData.keepAliveState = SendfileKeepAliveState.OPEN;
+ } else {
+ sendfileData.keepAliveState = SendfileKeepAliveState.PIPELINED;
+ }
+ } else {
+ sendfileData.keepAliveState = SendfileKeepAliveState.NONE;
+ }
switch (((Nio2Endpoint) endpoint)
.processSendfile((Nio2Endpoint.Nio2SocketWrapper) socketWrapper)) {
case DONE:
diff --git a/java/org/apache/coyote/http11/Http11NioProcessor.java b/java/org/apache/coyote/http11/Http11NioProcessor.java
index 30aa9e9..983bd06 100644
--- a/java/org/apache/coyote/http11/Http11NioProcessor.java
+++ b/java/org/apache/coyote/http11/Http11NioProcessor.java
@@ -36,6 +36,7 @@ import org.apache.tomcat.util.net.NioEndpoint;
import org.apache.tomcat.util.net.NioEndpoint.KeyAttachment;
import org.apache.tomcat.util.net.SSLSupport;
import org.apache.tomcat.util.net.SecureNioChannel;
+import org.apache.tomcat.util.net.SendfileKeepAliveState;
import org.apache.tomcat.util.net.SocketStatus;
import org.apache.tomcat.util.net.SocketWrapper;
@@ -270,28 +271,41 @@ public class Http11NioProcessor extends AbstractHttp11Processor<NioChannel> {
}
}
-
@Override
protected boolean breakKeepAliveLoop(SocketWrapper<NioChannel> socketWrapper) {
openSocket = keepAlive;
// Do sendfile as needed: add socket to sendfile and end
if (sendfileData != null && !getErrorState().isError()) {
((KeyAttachment) socketWrapper).setSendfileData(sendfileData);
- sendfileData.keepAlive = keepAlive;
+ if (keepAlive) {
+ if (getInputBuffer().available() == 0) {
+ sendfileData.keepAliveState = SendfileKeepAliveState.OPEN;
+ } else {
+ sendfileData.keepAliveState = SendfileKeepAliveState.PIPELINED;
+ }
+ } else {
+ sendfileData.keepAliveState = SendfileKeepAliveState.NONE;
+ }
SelectionKey key = socketWrapper.getSocket().getIOChannel().keyFor(
socketWrapper.getSocket().getPoller().getSelector());
//do the first write on this thread, might as well
- if (socketWrapper.getSocket().getPoller().processSendfile(key,
- (KeyAttachment) socketWrapper, true)) {
+ switch (socketWrapper.getSocket().getPoller().processSendfile(
+ key, (KeyAttachment) socketWrapper, true)) {
+ case DONE:
+ // If sendfile is complete, no need to break keep-alive loop
+ sendfileData = null;
+ return false;
+ case PENDING:
sendfileInProgress = true;
- } else {
+ return true;
+ case ERROR:
// Write failed
if (log.isDebugEnabled()) {
log.debug(sm.getString("http11processor.sendfile.error"));
}
setErrorState(ErrorState.CLOSE_NOW, null);
+ return true;
}
- return true;
}
return false;
}
diff --git a/java/org/apache/tomcat/util/net/AprEndpoint.java b/java/org/apache/tomcat/util/net/AprEndpoint.java
index 7db7214..66c876c 100644
--- a/java/org/apache/tomcat/util/net/AprEndpoint.java
+++ b/java/org/apache/tomcat/util/net/AprEndpoint.java
@@ -1973,7 +1973,7 @@ public class AprEndpoint extends AbstractEndpoint<Long> {
// Position
public long pos;
// KeepAlive flag
- public boolean keepAlive;
+ public SendfileKeepAliveState keepAliveState = SendfileKeepAliveState.NONE;
}
@@ -2061,7 +2061,7 @@ public class AprEndpoint extends AbstractEndpoint<Long> {
* @return true if all the data has been sent right away, and false
* otherwise
*/
- public boolean add(SendfileData data) {
+ public SendfileState add(SendfileData data) {
// Initialize fd from data given
try {
data.fdpool = Socket.pool(data.socket);
@@ -2079,7 +2079,7 @@ public class AprEndpoint extends AbstractEndpoint<Long> {
if (!(-nw == Status.EAGAIN)) {
Pool.destroy(data.fdpool);
data.socket = 0;
- return false;
+ return SendfileState.ERROR;
} else {
// Break the loop and add the socket to poller.
break;
@@ -2092,13 +2092,13 @@ public class AprEndpoint extends AbstractEndpoint<Long> {
// Set back socket to blocking mode
Socket.timeoutSet(
data.socket, getSoTimeout() * 1000);
- return true;
+ return SendfileState.DONE;
}
}
}
} catch (Exception e) {
log.warn(sm.getString("endpoint.sendfile.error"), e);
- return false;
+ return SendfileState.ERROR;
}
// Add socket to the list. Newly added sockets will wait
// at most for pollTime before being polled
@@ -2106,7 +2106,7 @@ public class AprEndpoint extends AbstractEndpoint<Long> {
addS.add(data);
this.notify();
}
- return false;
+ return SendfileState.PENDING;
}
/**
@@ -2216,20 +2216,33 @@ public class AprEndpoint extends AbstractEndpoint<Long> {
state.pos = state.pos + nw;
if (state.pos >= state.end) {
remove(state);
- if (state.keepAlive) {
- // Destroy file descriptor pool, which should close the file
- Pool.destroy(state.fdpool);
- Socket.timeoutSet(state.socket,
- getSoTimeout() * 1000);
- // If all done put the socket back in the
- // poller for processing of further requests
- getPoller().add(
- state.socket, getKeepAliveTimeout(),
- true, false);
- } else {
+ switch (state.keepAliveState) {
+ case NONE: {
// Close the socket since this is
- // the end of not keep-alive request.
+ // the end of the not keep-alive request.
closeSocket(state.socket);
+ break;
+ }
+ case PIPELINED: {
+ // Destroy file descriptor pool, which should close the file
+ Pool.destroy(state.fdpool);
+ Socket.timeoutSet(state.socket, getSoTimeout() * 1000);
+ // Process the pipelined request data
+ if (!processSocket(state.socket, SocketStatus.OPEN_READ)) {
+ closeSocket(state.socket);
+ }
+ break;
+ }
+ case OPEN: {
+ // Destroy file descriptor pool, which should close the file
+ Pool.destroy(state.fdpool);
+ Socket.timeoutSet(state.socket, getSoTimeout() * 1000);
+ // Put the socket back in the poller for
+ // processing of further requests
+ getPoller().add(state.socket,
+ getKeepAliveTimeout(),true,false);
+ break;
+ }
}
}
}
diff --git a/java/org/apache/tomcat/util/net/Nio2Endpoint.java b/java/org/apache/tomcat/util/net/Nio2Endpoint.java
index 0f9a023..389c615 100644
--- a/java/org/apache/tomcat/util/net/Nio2Endpoint.java
+++ b/java/org/apache/tomcat/util/net/Nio2Endpoint.java
@@ -909,17 +909,22 @@ public class Nio2Endpoint extends AbstractEndpoint<Nio2Channel> {
} catch (IOException e) {
// Ignore
}
- if (attachment.keepAlive) {
- if (!isInline()) {
- awaitBytes(attachment.socket);
- } else {
- attachment.doneInline = true;
- }
+ if (isInline()) {
+ attachment.doneInline = true;
} else {
- if (!isInline()) {
+ switch (attachment.keepAliveState) {
+ case NONE: {
processSocket(attachment.socket, SocketStatus.DISCONNECT, false);
- } else {
- attachment.doneInline = true;
+ break;
+ }
+ case PIPELINED: {
+ processSocket(attachment.socket, SocketStatus.OPEN_READ, true);
+ break;
+ }
+ case OPEN: {
+ awaitBytes(attachment.socket);
+ break;
+ }
}
}
return;
@@ -1159,7 +1164,7 @@ public class Nio2Endpoint extends AbstractEndpoint<Nio2Channel> {
public long pos;
public long length;
// KeepAlive flag
- public boolean keepAlive;
+ public SendfileKeepAliveState keepAliveState = SendfileKeepAliveState.NONE;
// Internal use only
private Nio2SocketWrapper socket;
private ByteBuffer buffer;
diff --git a/java/org/apache/tomcat/util/net/NioEndpoint.java b/java/org/apache/tomcat/util/net/NioEndpoint.java
index 36a1c53..96386a4 100644
--- a/java/org/apache/tomcat/util/net/NioEndpoint.java
+++ b/java/org/apache/tomcat/util/net/NioEndpoint.java
@@ -1166,7 +1166,9 @@ public class NioEndpoint extends AbstractEndpoint<NioChannel> {
return result;
}
- public boolean processSendfile(SelectionKey sk, KeyAttachment attachment, boolean event) {
+
+ public SendfileState processSendfile(SelectionKey sk, KeyAttachment attachment,
+ boolean calledByProcessor) {
NioChannel sc = null;
try {
unreg(sk, attachment, sk.readyOps());
@@ -1176,32 +1178,27 @@ public class NioEndpoint extends AbstractEndpoint<NioChannel> {
log.trace("Processing send file for: " + sd.fileName);
}
- //setup the file channel
- if ( sd.fchannel == null ) {
+ if (sd.fchannel == null) {
+ // Setup the file channel
File f = new File(sd.fileName);
- if ( !f.exists() ) {
- cancelledKey(sk,SocketStatus.ERROR);
- return false;
- }
@SuppressWarnings("resource") // Closed when channel is closed
FileInputStream fis = new FileInputStream(f);
sd.fchannel = fis.getChannel();
}
- //configure output channel
+ // Configure output channel
sc = attachment.getSocket();
- sc.setSendFile(true);
- //ssl channel is slightly different
+ // TLS/SSL channel is slightly different
WritableByteChannel wc = ((sc instanceof SecureNioChannel)?sc:sc.getIOChannel());
- //we still have data in the buffer
+ // We still have data in the buffer
if (sc.getOutboundRemaining()>0) {
if (sc.flushOutbound()) {
attachment.access();
}
} else {
long written = sd.fchannel.transferTo(sd.pos,sd.length,wc);
- if ( written > 0 ) {
+ if (written > 0) {
sd.pos += written;
sd.length -= written;
attachment.access();
@@ -1214,7 +1211,7 @@ public class NioEndpoint extends AbstractEndpoint<NioChannel> {
}
}
}
- if ( sd.length <= 0 && sc.getOutboundRemaining()<=0) {
+ if (sd.length <= 0 && sc.getOutboundRemaining()<=0) {
if (log.isDebugEnabled()) {
log.debug("Send file complete for: "+sd.fileName);
}
@@ -1223,48 +1220,61 @@ public class NioEndpoint extends AbstractEndpoint<NioChannel> {
sd.fchannel.close();
} catch (Exception ignore) {
}
- if ( sd.keepAlive ) {
+ // For calls from outside the Poller, the caller is
+ // responsible for registering the socket for the
+ // appropriate event(s) if sendfile completes.
+ if (!calledByProcessor) {
+ switch (sd.keepAliveState) {
+ case NONE: {
if (log.isDebugEnabled()) {
- log.debug("Connection is keep alive, registering back for OP_READ");
+ log.debug("Send file connection is being closed");
}
- if (event) {
- this.add(attachment.getSocket(),SelectionKey.OP_READ);
- } else {
- reg(sk,attachment,SelectionKey.OP_READ);
+ cancelledKey(sk,SocketStatus.STOP);
+ break;
+ }
+ case PIPELINED: {
+ if (log.isDebugEnabled()) {
+ log.debug("Connection is keep alive, processing pipe-lined data");
}
- } else {
- if (log.isDebugEnabled()) {
- log.debug("Send file connection is being closed");
+ if (!processSocket(attachment, SocketStatus.OPEN_READ, true)) {
+ cancelledKey(sk, SocketStatus.DISCONNECT);
+ }
+ break;
+ }
+ case OPEN: {
+ if (log.isDebugEnabled()) {
+ log.debug("Connection is keep alive, registering back for OP_READ");
+ }
+ reg(sk, attachment, SelectionKey.OP_READ);
+ break;
+ }
}
- cancelledKey(sk,SocketStatus.STOP);
- return false;
}
+ return SendfileState.DONE;
} else {
if (log.isDebugEnabled()) {
log.debug("OP_WRITE for sendfile: " + sd.fileName);
}
- if (event) {
+ if (calledByProcessor) {
add(attachment.getSocket(),SelectionKey.OP_WRITE);
} else {
reg(sk,attachment,SelectionKey.OP_WRITE);
}
+ return SendfileState.PENDING;
}
}catch ( IOException x ) {
if ( log.isDebugEnabled() ) log.debug("Unable to complete sendfile request:", x);
- if (!event) {
+ if (!calledByProcessor) {
cancelledKey(sk,SocketStatus.ERROR);
}
- return false;
+ return SendfileState.ERROR;
}catch ( Throwable t ) {
log.error("",t);
- if (!event) {
+ if (!calledByProcessor) {
cancelledKey(sk, SocketStatus.ERROR);
}
- return false;
- }finally {
- if (sc!=null) sc.setSendFile(false);
+ return SendfileState.ERROR;
}
- return true;
}
protected void unreg(SelectionKey sk, KeyAttachment attachment, int readyOps) {
@@ -1653,6 +1663,6 @@ public class NioEndpoint extends AbstractEndpoint<NioChannel> {
public long pos;
public long length;
// KeepAlive flag
- public boolean keepAlive;
+ public SendfileKeepAliveState keepAliveState = SendfileKeepAliveState.NONE;
}
}
diff --git a/java/org/apache/tomcat/util/net/SendfileKeepAliveState.java b/java/org/apache/tomcat/util/net/SendfileKeepAliveState.java
new file mode 100644
index 0000000..b27a9f1
--- /dev/null
+++ b/java/org/apache/tomcat/util/net/SendfileKeepAliveState.java
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.tomcat.util.net;
+
+public enum SendfileKeepAliveState {
+
+ /**
+ * Keep-alive is not in use. The socket can be closed when the response has
+ * been written.
+ */
+ NONE,
+
+ /**
+ * Keep-alive is in use and there is pipelined data in the input buffer to
+ * be read as soon as the current response has been written.
+ */
+ PIPELINED,
+
+ /**
+ * Keep-alive is in use. The socket should be added to the poller (or
+ * equivalent) to await more data as soon as the current response has been
+ * written.
+ */
+ OPEN
+}
diff --git a/java/org/apache/tomcat/util/net/SendfileState.java b/java/org/apache/tomcat/util/net/SendfileState.java
new file mode 100644
index 0000000..b354e2f
--- /dev/null
+++ b/java/org/apache/tomcat/util/net/SendfileState.java
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.tomcat.util.net;
+
+public enum SendfileState {
+
+ /**
+ * The sending of the file has started but has not completed. Sendfile is
+ * still using the socket.
+ */
+ PENDING,
+
+ /**
+ * The file has been fully sent. Sendfile is no longer using the socket.
+ */
+ DONE,
+
+ /**
+ * Something went wrong. The file may or may not have been sent. The socket
+ * is in an unknown state.
+ */
+ ERROR
+}
Reply to: