From c54cfffbb773306903ab2dfe92546b2f8394fe37 Mon Sep 17 00:00:00 2001 From: Hyunsang Han Date: Sun, 17 Aug 2025 01:04:15 +0900 Subject: [PATCH 1/5] binder: Avoid potential deadlock when canceling AsyncSecurityPolicy futures Move future cancellation outside of synchronized block in BinderClientTransport.notifyTerminated() to prevent deadlock if AsyncSecurityPolicy uses directExecutor() for callbacks. Fixes grpc#12190 --- .../grpc/binder/internal/BinderTransport.java | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java index d87cfb74044..3ae567ef134 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -813,14 +813,21 @@ void notifyTerminated() { readyTimeoutFuture.cancel(false); readyTimeoutFuture = null; } - if (preAuthResultFuture != null) { - preAuthResultFuture.cancel(false); // No effect if already complete. - } - if (authResultFuture != null) { - authResultFuture.cancel(false); // No effect if already complete. - } + + ListenableFuture preAuthFuture = preAuthResultFuture; + ListenableFuture authFuture = authResultFuture; + preAuthResultFuture = null; + authResultFuture = null; + serviceBinding.unbind(); clientTransportListener.transportTerminated(); + + if (preAuthFuture != null) { + preAuthFuture.cancel(false); // No effect if already complete. + } + if (authFuture != null) { + authFuture.cancel(false); // No effect if already complete. + } } @Override From 7138614b0c3055d6e9547917e9ab82128f264fa2 Mon Sep 17 00:00:00 2001 From: Hyunsang Han Date: Sun, 24 Aug 2025 23:04:45 +0900 Subject: [PATCH 2/5] binder: Fix potential deadlock in BinderClientTransport.notifyTerminated() Move future cancellation to offloadExecutor to avoid deadlock when AsyncSecurityPolicy uses directExecutor() for callbacks. Fixes #12190 Signed-off-by: Hyunsang Han --- .../internal/BinderClientTransport.java | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java index 95bd531aa41..6926e185627 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java @@ -316,14 +316,21 @@ void notifyTerminated() { readyTimeoutFuture.cancel(false); readyTimeoutFuture = null; } - if (preAuthResultFuture != null) { - preAuthResultFuture.cancel(false); // No effect if already complete. - } - if (authResultFuture != null) { - authResultFuture.cancel(false); // No effect if already complete. - } + + ListenableFuture preAuthFuture = preAuthResultFuture; + ListenableFuture authFuture = authResultFuture; + preAuthResultFuture = null; + authResultFuture = null; + serviceBinding.unbind(); clientTransportListener.transportTerminated(); + + if (preAuthFuture != null) { + offloadExecutor.execute(() -> preAuthFuture.cancel(false)); // No effect if already complete. + } + if (authFuture != null) { + offloadExecutor.execute(() -> authFuture.cancel(false)); // No effect if already complete. + } } @Override From 4d7ed66531ca0d4ca31692ddb42db4f9f7d6f057 Mon Sep 17 00:00:00 2001 From: Hyunsang Han Date: Thu, 28 Aug 2025 08:49:03 +0900 Subject: [PATCH 3/5] binder: Extract cancelAsync method with isDone optimization Extract future cancellation logic into cancelAsync method and only cancel futures that are not already done for better performance. Signed-off-by: Hyunsang Han --- .../internal/BinderClientTransport.java | 26 +++++++++++-------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java index 6926e185627..2ce282411eb 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java @@ -317,20 +317,13 @@ void notifyTerminated() { readyTimeoutFuture = null; } - ListenableFuture preAuthFuture = preAuthResultFuture; - ListenableFuture authFuture = authResultFuture; - preAuthResultFuture = null; - authResultFuture = null; - serviceBinding.unbind(); clientTransportListener.transportTerminated(); - if (preAuthFuture != null) { - offloadExecutor.execute(() -> preAuthFuture.cancel(false)); // No effect if already complete. - } - if (authFuture != null) { - offloadExecutor.execute(() -> authFuture.cancel(false)); // No effect if already complete. - } + cancelAsync(preAuthResultFuture); + cancelAsync(authResultFuture); + preAuthResultFuture = null; + authResultFuture = null; } @Override @@ -406,6 +399,17 @@ protected void handlePingResponse(Parcel parcel) { pingTracker.onPingResponse(parcel.readInt()); } + /** + * Enqueues a future for cancellation later, on another thread. + * Useful when the caller wants to cancel while holding locks but the future is visible to + * user code which might have added listeners to run on directExecutor(). + */ + private void cancelAsync(@Nullable ListenableFuture future) { + if (future != null && !future.isDone()) { + offloadExecutor.execute(() -> future.cancel(false)); + } + } + private static ClientStream newFailingClientStream( Status failure, Attributes attributes, Metadata headers, ClientStreamTracer[] tracers) { StatsTraceContext statsTraceContext = From fc77da49ba7b3da262d6ae896cfb0127059b9109 Mon Sep 17 00:00:00 2001 From: Hyunsang Han Date: Wed, 3 Sep 2025 07:35:56 +0900 Subject: [PATCH 4/5] binder: Rename method, relocate calls, and remove null assignments Rename cancelAsync to cancelAsyncIfNeeded, move future cancellation next to readyTimeoutFuture, and remove unnecessary null assignments. Signed-off-by: Hyunsang Han --- .../io/grpc/binder/internal/BinderClientTransport.java | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java index 2ce282411eb..b87107e0460 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java @@ -316,14 +316,11 @@ void notifyTerminated() { readyTimeoutFuture.cancel(false); readyTimeoutFuture = null; } + cancelAsyncIfNeeded(preAuthResultFuture); + cancelAsyncIfNeeded(authResultFuture); serviceBinding.unbind(); clientTransportListener.transportTerminated(); - - cancelAsync(preAuthResultFuture); - cancelAsync(authResultFuture); - preAuthResultFuture = null; - authResultFuture = null; } @Override @@ -404,7 +401,7 @@ protected void handlePingResponse(Parcel parcel) { * Useful when the caller wants to cancel while holding locks but the future is visible to * user code which might have added listeners to run on directExecutor(). */ - private void cancelAsync(@Nullable ListenableFuture future) { + private void cancelAsyncIfNeeded(@Nullable ListenableFuture future) { if (future != null && !future.isDone()) { offloadExecutor.execute(() -> future.cancel(false)); } From 1c3a0ea71eb5f247ef507e019f6521591f5fd068 Mon Sep 17 00:00:00 2001 From: Hyunsang Han Date: Wed, 8 Oct 2025 15:23:48 +0900 Subject: [PATCH 5/5] binder: Use register() pattern to cancel futures safely without holding locks Signed-off-by: Hyunsang Han --- .../internal/BinderClientTransport.java | 26 +++---------------- .../grpc/binder/internal/BinderTransport.java | 19 ++++++++++++++ 2 files changed, 23 insertions(+), 22 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java index b87107e0460..dcaeb464f0c 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java @@ -87,13 +87,6 @@ public final class BinderClientTransport extends BinderTransport @GuardedBy("this") private ScheduledFuture readyTimeoutFuture; // != null iff timeout scheduled. - @GuardedBy("this") - @Nullable - private ListenableFuture authResultFuture; // null before we check auth. - - @GuardedBy("this") - @Nullable - private ListenableFuture preAuthResultFuture; // null before we pre-auth. /** * Constructs a new transport instance. @@ -195,7 +188,8 @@ private void preAuthorize(ServiceInfo serviceInfo) { // unauthorized server a chance to run, but the connection will still fail by SecurityPolicy // check later in handshake. Pre-auth remains effective at mitigating abuse because malware // can't typically control the exact timing of its installation. - preAuthResultFuture = checkServerAuthorizationAsync(serviceInfo.applicationInfo.uid); + ListenableFuture preAuthResultFuture = + register(checkServerAuthorizationAsync(serviceInfo.applicationInfo.uid)); Futures.addCallback( preAuthResultFuture, new FutureCallback() { @@ -316,9 +310,6 @@ void notifyTerminated() { readyTimeoutFuture.cancel(false); readyTimeoutFuture = null; } - cancelAsyncIfNeeded(preAuthResultFuture); - cancelAsyncIfNeeded(authResultFuture); - serviceBinding.unbind(); clientTransportListener.transportTerminated(); } @@ -337,7 +328,8 @@ protected void handleSetupTransport(Parcel parcel) { shutdownInternal( Status.UNAVAILABLE.withDescription("Malformed SETUP_TRANSPORT data"), true); } else { - authResultFuture = checkServerAuthorizationAsync(remoteUid); + ListenableFuture authResultFuture = + register(checkServerAuthorizationAsync(remoteUid)); Futures.addCallback( authResultFuture, new FutureCallback() { @@ -396,16 +388,6 @@ protected void handlePingResponse(Parcel parcel) { pingTracker.onPingResponse(parcel.readInt()); } - /** - * Enqueues a future for cancellation later, on another thread. - * Useful when the caller wants to cancel while holding locks but the future is visible to - * user code which might have added listeners to run on directExecutor(). - */ - private void cancelAsyncIfNeeded(@Nullable ListenableFuture future) { - if (future != null && !future.isDone()) { - offloadExecutor.execute(() -> future.cancel(false)); - } - } private static ClientStream newFailingClientStream( Status failure, Attributes attributes, Metadata headers, ClientStreamTracer[] tracers) { diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java index 0fe131a0728..99b9828a2d7 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -41,9 +41,11 @@ import java.util.ArrayList; import java.util.Iterator; import java.util.LinkedHashSet; +import java.util.List; import java.util.Map; import java.util.NoSuchElementException; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Future; import java.util.concurrent.ScheduledExecutorService; import java.util.logging.Level; import java.util.logging.Logger; @@ -161,6 +163,9 @@ protected enum TransportState { @GuardedBy("this") private final LinkedHashSet callIdsToNotifyWhenReady = new LinkedHashSet<>(); + @GuardedBy("this") + private final List> ownedFutures = new ArrayList<>(); // To cancel upon terminate. + @GuardedBy("this") protected Attributes attributes; @@ -236,6 +241,13 @@ void releaseExecutors() { executorServicePool.returnObject(scheduledExecutorService); } + // Registers the specified future for eventual safe cancellation upon shutdown/terminate. + @GuardedBy("this") + protected final > T register(T future) { + ownedFutures.add(future); + return future; + } + @GuardedBy("this") boolean inState(TransportState transportState) { return this.transportState == transportState; @@ -286,6 +298,8 @@ final void shutdownInternal(Status shutdownStatus, boolean forceTerminate) { sendShutdownTransaction(); ArrayList> calls = new ArrayList<>(ongoingCalls.values()); ongoingCalls.clear(); + ArrayList> futuresToCancel = new ArrayList<>(ownedFutures); + ownedFutures.clear(); scheduledExecutorService.execute( () -> { for (Inbound inbound : calls) { @@ -297,6 +311,11 @@ final void shutdownInternal(Status shutdownStatus, boolean forceTerminate) { notifyTerminated(); } releaseExecutors(); + + for (Future future : futuresToCancel) { + // Not holding any locks here just in case some listener runs on a direct Executor. + future.cancel(false); // No effect if already isDone(). + } }); } }