You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2021/11/18 09:18:00 UTC
[tomcat] branch 9.0.x updated: Protect against a known OS bug
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 bbbb7a8 Protect against a known OS bug
bbbb7a8 is described below
commit bbbb7a8c4315bbd5f6b3d620f748cf30a41ba2e9
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Wed Nov 17 18:48:33 2021 +0000
Protect against a known OS bug
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1924298
---
java/org/apache/tomcat/util/net/AprEndpoint.java | 15 +++++++++++++++
java/org/apache/tomcat/util/net/LocalStrings.properties | 1 +
java/org/apache/tomcat/util/net/Nio2Endpoint.java | 15 ++++++++++++---
java/org/apache/tomcat/util/net/NioEndpoint.java | 11 ++++++++++-
webapps/docs/changelog.xml | 6 ++++++
5 files changed, 44 insertions(+), 4 deletions(-)
diff --git a/java/org/apache/tomcat/util/net/AprEndpoint.java b/java/org/apache/tomcat/util/net/AprEndpoint.java
index 48c6716..dde0e0c 100644
--- a/java/org/apache/tomcat/util/net/AprEndpoint.java
+++ b/java/org/apache/tomcat/util/net/AprEndpoint.java
@@ -112,6 +112,9 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
protected long sslContext = 0;
+ private int previousAcceptedPort = -1;
+ private String previousAcceptedAddress = null;
+
// ------------------------------------------------------------ Constructor
public AprEndpoint() {
@@ -790,7 +793,18 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
if (log.isDebugEnabled()) {
log.debug(sm.getString("endpoint.debug.socket", socket));
}
+
+ // Do the duplicate accept check here rather than in serverSocketaccept()
+ // so we can cache the results in the SocketWrapper
AprSocketWrapper wrapper = new AprSocketWrapper(socket, this);
+ if (wrapper.getRemotePort() == previousAcceptedPort) {
+ if (wrapper.getRemoteAddr().equals(previousAcceptedAddress)) {
+ throw new IOException(sm.getString("endpoint.err.duplicateAccept"));
+ }
+ }
+ previousAcceptedPort = wrapper.getRemotePort();
+ previousAcceptedAddress = wrapper.getRemoteAddr();
+
connections.put(socket, wrapper);
wrapper.setKeepAliveLeft(getMaxKeepAliveRequests());
wrapper.setReadTimeout(getConnectionTimeout());
@@ -811,6 +825,7 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
@Override
protected Long serverSocketAccept() throws Exception {
+ // See setSocketOptions(Long) for duplicate accept check
long socket = Socket.accept(serverSock);
if (socket == 0) {
throw new IOException(sm.getString("endpoint.err.accept", getName()));
diff --git a/java/org/apache/tomcat/util/net/LocalStrings.properties b/java/org/apache/tomcat/util/net/LocalStrings.properties
index 3e306ec..0cc32e4 100644
--- a/java/org/apache/tomcat/util/net/LocalStrings.properties
+++ b/java/org/apache/tomcat/util/net/LocalStrings.properties
@@ -82,6 +82,7 @@ endpoint.duplicateSslHostName=Multiple SSLHostConfig elements were provided for
endpoint.err.accept=Failed to accept socket for end point [{0}]
endpoint.err.attach=Failed to attach SSLContext to socket - error [{0}]
endpoint.err.close=Caught exception trying to close socket
+endpoint.err.duplicateAccept=Duplicate accept detected. This is a known OS bug. Please consider reporting that you are affected: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1924298
endpoint.err.handshake=Handshake failed
endpoint.err.unexpected=Unexpected error processing socket
endpoint.executor.fail=Executor rejected socket [{0}] for processing
diff --git a/java/org/apache/tomcat/util/net/Nio2Endpoint.java b/java/org/apache/tomcat/util/net/Nio2Endpoint.java
index 365fe3a..125261a 100644
--- a/java/org/apache/tomcat/util/net/Nio2Endpoint.java
+++ b/java/org/apache/tomcat/util/net/Nio2Endpoint.java
@@ -84,8 +84,10 @@ public class Nio2Endpoint extends AbstractJsseEndpoint<Nio2Channel,AsynchronousS
*/
private SynchronizedStack<Nio2Channel> nioChannels;
- // ------------------------------------------------------------- Properties
+ private SocketAddress previousAcceptedSocketRemoteAddress = null;
+
+ // ------------------------------------------------------------- Properties
/**
* Is deferAccept supported?
@@ -99,7 +101,6 @@ public class Nio2Endpoint extends AbstractJsseEndpoint<Nio2Channel,AsynchronousS
// --------------------------------------------------------- Public Methods
-
/**
* Number of keep-alive sockets.
*
@@ -367,7 +368,15 @@ public class Nio2Endpoint extends AbstractJsseEndpoint<Nio2Channel,AsynchronousS
@Override
protected AsynchronousSocketChannel serverSocketAccept() throws Exception {
- return serverSock.accept().get();
+ AsynchronousSocketChannel result = serverSock.accept().get();
+
+ SocketAddress currentRemoteAddress = result.getRemoteAddress();
+ if (currentRemoteAddress.equals(previousAcceptedSocketRemoteAddress)) {
+ throw new IOException(sm.getString("endpoint.err.duplicateAccept"));
+ }
+ previousAcceptedSocketRemoteAddress = currentRemoteAddress;
+
+ return result;
}
diff --git a/java/org/apache/tomcat/util/net/NioEndpoint.java b/java/org/apache/tomcat/util/net/NioEndpoint.java
index da77764..9c8a713 100644
--- a/java/org/apache/tomcat/util/net/NioEndpoint.java
+++ b/java/org/apache/tomcat/util/net/NioEndpoint.java
@@ -108,6 +108,8 @@ public class NioEndpoint extends AbstractJsseEndpoint<NioChannel,SocketChannel>
*/
private SynchronizedStack<NioChannel> nioChannels;
+ private SocketAddress previousAcceptedSocketRemoteAddress = null;
+
// ------------------------------------------------------------- Properties
@@ -537,7 +539,14 @@ public class NioEndpoint extends AbstractJsseEndpoint<NioChannel,SocketChannel>
@Override
protected SocketChannel serverSocketAccept() throws Exception {
- return serverSock.accept();
+ SocketChannel result = serverSock.accept();
+ SocketAddress currentRemoteAddress = result.getRemoteAddress();
+ if (currentRemoteAddress.equals(previousAcceptedSocketRemoteAddress)) {
+ throw new IOException(sm.getString("endpoint.err.duplicateAccept"));
+ }
+ previousAcceptedSocketRemoteAddress = currentRemoteAddress;
+
+ return result;
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 43260c7..57f515a 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -115,6 +115,12 @@
Improve error handling if APR/Native fails to accept an incoming
connection. (markt)
</fix>
+ <add>
+ Provide protection against a known <a
+ href="https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1924298">OS
+ bug</a> that causes the acceptor to report an incoming connection more
+ than once. (markt)
+ </add>
</changelog>
</subsection>
</section>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch 9.0.x updated: Protect against a known OS bug
Posted by Rémy Maucherat <re...@apache.org>.
On Thu, Nov 18, 2021 at 10:53 AM Mark Thomas <ma...@apache.org> wrote:
>
> On 18/11/2021 09:18, markt@apache.org wrote:
> > 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 bbbb7a8 Protect against a known OS bug
> > bbbb7a8 is described below
> >
> > commit bbbb7a8c4315bbd5f6b3d620f748cf30a41ba2e9
> > Author: Mark Thomas <ma...@apache.org>
> > AuthorDate: Wed Nov 17 18:48:33 2021 +0000
> >
> > Protect against a known OS bug
> >
> > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1924298
>
> Hi all,
>
> Back-porting this to 8.5.x is complicated by 8.5.x having the option for
> multiple acceptor threads. I can see the following options.
>
> 1. Don't back-port this protection to 8.5.x.
> ===
> 8.5.x has survived this long without the protection.
> There aren't any outstanding 8.5.x bug reports that could be
> attributable to this issue.
> Having the protection (and log message) in the other branches should
> be enough to raise visibility of the bug and get it fixed.
> There may be uses that are experiencing this bug on rare occasions
> without realising it.
>
> 2. Limit 8.5.x to a single acceptor thread and back-port the 9.0.x fix
> ===
> Multiple acceptor threads have been shown to offer little/no
> performance benefit.
> Making the acceptorThreadCount setter a NO-OP and hard-coding the
> getter to return 1 would be relatively simple.
> This feels like a fairly big change this late in the 8.5.x series.
>
> 3. Introduce a synchronization block around the accept() call and add
> the duplicate check inside the synchronization block.
> ===
> There may be a performance impact for users who have configured
> multiple acceptor threads.
>
>
> My current preference order is 2 first, closely followed by 1 with 3
> further behind.
>
> Thoughts?
Ok for 2 if you feel like trying it.
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch 9.0.x updated: Protect against a known OS bug
Posted by Mark Thomas <ma...@apache.org>.
On 18/11/2021 09:18, markt@apache.org wrote:
> 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 bbbb7a8 Protect against a known OS bug
> bbbb7a8 is described below
>
> commit bbbb7a8c4315bbd5f6b3d620f748cf30a41ba2e9
> Author: Mark Thomas <ma...@apache.org>
> AuthorDate: Wed Nov 17 18:48:33 2021 +0000
>
> Protect against a known OS bug
>
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1924298
Hi all,
Back-porting this to 8.5.x is complicated by 8.5.x having the option for
multiple acceptor threads. I can see the following options.
1. Don't back-port this protection to 8.5.x.
===
8.5.x has survived this long without the protection.
There aren't any outstanding 8.5.x bug reports that could be
attributable to this issue.
Having the protection (and log message) in the other branches should
be enough to raise visibility of the bug and get it fixed.
There may be uses that are experiencing this bug on rare occasions
without realising it.
2. Limit 8.5.x to a single acceptor thread and back-port the 9.0.x fix
===
Multiple acceptor threads have been shown to offer little/no
performance benefit.
Making the acceptorThreadCount setter a NO-OP and hard-coding the
getter to return 1 would be relatively simple.
This feels like a fairly big change this late in the 8.5.x series.
3. Introduce a synchronization block around the accept() call and add
the duplicate check inside the synchronization block.
===
There may be a performance impact for users who have configured
multiple acceptor threads.
My current preference order is 2 first, closely followed by 1 with 3
further behind.
Thoughts?
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org