Fix race when handling delegating tasks in ReferenceCountedOpenSslEng…

…ine (netty#12149)

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 netty#12139

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;
         }
     }
 

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);
                 }
             }
         }