Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix race when handling delegating tasks in ReferenceCountedOpenSslEngine #12149

Merged
merged 1 commit into from Mar 8, 2022
Merged

Fix race when handling delegating tasks in ReferenceCountedOpenSslEngine #12149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 15 additions & 18 deletions handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
View file
Open in desktop
Expand Up @@ -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);
}
}

Expand All @@ -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();
Copy link
Contributor

@johnou johnou Mar 8, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@normanmaurer intentionally not running the runnable in this refactor if the engine is destroyed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes as the SSL object is already destroyed

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

Expand Down
4 changes: 2 additions & 2 deletions handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java
View file
Open in desktop
Expand Up @@ -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();
Expand All @@ -1722,7 +1722,7 @@ private void runDelegatedTasks(boolean delegate, SSLEngineResult result, SSLEngi
if (!delegate) {
task.run();
} else {
delegatingExecutor.submit(task).get();
delegatingExecutor.execute(task);
}
}
}
Expand Down