Fix race when handling delegating tasks in ReferenceCountedOpenSslEngine (#12149)

normanmaurer · web-flow · commit 1df38d2c7be9 · 2022-03-08T13:24:35.000-08:00
Motivation: Due incorrect handling of delegating tasks in ReferenceCountedOpenSslEngine it was possible to observe handshake timeouts / failures. Modification: - Only reset needsTask variable after we actually ran the task. - Add missing synchronized as otherwise we might end up calling native code concurrently which is not safe. - Change how we excute delegating tasks in our SSLEngineTest. This change would result in timeouts / failures before the fix. Result: Fixes #12139
diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
@@ -1456,13 +1456,7 @@ private class TaskDecorator<R extends Runnable> implements Runnable {
 
         @Override
         public void run() {
-            if (isDestroyed()) {
-                // The engine was destroyed in the meantime, just return.
-                return;
-            }
-            // The task was run, reset needTask to false so getHandshakeStatus() returns the correct value.
-            needTask = false;
-            task.run();
+            runAndResetNeedTask(task);
         }
     }
 
@@ -1475,19 +1469,22 @@ private final class AsyncTaskDecorator extends TaskDecorator<AsyncTask> implemen
         public void run(final Runnable runnable) {
             if (isDestroyed()) {
                 // The engine was destroyed in the meantime, just return.
-                runnable.run();
                 return;
             }
-            task.runAsync(new Runnable() {
-                @Override
-                public void run() {
-                    // The task was run, reset needTask to false so getHandshakeStatus() returns the correct value.
-                    // This needs to be done before we run the completion runnable, since that might
-                    // query the handshake status.
-                    needTask = false;
-                    runnable.run();
-                }
-            });
+            task.runAsync(new TaskDecorator<Runnable>(runnable));
+        }
+    }
+
+    private synchronized void runAndResetNeedTask(Runnable task) {
+        try {
+            if (isDestroyed()) {
+                // The engine was destroyed in the meantime, just return.
+                return;
+            }
+            task.run();
+        } finally {
+            // The task was run, reset needTask to false so getHandshakeStatus() returns the correct value.
+            needTask = false;
         }
     }
 
diff --git a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java
@@ -1712,7 +1712,7 @@ private static boolean isHandshakeFinished(SSLEngineResult result) {
         return result.getHandshakeStatus() == SSLEngineResult.HandshakeStatus.FINISHED;
     }
 
-    private void runDelegatedTasks(boolean delegate, SSLEngineResult result, SSLEngine engine) throws Exception {
+    private void runDelegatedTasks(boolean delegate, SSLEngineResult result, SSLEngine engine) {
         if (result.getHandshakeStatus() == SSLEngineResult.HandshakeStatus.NEED_TASK) {
             for (;;) {
                 Runnable task = engine.getDelegatedTask();
@@ -1722,7 +1722,7 @@ private void runDelegatedTasks(boolean delegate, SSLEngineResult result, SSLEngi
                 if (!delegate) {
                     task.run();
                 } else {
-                    delegatingExecutor.submit(task).get();
+                    delegatingExecutor.execute(task);
                 }
             }
         }

Original file line number	Diff line number	Diff line change
`@@ -1712,7 +1712,7 @@ private static boolean isHandshakeFinished(SSLEngineResult result) {`
`1712`	`1712`	`return result.getHandshakeStatus() == SSLEngineResult.HandshakeStatus.FINISHED;`
`1713`	`1713`	`}`
`1714`	`1714`
`1715`		`- private void runDelegatedTasks(boolean delegate, SSLEngineResult result, SSLEngine engine) throws Exception {`
	`1715`	`+ private void runDelegatedTasks(boolean delegate, SSLEngineResult result, SSLEngine engine) {`
`1716`	`1716`	`if (result.getHandshakeStatus() == SSLEngineResult.HandshakeStatus.NEED_TASK) {`
`1717`	`1717`	`for (;;) {`
`1718`	`1718`	`Runnable task = engine.getDelegatedTask();`
`@@ -1722,7 +1722,7 @@ private void runDelegatedTasks(boolean delegate, SSLEngineResult result, SSLEngi`
`1722`	`1722`	`if (!delegate) {`
`1723`	`1723`	`task.run();`
`1724`	`1724`	`} else {`
`1725`		`- delegatingExecutor.submit(task).get();`
	`1725`	`+ delegatingExecutor.execute(task);`
`1726`	`1726`	`}`
`1727`	`1727`	`}`
`1728`	`1728`	`}`