[tomcat] branch master updated: Better cleanup in setSocketOptions

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

[tomcat] branch master updated: Better cleanup in setSocketOptions

Rémy Maucherat
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 <[hidden email]>
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [tomcat] branch master updated: Better cleanup in setSocketOptions

Rémy Maucherat
On Thu, Nov 7, 2019 at 11:11 AM <[hidden email]> wrote:
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 <[hidden email]>
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.

Note: APR doesn't need this change since it doesn't have a channel needing cleanup, and its closeSocket method should do the remove from the connections map.

Rémy