This is an automated email from the ASF dual-hosted git repository.

remm pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/master by this push:
     new 5f4d5b3  Better cleanup in setSocketOptions
5f4d5b3 is described below

commit 5f4d5b357f4388149845fa96d87930d6f664e252
Author: remm <r...@apache.org>
AuthorDate: Thu Nov 7 11:10:41 2019 +0100

    Better cleanup in setSocketOptions
    
    Since the connections map is updated here, the socket must be removed
    from it if things go wrong before the wrapper processing begins. Also
    call free() on the channel (and then discard it) since this wouldn't be
    done anywhere and could leak direct memory in some cases.
---
 java/org/apache/tomcat/util/net/Nio2Endpoint.java | 18 ++++++++++++------
 java/org/apache/tomcat/util/net/NioEndpoint.java  | 15 ++++++++++-----
 webapps/docs/changelog.xml                        |  3 +++
 3 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/java/org/apache/tomcat/util/net/Nio2Endpoint.java 
b/java/org/apache/tomcat/util/net/Nio2Endpoint.java
index 6ecba6a..5879fa9 100644
--- a/java/org/apache/tomcat/util/net/Nio2Endpoint.java
+++ b/java/org/apache/tomcat/util/net/Nio2Endpoint.java
@@ -302,9 +302,10 @@ public class Nio2Endpoint extends 
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
      */
     @Override
     protected boolean setSocketOptions(AsynchronousSocketChannel socket) {
+        Nio2Channel channel = null;
+        boolean success = false;
         try {
             socketProperties.setProperties(socket);
-            Nio2Channel channel = null;
             if (nioChannels != null) {
                 channel = nioChannels.pop();
             }
@@ -326,14 +327,19 @@ public class Nio2Endpoint extends 
AbstractJsseEndpoint<Nio2Channel,AsynchronousS
             socketWrapper.setWriteTimeout(getConnectionTimeout());
             
socketWrapper.setKeepAliveLeft(Nio2Endpoint.this.getMaxKeepAliveRequests());
             socketWrapper.setSecure(isSSLEnabled());
-            // Continue processing on another thread
-            return processSocket(socketWrapper, SocketEvent.OPEN_READ, false);
+            // Continue processing on the same thread as the acceptor is async
+            success = processSocket(socketWrapper, SocketEvent.OPEN_READ, 
false);
         } catch (Throwable t) {
             ExceptionUtils.handleThrowable(t);
-            log.error(sm.getString("endpoint.socketOptionsError"),t);
+            log.error(sm.getString("endpoint.socketOptionsError"), t);
+        } finally {
+            if (!success && channel != null) {
+                connections.remove(channel);
+                channel.free();
+            }
         }
-        // Tell to close the socket
-        return false;
+        // Tell to close the socket if needed
+        return success;
     }
 
 
diff --git a/java/org/apache/tomcat/util/net/NioEndpoint.java 
b/java/org/apache/tomcat/util/net/NioEndpoint.java
index d059a70..1e5b900 100644
--- a/java/org/apache/tomcat/util/net/NioEndpoint.java
+++ b/java/org/apache/tomcat/util/net/NioEndpoint.java
@@ -393,6 +393,8 @@ public class NioEndpoint extends 
AbstractJsseEndpoint<NioChannel,SocketChannel>
      */
     @Override
     protected boolean setSocketOptions(SocketChannel socket) {
+        NioChannel channel = null;
+        boolean success = false;
         // Process the connection
         try {
             // Disable blocking, polling will be used
@@ -400,7 +402,6 @@ public class NioEndpoint extends 
AbstractJsseEndpoint<NioChannel,SocketChannel>
             Socket sock = socket.socket();
             socketProperties.setProperties(sock);
 
-            NioChannel channel = null;
             if (nioChannels != null) {
                 channel = nioChannels.pop();
             }
@@ -414,7 +415,6 @@ public class NioEndpoint extends 
AbstractJsseEndpoint<NioChannel,SocketChannel>
                 } else {
                     channel = new NioChannel(bufhandler);
                 }
-            } else {
             }
             NioSocketWrapper socketWrapper = new NioSocketWrapper(channel, 
this);
             connections.put(channel, socketWrapper);
@@ -424,7 +424,7 @@ public class NioEndpoint extends 
AbstractJsseEndpoint<NioChannel,SocketChannel>
             
socketWrapper.setKeepAliveLeft(NioEndpoint.this.getMaxKeepAliveRequests());
             socketWrapper.setSecure(isSSLEnabled());
             poller.register(channel, socketWrapper);
-            return true;
+            success = true;
         } catch (Throwable t) {
             ExceptionUtils.handleThrowable(t);
             try {
@@ -432,9 +432,14 @@ public class NioEndpoint extends 
AbstractJsseEndpoint<NioChannel,SocketChannel>
             } catch (Throwable tt) {
                 ExceptionUtils.handleThrowable(tt);
             }
+        } finally {
+            if (!success && channel != null) {
+                connections.remove(channel);
+                channel.free();
+            }
         }
-        // Tell to close the socket
-        return false;
+        // Tell to close the socket if needed
+        return success;
     }
 
 
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 0dfa3f2..926f6ce 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -116,6 +116,9 @@
         <code>certificateVerificationDepth</code> are correctly passed to the
         OpenSSL based SSLEngine implementation. (remm/markt)
       </fix>
+      <fix>
+        Improve cleanup after errors when setting socket options. (remm)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Jasper">


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Reply via email to