This is an automated email from the ASF dual-hosted git repository.
remm pushed a commit to branch 10.1.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/10.1.x by this push:
new aadba135f1 Tighten up readNotify and writeNotify
aadba135f1 is described below
commit aadba135f178354a3b01d12128970f30f76921bd
Author: remm <r...@apache.org>
AuthorDate: Tue Sep 19 14:24:15 2023 +0200
Tighten up readNotify and writeNotify
The two flags are important, so it is best to fully protect reading and
updating them. Hopefully will not cause any regressions.
Found by coverity.
---
java/org/apache/tomcat/util/net/Nio2Endpoint.java | 40 +++++++++++++++++------
webapps/docs/changelog.xml | 4 +++
2 files changed, 34 insertions(+), 10 deletions(-)
diff --git a/java/org/apache/tomcat/util/net/Nio2Endpoint.java
b/java/org/apache/tomcat/util/net/Nio2Endpoint.java
index 3196247af1..ca3dd37e23 100644
--- a/java/org/apache/tomcat/util/net/Nio2Endpoint.java
+++ b/java/org/apache/tomcat/util/net/Nio2Endpoint.java
@@ -611,8 +611,9 @@ public class Nio2Endpoint extends
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
if (log.isDebugEnabled()) {
log.debug("Socket: [" + Nio2SocketWrapper.this + "],
Interest: [" + readInterest + "]");
}
- readNotify = false;
+ boolean notify = false;
synchronized (readCompletionHandler) {
+ readNotify = false;
if (nBytes.intValue() < 0) {
failed(new EOFException(), attachment);
} else {
@@ -625,8 +626,9 @@ public class Nio2Endpoint extends
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
}
readInterest = false;
}
+ notify = readNotify;
}
- if (readNotify) {
+ if (notify) {
getEndpoint().processSocket(Nio2SocketWrapper.this,
SocketEvent.OPEN_READ, false);
}
}
@@ -654,9 +656,9 @@ public class Nio2Endpoint extends
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
this.writeCompletionHandler = new CompletionHandler<>() {
@Override
public void completed(Integer nBytes, ByteBuffer attachment) {
- writeNotify = false;
boolean notify = false;
synchronized (writeCompletionHandler) {
+ writeNotify = false;
if (nBytes.intValue() < 0) {
failed(new
EOFException(sm.getString("iob.failedwrite")), attachment);
} else if (!nonBlockingWriteBuffer.isEmpty()) {
@@ -708,9 +710,9 @@ public class Nio2Endpoint extends
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
gatheringWriteCompletionHandler = new CompletionHandler<>() {
@Override
public void completed(Long nBytes, ByteBuffer[] attachment) {
- writeNotify = false;
boolean notify = false;
synchronized (writeCompletionHandler) {
+ writeNotify = false;
if (nBytes.longValue() < 0) {
failed(new
EOFException(sm.getString("iob.failedwrite")), attachment);
} else if (!nonBlockingWriteBuffer.isEmpty() ||
buffersArrayHasRemaining(attachment, 0, attachment.length)) {
@@ -827,7 +829,12 @@ public class Nio2Endpoint extends
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
throw new IOException(sm.getString("socket.closed"));
}
- if (!readNotify) {
+ boolean notify = false;
+ synchronized (readCompletionHandler) {
+ notify = readNotify;
+ }
+
+ if (!notify) {
if (block) {
try {
readPending.acquire();
@@ -847,7 +854,9 @@ public class Nio2Endpoint extends
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
int nRead = populateReadBuffer(b, off, len);
if (nRead > 0) {
// The code that was notified is now reading its data
- readNotify = false;
+ synchronized (readCompletionHandler) {
+ readNotify = false;
+ }
// This may be sufficient to complete the request and we
// don't want to trigger another read since if there is no
// more data to read and this request takes a while to
@@ -884,7 +893,12 @@ public class Nio2Endpoint extends
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
throw new IOException(sm.getString("socket.closed"));
}
- if (!readNotify) {
+ boolean notify = false;
+ synchronized (readCompletionHandler) {
+ notify = readNotify;
+ }
+
+ if (!notify) {
if (block) {
try {
readPending.acquire();
@@ -904,7 +918,9 @@ public class Nio2Endpoint extends
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
int nRead = populateReadBuffer(to);
if (nRead > 0) {
// The code that was notified is now reading its data
- readNotify = false;
+ synchronized (readCompletionHandler) {
+ readNotify = false;
+ }
// This may be sufficient to complete the request and we
// don't want to trigger another read since if there is no
// more data to read and this request takes a while to
@@ -1022,10 +1038,14 @@ public class Nio2Endpoint extends
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
protected void start() {
if (read) {
// Disable any regular read notifications caused by
registerReadInterest
- readNotify = true;
+ synchronized (readCompletionHandler) {
+ readNotify = true;
+ }
} else {
// Disable any regular write notifications caused by
registerWriteInterest
- writeNotify = true;
+ synchronized (writeCompletionHandler) {
+ writeNotify = true;
+ }
}
Nio2Endpoint.startInline();
try {
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 87445c1198..3014aa1b8b 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -133,6 +133,10 @@
Fix logic issue trying to match no argument method in IntropectionUtil.
(remm)
</fix>
+ <fix>
+ Improve thread safety around readNotify and writeNotify in the NIO2
+ endpoint. (remm)
+ </fix>
</changelog>
</subsection>
<subsection name="Other">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
