This is an automated email from the ASF dual-hosted git repository.
remm 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 ed9f734b41 Tighten up readNotify and writeNotify
ed9f734b41 is described below
commit ed9f734b4110acfc42b2453643d9b14641645321
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 94c7543497..f64a1bd423 100644
--- a/java/org/apache/tomcat/util/net/Nio2Endpoint.java
+++ b/java/org/apache/tomcat/util/net/Nio2Endpoint.java
@@ -625,8 +625,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 {
@@ -639,8 +640,9 @@ public class Nio2Endpoint extends
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
}
readInterest = false;
}
+ notify = readNotify;
}
- if (readNotify) {
+ if (notify) {
getEndpoint().processSocket(Nio2SocketWrapper.this,
SocketEvent.OPEN_READ, false);
}
}
@@ -668,9 +670,9 @@ public class Nio2Endpoint extends
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
this.writeCompletionHandler = new CompletionHandler<Integer,
ByteBuffer>() {
@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()) {
@@ -722,9 +724,9 @@ public class Nio2Endpoint extends
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
gatheringWriteCompletionHandler = new CompletionHandler<Long,
ByteBuffer[]>() {
@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)) {
@@ -841,7 +843,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();
@@ -861,7 +868,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
@@ -898,7 +907,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();
@@ -918,7 +932,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
@@ -1036,10 +1052,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 096d46850f..828a94d299 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
