possible race condition in jetty 12.x code

[ludoch]

Jetty version(s)
Jetty 12.1
Jetty Environment

HTTP version

Java version/vendor (use: java -version)

OS type/version
Description
Internal Google code scanner and tests have detected some race conditions in AppEngine code, that we are fixing. While doing the fixes other race conditions were seen in Jetty code. Google Gemini recommended these changes:

org.eclipse.jetty.io.SelectableChannelEndPoint: The race occurs because toEndPointString() reads _currentInterestOps and _desiredInterestOps concurrently with modifications in updateKey() and onSelected(). These variables must be read inside a locked block.

--- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/SelectableChannelEndPoint.java
+++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/SelectableChannelEndPoint.java
@@ -324,11 +324,19 @@
     @Override
     public String toEndPointString()
     {
+        int currentInterestOps;
+        int desiredInterestOps;
+        try (AutoLock l = _lock.lock())
+        {
+            currentInterestOps = _currentInterestOps;
+            desiredInterestOps = _desiredInterestOps;
+        }
+
         // We do a best effort to print the right toString() and that's it.
         return String.format("%s{io=%d/%d,kio=%d,kro=%d}",
             super.toEndPointString(),
-            _currentInterestOps,
-            _desiredInterestOps,
+            currentInterestOps,
+            desiredInterestOps,
             ManagedSelector.safeInterestOps(_key),
             ManagedSelector.safeReadyOps(_key));
     }

org.eclipse.jetty.server.AbstractConnector: There are multiple data races related to the concurrent access to the _acceptors array elements from the shutdown thread (isShutdownDone()) while the Acceptor.run() worker loops modify them. Furthermore, primitive timeout configurations like _idleTimeout, _shutdownIdleTimeout, and _acceptorPriorityDelta are not volatile.

--- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java
+++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java
@@ -153,13 +153,13 @@
     private final Set<EndPoint> _endpoints = Collections.newSetFromMap(new ConcurrentHashMap<>());
     private final Set<EndPoint> _immutableEndPoints = Collections.unmodifiableSet(_endpoints);
     private volatile Shutdown _shutdown;
-    private long _idleTimeout = 30000;
-    private long _shutdownIdleTimeout = 1000L;
+    private volatile long _idleTimeout = 30000;
+    private volatile long _shutdownIdleTimeout = 1000L;
     private String _defaultProtocol;
     private ConnectionFactory _defaultConnectionFactory;
     /* The name used to link up virtual host configuration to named connectors */
     private String _name;
-    private int _acceptorPriorityDelta = -2;
+    private volatile int _acceptorPriorityDelta = -2;
     private boolean _accepting = true;
     private ThreadPoolBudget.Lease _lease;
 
@@ -291,10 +291,13 @@
                 if (!_endpoints.isEmpty())
                     return false;
 
-                for (Thread a : _acceptors)
+                try (AutoLock lock = _lock.lock())
                 {
-                    if (a != null)
-                        return false;
+                    for (Thread a : _acceptors)
+                    {
+                        if (a != null)
+                            return false;
+                    }
                 }
 
                 return true;

Feel free to analyse and apply the fixes if you see they are valuable.

Similar for Jetty9, but no need to look at.
How to reproduce?

[ludoch]

See what we could fix and the suppressions txt files in GoogleCloudPlatform/appengine-java-standard@d008b3f

[lorban]

@ludoch please note that toString() (and the methods called from those) cannot take a lock otherwise we risk a deadlock when debug logging is enabled. tryLock() should be used instead, see HttpChannelState.toString() for the pattern to use in these cases.