From fc3e619bfb1dfbd443619ce1e4e7063cf0f23a73 Mon Sep 17 00:00:00 2001
From: Norman Maurer <norman_maurer@apple.com>
Date: Tue, 8 Mar 2022 22:24:35 +0100
Subject: [PATCH] Fix race when handling delegating tasks in
 ReferenceCountedOpenSslEngine (#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 https://github.com/netty/netty/issues/12139
---
 .../ssl/ReferenceCountedOpenSslEngine.java    | 33 +++++++++----------
 .../io/netty/handler/ssl/SSLEngineTest.java   |  4 +--
 2 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
index 0d98e6af2bd..505ae42d814 100644
--- 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
index 112947e9599..6a187fded36 100644
--- 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);
                 }
             }
         }