From c11f45d6748b07dbe20fea363731920fa96d2101 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 19 Aug 2025 17:21:15 -0700 Subject: [PATCH 01/24] Pre-factor out the guts of the BinderClientTransport handshake --- .../grpc/binder/internal/BinderTransport.java | 123 ++++++++++++------ 1 file changed, 86 insertions(+), 37 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..7fefde7720b 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -22,6 +22,7 @@ import static io.grpc.binder.ApiConstants.PRE_AUTH_SERVER_OVERRIDE; import static java.util.concurrent.TimeUnit.MILLISECONDS; +import android.content.ComponentName; import android.content.Context; import android.content.pm.ServiceInfo; import android.os.Binder; @@ -32,6 +33,7 @@ import android.os.RemoteException; import android.os.TransactionTooLargeException; import androidx.annotation.BinderThread; +import androidx.annotation.MainThread; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Ticker; import com.google.common.base.Verify; @@ -559,6 +561,27 @@ final void handleAcknowledgedBytes(long numBytes) { } } + /** + * An abstraction of the client handshake, used to transition off a problematic legacy approach. + */ + interface ClientHandshake { + /** + * Notifies the implementation that the binding has succeeded and we are now connected to the + * server 'endpointBinder'. + */ + @GuardedBy("this") + @MainThread + void onBound(IBinder endpointBinder); + + /** + * Notifies the implementation that we've received a valid SETUP_TRANSPORT transaction from a + * server that can be reached at 'serverBinder'. + */ + @GuardedBy("this") + @BinderThread + void handleSetupTransport(IBinder serverBinder); + } + /** Concrete client-side transport implementation. */ @ThreadSafe @Internal @@ -576,6 +599,7 @@ public static final class BinderClientTransport extends BinderTransport private final long readyTimeoutMillis; private final PingTracker pingTracker; private final boolean preAuthorizeServer; + private final ClientHandshake handshakeImpl; @Nullable private ManagedClientTransport.Listener clientTransportListener; @@ -618,6 +642,7 @@ public BinderClientTransport( Boolean preAuthServerOverride = options.getEagAttributes().get(PRE_AUTH_SERVER_OVERRIDE); this.preAuthorizeServer = preAuthServerOverride != null ? preAuthServerOverride : factory.preAuthorizeServers; + this.handshakeImpl = new LegacyClientHandshake(); numInUseStreams = new AtomicInteger(); pingTracker = new PingTracker(Ticker.systemTicker(), (id) -> sendPing(id)); @@ -641,9 +666,8 @@ void releaseExecutors() { } @Override - public synchronized void onBound(IBinder binder) { - sendSetupTransaction( - binderDecorator.decorate(OneWayBinderProxy.wrap(binder, offloadExecutor))); + public synchronized void onBound(IBinder endpointBinder) { + handshakeImpl.onBound(endpointBinder); } @Override @@ -826,8 +850,6 @@ void notifyTerminated() { @Override @GuardedBy("this") protected void handleSetupTransport(Parcel parcel) { - int remoteUid = Binder.getCallingUid(); - attributes = setSecurityAttrs(attributes, remoteUid); if (inState(TransportState.SETUP)) { int version = parcel.readInt(); IBinder binder = parcel.readStrongBinder(); @@ -838,21 +860,7 @@ protected void handleSetupTransport(Parcel parcel) { shutdownInternal( Status.UNAVAILABLE.withDescription("Malformed SETUP_TRANSPORT data"), true); } else { - authResultFuture = checkServerAuthorizationAsync(remoteUid); - Futures.addCallback( - authResultFuture, - new FutureCallback() { - @Override - public void onSuccess(Status result) { - handleAuthResult(binder, result); - } - - @Override - public void onFailure(Throwable t) { - handleAuthResult(t); - } - }, - offloadExecutor); + handshakeImpl.handleSetupTransport(binder); } } } @@ -863,29 +871,70 @@ private ListenableFuture checkServerAuthorizationAsync(int remoteUid) { : Futures.submit(() -> securityPolicy.checkAuthorization(remoteUid), offloadExecutor); } - private synchronized void handleAuthResult(IBinder binder, Status authorization) { - if (inState(TransportState.SETUP)) { - if (!authorization.isOk()) { - shutdownInternal(authorization, true); - } else if (!setOutgoingBinder(OneWayBinderProxy.wrap(binder, offloadExecutor))) { - shutdownInternal( - Status.UNAVAILABLE.withDescription("Failed to observe outgoing binder"), true); - } else { - // Check state again, since a failure inside setOutgoingBinder (or a callback it - // triggers), could have shut us down. - if (!isShutdown()) { - setState(TransportState.READY); - attributes = clientTransportListener.filterTransport(attributes); - clientTransportListener.transportReady(); - if (readyTimeoutFuture != null) { - readyTimeoutFuture.cancel(false); - readyTimeoutFuture = null; + class LegacyClientHandshake implements ClientHandshake { + @Override + @MainThread + @GuardedBy("BinderClientTransport.this") + public void onBound(IBinder binder) { + sendSetupTransaction( + binderDecorator.decorate(OneWayBinderProxy.wrap(binder, offloadExecutor))); + } + + @Override + @BinderThread + @GuardedBy("BinderClientTransport.this") + public void handleSetupTransport(IBinder binder) { + int remoteUid = Binder.getCallingUid(); + attributes = setSecurityAttrs(attributes, remoteUid); + authResultFuture = checkServerAuthorizationAsync(remoteUid); + Futures.addCallback( + authResultFuture, + new FutureCallback() { + @Override + public void onSuccess(Status result) { + synchronized (BinderClientTransport.this) { + handleAuthResult(binder, result); + } + } + + @Override + public void onFailure(Throwable t) { + BinderClientTransport.this.handleAuthResult(t); + } + }, + offloadExecutor); + } + + @GuardedBy("BinderClientTransport.this") + private void handleAuthResult(IBinder binder, Status authorization) { + if (inState(TransportState.SETUP)) { + if (!authorization.isOk()) { + shutdownInternal(authorization, true); + } else if (!setOutgoingBinder(OneWayBinderProxy.wrap(binder, offloadExecutor))) { + shutdownInternal( + Status.UNAVAILABLE.withDescription("Failed to observe outgoing binder"), true); + } else { + // Check state again, since a failure inside setOutgoingBinder (or a callback it + // triggers), could have shut us down. + if (!isShutdown()) { + reportReady(); } } } } } + @GuardedBy("this") + private void reportReady() { + setState(TransportState.READY); + attributes = clientTransportListener.filterTransport(attributes); + clientTransportListener.transportReady(); + if (readyTimeoutFuture != null) { + readyTimeoutFuture.cancel(false); + readyTimeoutFuture = null; + } + } + private synchronized void handleAuthResult(Throwable t) { shutdownInternal( Status.INTERNAL.withDescription("Could not evaluate SecurityPolicy").withCause(t), true); From 8486ac1c2e4a7ff4c57f3601688bdb17e6ed7c2d Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 19 Aug 2025 17:23:34 -0700 Subject: [PATCH 02/24] don't rename binder --- .../main/java/io/grpc/binder/internal/BinderTransport.java | 4 ++-- 1 file changed, 2 insertions(+), 2 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 7fefde7720b..ba02e597714 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -666,8 +666,8 @@ void releaseExecutors() { } @Override - public synchronized void onBound(IBinder endpointBinder) { - handshakeImpl.onBound(endpointBinder); + public synchronized void onBound(IBinder binder) { + handshakeImpl.onBound(binder); } @Override From efd855f8183df7ccb1f88d266c05c64e7ff59e55 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 19 Aug 2025 17:31:34 -0700 Subject: [PATCH 03/24] ComponentName --- .../src/main/java/io/grpc/binder/internal/BinderTransport.java | 1 - 1 file changed, 1 deletion(-) 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 ba02e597714..f1b1509d9e5 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -22,7 +22,6 @@ import static io.grpc.binder.ApiConstants.PRE_AUTH_SERVER_OVERRIDE; import static java.util.concurrent.TimeUnit.MILLISECONDS; -import android.content.ComponentName; import android.content.Context; import android.content.pm.ServiceInfo; import android.os.Binder; From 80eee5816434b5fb6eef8373b5890ae6998ca3ba Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 19 Aug 2025 17:21:15 -0700 Subject: [PATCH 04/24] Pre-factor out the guts of the BinderClientTransport handshake # Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java --- .../main/java/io/grpc/binder/internal/BinderTransport.java | 4 ++++ 1 file changed, 4 insertions(+) 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..39779f7bd7c 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -20,12 +20,16 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.common.util.concurrent.Futures.immediateFuture; +import android.content.Context; +import android.content.pm.ServiceInfo; +import android.os.Binder; import android.os.DeadObjectException; import android.os.IBinder; import android.os.Parcel; import android.os.RemoteException; import android.os.TransactionTooLargeException; import androidx.annotation.BinderThread; +import androidx.annotation.MainThread; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Verify; import com.google.common.util.concurrent.ListenableFuture; From 643144c17912e9bab54a1e02fe1929b7d47647b8 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 19 Aug 2025 17:21:15 -0700 Subject: [PATCH 05/24] Pre-factor out the guts of the BinderClientTransport handshake # Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java --- .../src/main/java/io/grpc/binder/internal/BinderTransport.java | 1 + 1 file changed, 1 insertion(+) 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 39779f7bd7c..0c9259b91db 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -20,6 +20,7 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.common.util.concurrent.Futures.immediateFuture; +import android.content.ComponentName; import android.content.Context; import android.content.pm.ServiceInfo; import android.os.Binder; From aedc3bc8e3b7fe132f55f6147c1225056ac4cc4c Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 19 Aug 2025 17:31:34 -0700 Subject: [PATCH 06/24] ComponentName --- .../src/main/java/io/grpc/binder/internal/BinderTransport.java | 1 - 1 file changed, 1 deletion(-) 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 0c9259b91db..39779f7bd7c 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -20,7 +20,6 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.common.util.concurrent.Futures.immediateFuture; -import android.content.ComponentName; import android.content.Context; import android.content.pm.ServiceInfo; import android.os.Binder; From c208ab3cfdeb5799db4e1ba39bba42a704240d47 Mon Sep 17 00:00:00 2001 From: jdcormie Date: Sat, 30 Aug 2025 00:03:33 +0100 Subject: [PATCH 07/24] factor out the decorate() and wrap() calls common to all handshakes --- .../internal/BinderClientTransport.java | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 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 d77e16ed5ce..d4f384a1fe4 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java @@ -150,7 +150,9 @@ void releaseExecutors() { @Override public synchronized void onBound(IBinder binder) { - handshake.onBound(binder); + OneWayBinderProxy binderProxy = OneWayBinderProxy.wrap(binder, offloadExecutor); + binderProxy = binderDecorator.decorate(binderProxy); + handshake.onBound(binderProxy); } @Override @@ -342,7 +344,8 @@ protected void handleSetupTransport(Parcel parcel) { shutdownInternal( Status.UNAVAILABLE.withDescription("Malformed SETUP_TRANSPORT data"), true); } else { - handshake.handleSetupTransport(binder); + OneWayBinderProxy binderProxy = OneWayBinderProxy.wrap(binder, offloadExecutor); + handshake.handleSetupTransport(binderProxy); } } } @@ -357,15 +360,14 @@ class LegacyClientHandshake implements ClientHandshake { @Override @MainThread @GuardedBy("BinderClientTransport.this") - public void onBound(IBinder binder) { - sendSetupTransaction( - binderDecorator.decorate(OneWayBinderProxy.wrap(binder, offloadExecutor))); + public void onBound(OneWayBinderProxy binder) { + sendSetupTransaction(binder); } @Override @BinderThread @GuardedBy("BinderClientTransport.this") - public void handleSetupTransport(IBinder binder) { + public void handleSetupTransport(OneWayBinderProxy binder) { int remoteUid = Binder.getCallingUid(); attributes = setSecurityAttrs(attributes, remoteUid); authResultFuture = checkServerAuthorizationAsync(remoteUid); @@ -388,11 +390,11 @@ public void onFailure(Throwable t) { } @GuardedBy("BinderClientTransport.this") - private void handleAuthResult(IBinder binder, Status authorization) { + private void handleAuthResult(OneWayBinderProxy binder, Status authorization) { if (inState(TransportState.SETUP)) { if (!authorization.isOk()) { shutdownInternal(authorization, true); - } else if (!setOutgoingBinder(OneWayBinderProxy.wrap(binder, offloadExecutor))) { + } else if (!setOutgoingBinder(binder)) { shutdownInternal( Status.UNAVAILABLE.withDescription("Failed to observe outgoing binder"), true); } else { @@ -438,7 +440,7 @@ interface ClientHandshake { */ @GuardedBy("this") @MainThread - void onBound(IBinder endpointBinder); + void onBound(OneWayBinderProxy endpointBinder); /** * Notifies the implementation that we've received a valid SETUP_TRANSPORT transaction from a @@ -446,7 +448,7 @@ interface ClientHandshake { */ @GuardedBy("this") @BinderThread - void handleSetupTransport(IBinder serverBinder); + void handleSetupTransport(OneWayBinderProxy serverBinder); } private static ClientStream newFailingClientStream( From a5a6057343de6907a30c4b03eef7d1e4257fc91c Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 19 Aug 2025 17:21:15 -0700 Subject: [PATCH 08/24] Pre-factor out the guts of the BinderClientTransport handshake # Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java --- .../main/java/io/grpc/binder/internal/BinderTransport.java | 4 ++++ 1 file changed, 4 insertions(+) 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 6c89c56ffd4..b3987e1ee1f 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -21,12 +21,16 @@ import static com.google.common.util.concurrent.Futures.immediateFuture; import static io.grpc.binder.internal.TransactionUtils.newCallerFilteringHandler; +import android.content.Context; +import android.content.pm.ServiceInfo; +import android.os.Binder; import android.os.DeadObjectException; import android.os.IBinder; import android.os.Parcel; import android.os.RemoteException; import android.os.TransactionTooLargeException; import androidx.annotation.BinderThread; +import androidx.annotation.MainThread; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Verify; import com.google.common.util.concurrent.ListenableFuture; From 0400204fad0a1180920c4b383200a11ecd0dd6f2 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 19 Aug 2025 17:21:15 -0700 Subject: [PATCH 09/24] Pre-factor out the guts of the BinderClientTransport handshake # Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java --- .../grpc/binder/internal/BinderTransport.java | 520 ++++++++++++++++++ 1 file changed, 520 insertions(+) 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 b3987e1ee1f..d4a04e063b0 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -21,6 +21,7 @@ import static com.google.common.util.concurrent.Futures.immediateFuture; import static io.grpc.binder.internal.TransactionUtils.newCallerFilteringHandler; +import android.content.ComponentName; import android.content.Context; import android.content.pm.ServiceInfo; import android.os.Binder; @@ -550,6 +551,525 @@ final void handleAcknowledgedBytes(long numBytes) { } } + /** + * An abstraction of the client handshake, used to transition off a problematic legacy approach. + */ + interface ClientHandshake { + /** + * Notifies the implementation that the binding has succeeded and we are now connected to the + * server 'endpointBinder'. + */ + @GuardedBy("this") + @MainThread + void onBound(IBinder endpointBinder); + + /** + * Notifies the implementation that we've received a valid SETUP_TRANSPORT transaction from a + * server that can be reached at 'serverBinder'. + */ + @GuardedBy("this") + @BinderThread + void handleSetupTransport(IBinder serverBinder); + } + + /** Concrete client-side transport implementation. */ + @ThreadSafe + @Internal + public static final class BinderClientTransport extends BinderTransport + implements ConnectionClientTransport, Bindable.Observer { + + private final ObjectPool offloadExecutorPool; + private final Executor offloadExecutor; + private final SecurityPolicy securityPolicy; + private final Bindable serviceBinding; + + /** Number of ongoing calls which keep this transport "in-use". */ + private final AtomicInteger numInUseStreams; + + private final long readyTimeoutMillis; + private final PingTracker pingTracker; + private final boolean preAuthorizeServer; + private final ClientHandshake handshakeImpl; + + @Nullable private ManagedClientTransport.Listener clientTransportListener; + + @GuardedBy("this") + private int latestCallId = FIRST_CALL_ID; + + @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. + * + * @param factory parameters common to all a Channel's transports + * @param targetAddress the fully resolved and load-balanced server address + * @param options other parameters that can vary as transports come and go within a Channel + */ + public BinderClientTransport( + BinderClientTransportFactory factory, + AndroidComponentAddress targetAddress, + ClientTransportOptions options) { + super( + factory.scheduledExecutorPool, + buildClientAttributes( + options.getEagAttributes(), + factory.sourceContext, + targetAddress, + factory.inboundParcelablePolicy), + factory.binderDecorator, + buildLogId(factory.sourceContext, targetAddress)); + this.offloadExecutorPool = factory.offloadExecutorPool; + this.securityPolicy = factory.securityPolicy; + this.offloadExecutor = offloadExecutorPool.getObject(); + this.readyTimeoutMillis = factory.readyTimeoutMillis; + Boolean preAuthServerOverride = options.getEagAttributes().get(PRE_AUTH_SERVER_OVERRIDE); + this.preAuthorizeServer = + preAuthServerOverride != null ? preAuthServerOverride : factory.preAuthorizeServers; + this.handshakeImpl = new LegacyClientHandshake(); + numInUseStreams = new AtomicInteger(); + pingTracker = new PingTracker(Ticker.systemTicker(), (id) -> sendPing(id)); + + serviceBinding = + new ServiceBinding( + factory.mainThreadExecutor, + factory.sourceContext, + factory.channelCredentials, + targetAddress.asBindIntent(), + targetAddress.getTargetUser() != null + ? targetAddress.getTargetUser() + : factory.defaultTargetUserHandle, + factory.bindServiceFlags.toInteger(), + this); + } + + @Override + void releaseExecutors() { + super.releaseExecutors(); + offloadExecutorPool.returnObject(offloadExecutor); + } + + @Override + public synchronized void onBound(IBinder endpointBinder) { + handshakeImpl.onBound(endpointBinder); + } + + @Override + public synchronized void onUnbound(Status reason) { + shutdownInternal(reason, true); + } + + @CheckReturnValue + @Override + public synchronized Runnable start(ManagedClientTransport.Listener clientTransportListener) { + this.clientTransportListener = checkNotNull(clientTransportListener); + return () -> { + synchronized (BinderClientTransport.this) { + if (inState(TransportState.NOT_STARTED)) { + setState(TransportState.SETUP); + try { + if (preAuthorizeServer) { + preAuthorize(serviceBinding.resolve()); + } else { + serviceBinding.bind(); + } + } catch (StatusException e) { + shutdownInternal(e.getStatus(), true); + return; + } + if (readyTimeoutMillis >= 0) { + readyTimeoutFuture = + getScheduledExecutorService() + .schedule( + BinderClientTransport.this::onReadyTimeout, + readyTimeoutMillis, + MILLISECONDS); + } + } + } + }; + } + + @GuardedBy("this") + private void preAuthorize(ServiceInfo serviceInfo) { + // It's unlikely, but the identity/existence of this Service could change by the time we + // actually connect. It doesn't matter though, because: + // - If pre-auth fails (but would succeed against the server's new state), the grpc-core layer + // will eventually retry using a new transport instance that will see the Service's new state. + // - If pre-auth succeeds (but would fail against the server's new state), we might give an + // 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); + Futures.addCallback( + preAuthResultFuture, + new FutureCallback() { + @Override + public void onSuccess(Status result) { + handlePreAuthResult(result); + } + + @Override + public void onFailure(Throwable t) { + handleAuthResult(t); + } + }, + offloadExecutor); + } + + private synchronized void handlePreAuthResult(Status authorization) { + if (inState(TransportState.SETUP)) { + if (!authorization.isOk()) { + shutdownInternal(authorization, true); + } else { + serviceBinding.bind(); + } + } + } + + private synchronized void onReadyTimeout() { + if (inState(TransportState.SETUP)) { + readyTimeoutFuture = null; + shutdownInternal( + Status.DEADLINE_EXCEEDED.withDescription( + "Connect timeout " + readyTimeoutMillis + "ms lapsed"), + true); + } + } + + @Override + public synchronized ClientStream newStream( + final MethodDescriptor method, + final Metadata headers, + final CallOptions callOptions, + ClientStreamTracer[] tracers) { + if (!inState(TransportState.READY)) { + return newFailingClientStream( + isShutdown() + ? shutdownStatus + : Status.INTERNAL.withDescription("newStream() before transportReady()"), + attributes, + headers, + tracers); + } + + int callId = latestCallId++; + if (latestCallId == LAST_CALL_ID) { + latestCallId = FIRST_CALL_ID; + } + StatsTraceContext statsTraceContext = + StatsTraceContext.newClientContext(tracers, attributes, headers); + Inbound.ClientInbound inbound = + new Inbound.ClientInbound( + this, attributes, callId, GrpcUtil.shouldBeCountedForInUse(callOptions)); + if (ongoingCalls.putIfAbsent(callId, inbound) != null) { + Status failure = Status.INTERNAL.withDescription("Clashing call IDs"); + shutdownInternal(failure, true); + return newFailingClientStream(failure, attributes, headers, tracers); + } else { + if (inbound.countsForInUse() && numInUseStreams.getAndIncrement() == 0) { + clientTransportListener.transportInUse(true); + } + Outbound.ClientOutbound outbound = + new Outbound.ClientOutbound(this, callId, method, headers, statsTraceContext); + if (method.getType().clientSendsOneMessage()) { + return new SingleMessageClientStream(inbound, outbound, attributes); + } else { + return new MultiMessageClientStream(inbound, outbound, attributes); + } + } + } + + @Override + protected void unregisterInbound(Inbound inbound) { + if (inbound.countsForInUse() && numInUseStreams.decrementAndGet() == 0) { + clientTransportListener.transportInUse(false); + } + super.unregisterInbound(inbound); + } + + @Override + public void ping(final PingCallback callback, Executor executor) { + pingTracker.startPing(callback, executor); + } + + @Override + public synchronized void shutdown(Status reason) { + checkNotNull(reason, "reason"); + shutdownInternal(reason, false); + } + + @Override + public synchronized void shutdownNow(Status reason) { + checkNotNull(reason, "reason"); + shutdownInternal(reason, true); + } + + @Override + @GuardedBy("this") + void notifyShutdown(Status status) { + clientTransportListener.transportShutdown(status); + } + + @Override + @GuardedBy("this") + void notifyTerminated() { + if (numInUseStreams.getAndSet(0) > 0) { + clientTransportListener.transportInUse(false); + } + if (readyTimeoutFuture != null) { + 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. + } + serviceBinding.unbind(); + clientTransportListener.transportTerminated(); + } + + @Override + @GuardedBy("this") + protected void handleSetupTransport(Parcel parcel) { + if (inState(TransportState.SETUP)) { + int version = parcel.readInt(); + IBinder binder = parcel.readStrongBinder(); + if (version != WIRE_FORMAT_VERSION) { + shutdownInternal( + Status.UNAVAILABLE.withDescription("Wire format version mismatch"), true); + } else if (binder == null) { + shutdownInternal( + Status.UNAVAILABLE.withDescription("Malformed SETUP_TRANSPORT data"), true); + } else { + handshakeImpl.handleSetupTransport(binder); + } + } + } + + private ListenableFuture checkServerAuthorizationAsync(int remoteUid) { + return (securityPolicy instanceof AsyncSecurityPolicy) + ? ((AsyncSecurityPolicy) securityPolicy).checkAuthorizationAsync(remoteUid) + : Futures.submit(() -> securityPolicy.checkAuthorization(remoteUid), offloadExecutor); + } + + class LegacyClientHandshake implements ClientHandshake { + @Override + @MainThread + @GuardedBy("BinderClientTransport.this") + public void onBound(IBinder binder) { + sendSetupTransaction( + binderDecorator.decorate(OneWayBinderProxy.wrap(binder, offloadExecutor))); + } + + @Override + @BinderThread + @GuardedBy("BinderClientTransport.this") + public void handleSetupTransport(IBinder binder) { + int remoteUid = Binder.getCallingUid(); + attributes = setSecurityAttrs(attributes, remoteUid); + authResultFuture = checkServerAuthorizationAsync(remoteUid); + Futures.addCallback( + authResultFuture, + new FutureCallback() { + @Override + public void onSuccess(Status result) { + synchronized (BinderClientTransport.this) { + handleAuthResult(binder, result); + } + } + + @Override + public void onFailure(Throwable t) { + BinderClientTransport.this.handleAuthResult(t); + } + }, + offloadExecutor); + } + + @GuardedBy("BinderClientTransport.this") + private void handleAuthResult(IBinder binder, Status authorization) { + if (inState(TransportState.SETUP)) { + if (!authorization.isOk()) { + shutdownInternal(authorization, true); + } else if (!setOutgoingBinder(OneWayBinderProxy.wrap(binder, offloadExecutor))) { + shutdownInternal( + Status.UNAVAILABLE.withDescription("Failed to observe outgoing binder"), true); + } else { + // Check state again, since a failure inside setOutgoingBinder (or a callback it + // triggers), could have shut us down. + if (!isShutdown()) { + reportReady(); + } + } + } + } + } + + @GuardedBy("this") + private void reportReady() { + setState(TransportState.READY); + attributes = clientTransportListener.filterTransport(attributes); + clientTransportListener.transportReady(); + if (readyTimeoutFuture != null) { + readyTimeoutFuture.cancel(false); + readyTimeoutFuture = null; + } + } + + private synchronized void handleAuthResult(Throwable t) { + shutdownInternal( + Status.INTERNAL.withDescription("Could not evaluate SecurityPolicy").withCause(t), true); + } + + @GuardedBy("this") + @Override + protected void handlePingResponse(Parcel parcel) { + pingTracker.onPingResponse(parcel.readInt()); + } + + private static ClientStream newFailingClientStream( + Status failure, Attributes attributes, Metadata headers, ClientStreamTracer[] tracers) { + StatsTraceContext statsTraceContext = + StatsTraceContext.newClientContext(tracers, attributes, headers); + statsTraceContext.clientOutboundHeaders(); + return new FailingClientStream(failure, tracers); + } + + private static InternalLogId buildLogId( + Context sourceContext, AndroidComponentAddress targetAddress) { + return InternalLogId.allocate( + BinderClientTransport.class, + sourceContext.getClass().getSimpleName() + "->" + targetAddress); + } + + private static Attributes buildClientAttributes( + Attributes eagAttrs, + Context sourceContext, + AndroidComponentAddress targetAddress, + InboundParcelablePolicy inboundParcelablePolicy) { + return Attributes.newBuilder() + .set(GrpcAttributes.ATTR_SECURITY_LEVEL, SecurityLevel.NONE) // Trust noone for now. + .set(GrpcAttributes.ATTR_CLIENT_EAG_ATTRS, eagAttrs) + .set(Grpc.TRANSPORT_ATTR_LOCAL_ADDR, AndroidComponentAddress.forContext(sourceContext)) + .set(Grpc.TRANSPORT_ATTR_REMOTE_ADDR, targetAddress) + .set(INBOUND_PARCELABLE_POLICY, inboundParcelablePolicy) + .build(); + } + + private static Attributes setSecurityAttrs(Attributes attributes, int uid) { + return attributes.toBuilder() + .set(REMOTE_UID, uid) + .set( + GrpcAttributes.ATTR_SECURITY_LEVEL, + uid == Process.myUid() + ? SecurityLevel.PRIVACY_AND_INTEGRITY + : SecurityLevel.INTEGRITY) // TODO: Have the SecrityPolicy decide this. + .build(); + } + } + + /** Concrete server-side transport implementation. */ + @Internal + public static final class BinderServerTransport extends BinderTransport + implements ServerTransport { + + private final List streamTracerFactories; + @Nullable private ServerTransportListener serverTransportListener; + + /** + * Constructs a new transport instance. + * + * @param binderDecorator used to decorate 'callbackBinder', for fault injection. + */ + public BinderServerTransport( + ObjectPool executorServicePool, + Attributes attributes, + List streamTracerFactories, + OneWayBinderProxy.Decorator binderDecorator, + IBinder callbackBinder) { + super(executorServicePool, attributes, binderDecorator, buildLogId(attributes)); + this.streamTracerFactories = streamTracerFactories; + // TODO(jdcormie): Plumb in the Server's executor() and use it here instead. + setOutgoingBinder(OneWayBinderProxy.wrap(callbackBinder, getScheduledExecutorService())); + } + + public synchronized void setServerTransportListener( + ServerTransportListener serverTransportListener) { + this.serverTransportListener = serverTransportListener; + if (isShutdown()) { + setState(TransportState.SHUTDOWN_TERMINATED); + notifyTerminated(); + releaseExecutors(); + } else { + sendSetupTransaction(); + // Check we're not shutdown again, since a failure inside sendSetupTransaction (or a + // callback it triggers), could have shut us down. + if (!isShutdown()) { + setState(TransportState.READY); + attributes = serverTransportListener.transportReady(attributes); + } + } + } + + StatsTraceContext createStatsTraceContext(String methodName, Metadata headers) { + return StatsTraceContext.newServerContext(streamTracerFactories, methodName, headers); + } + + synchronized Status startStream(ServerStream stream, String methodName, Metadata headers) { + if (isShutdown()) { + return Status.UNAVAILABLE.withDescription("transport is shutdown"); + } else { + serverTransportListener.streamCreated(stream, methodName, headers); + return Status.OK; + } + } + + @Override + @GuardedBy("this") + void notifyShutdown(Status status) { + // Nothing to do. + } + + @Override + @GuardedBy("this") + void notifyTerminated() { + if (serverTransportListener != null) { + serverTransportListener.transportTerminated(); + } + } + + @Override + public synchronized void shutdown() { + shutdownInternal(Status.OK, false); + } + + @Override + public synchronized void shutdownNow(Status reason) { + shutdownInternal(reason, true); + } + + @Override + @Nullable + @GuardedBy("this") + protected Inbound createInbound(int callId) { + return new Inbound.ServerInbound(this, attributes, callId); + } + + private static InternalLogId buildLogId(Attributes attributes) { + return InternalLogId.allocate( + BinderServerTransport.class, "from " + attributes.get(Grpc.TRANSPORT_ATTR_REMOTE_ADDR)); + } + } + private static void checkTransition(TransportState current, TransportState next) { switch (next) { case SETUP: From 1bb363cac7e6fedb583c5acc7751947ce083d538 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 19 Aug 2025 17:23:34 -0700 Subject: [PATCH 10/24] don't rename binder --- .../main/java/io/grpc/binder/internal/BinderTransport.java | 4 ++-- 1 file changed, 2 insertions(+), 2 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 d4a04e063b0..f7a928e4875 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -656,8 +656,8 @@ void releaseExecutors() { } @Override - public synchronized void onBound(IBinder endpointBinder) { - handshakeImpl.onBound(endpointBinder); + public synchronized void onBound(IBinder binder) { + handshakeImpl.onBound(binder); } @Override From d77c6f58f20fea75a1e87854f9c06b7b61488206 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 19 Aug 2025 17:31:34 -0700 Subject: [PATCH 11/24] ComponentName --- .../src/main/java/io/grpc/binder/internal/BinderTransport.java | 1 - 1 file changed, 1 deletion(-) 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 f7a928e4875..7286d44dad8 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -21,7 +21,6 @@ import static com.google.common.util.concurrent.Futures.immediateFuture; import static io.grpc.binder.internal.TransactionUtils.newCallerFilteringHandler; -import android.content.ComponentName; import android.content.Context; import android.content.pm.ServiceInfo; import android.os.Binder; From 1cfb40bd3dd8127f21e6c840a908a9a39dbdba00 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 19 Aug 2025 17:21:15 -0700 Subject: [PATCH 12/24] Pre-factor out the guts of the BinderClientTransport handshake # Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java --- .../src/main/java/io/grpc/binder/internal/BinderTransport.java | 1 + 1 file changed, 1 insertion(+) 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 7286d44dad8..f7a928e4875 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -21,6 +21,7 @@ import static com.google.common.util.concurrent.Futures.immediateFuture; import static io.grpc.binder.internal.TransactionUtils.newCallerFilteringHandler; +import android.content.ComponentName; import android.content.Context; import android.content.pm.ServiceInfo; import android.os.Binder; From beb8f6e94cbde15ba42be29e5af83840551d0eeb Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 19 Aug 2025 17:31:34 -0700 Subject: [PATCH 13/24] ComponentName --- .../src/main/java/io/grpc/binder/internal/BinderTransport.java | 1 - 1 file changed, 1 deletion(-) 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 f7a928e4875..7286d44dad8 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -21,7 +21,6 @@ import static com.google.common.util.concurrent.Futures.immediateFuture; import static io.grpc.binder.internal.TransactionUtils.newCallerFilteringHandler; -import android.content.ComponentName; import android.content.Context; import android.content.pm.ServiceInfo; import android.os.Binder; From 3baa4b95f00e6bd48192e38e1db290a84adb7f02 Mon Sep 17 00:00:00 2001 From: jdcormie Date: Sat, 30 Aug 2025 00:03:33 +0100 Subject: [PATCH 14/24] factor out the decorate() and wrap() calls common to all handshakes --- .../internal/BinderClientTransport.java | 113 +++++++++++++----- 1 file changed, 81 insertions(+), 32 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 144ad56eec3..84a75f599e4 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java @@ -146,7 +146,9 @@ void releaseExecutors() { @Override public synchronized void onBound(IBinder binder) { - sendSetupTransaction(binderDecorator.decorate(OneWayBinderProxy.wrap(binder, offloadExecutor))); + OneWayBinderProxy binderProxy = OneWayBinderProxy.wrap(binder, offloadExecutor); + binderProxy = binderDecorator.decorate(binderProxy); + handshake.onBound(binderProxy); } @Override @@ -339,14 +341,39 @@ protected void handleSetupTransport(Parcel parcel) { shutdownInternal( Status.UNAVAILABLE.withDescription("Malformed SETUP_TRANSPORT data"), true); } else { - restrictIncomingBinderToCallsFrom(remoteUid); - attributes = setSecurityAttrs(attributes, remoteUid); - authResultFuture = checkServerAuthorizationAsync(remoteUid); - Futures.addCallback( - authResultFuture, - new FutureCallback() { - @Override - public void onSuccess(Status result) { + OneWayBinderProxy binderProxy = OneWayBinderProxy.wrap(binder, offloadExecutor); + handshake.handleSetupTransport(binderProxy); + } + } + } + + private ListenableFuture checkServerAuthorizationAsync(int remoteUid) { + return (securityPolicy instanceof AsyncSecurityPolicy) + ? ((AsyncSecurityPolicy) securityPolicy).checkAuthorizationAsync(remoteUid) + : Futures.submit(() -> securityPolicy.checkAuthorization(remoteUid), offloadExecutor); + } + + class LegacyClientHandshake implements ClientHandshake { + @Override + @MainThread + @GuardedBy("BinderClientTransport.this") + public void onBound(OneWayBinderProxy binder) { + sendSetupTransaction(binder); + } + + @Override + @BinderThread + @GuardedBy("BinderClientTransport.this") + public void handleSetupTransport(OneWayBinderProxy binder) { + int remoteUid = Binder.getCallingUid(); + attributes = setSecurityAttrs(attributes, remoteUid); + authResultFuture = checkServerAuthorizationAsync(remoteUid); + Futures.addCallback( + authResultFuture, + new FutureCallback() { + @Override + public void onSuccess(Status result) { + synchronized (BinderClientTransport.this) { handleAuthResult(binder, result); } @@ -360,35 +387,36 @@ public void onFailure(Throwable t) { } } - private ListenableFuture checkServerAuthorizationAsync(int remoteUid) { - return (securityPolicy instanceof AsyncSecurityPolicy) - ? ((AsyncSecurityPolicy) securityPolicy).checkAuthorizationAsync(remoteUid) - : Futures.submit(() -> securityPolicy.checkAuthorization(remoteUid), offloadExecutor); - } - - private synchronized void handleAuthResult(IBinder binder, Status authorization) { - if (inState(TransportState.SETUP)) { - if (!authorization.isOk()) { - shutdownInternal(authorization, true); - } else if (!setOutgoingBinder(OneWayBinderProxy.wrap(binder, offloadExecutor))) { - shutdownInternal( - Status.UNAVAILABLE.withDescription("Failed to observe outgoing binder"), true); - } else { - // Check state again, since a failure inside setOutgoingBinder (or a callback it - // triggers), could have shut us down. - if (!isShutdown()) { - setState(TransportState.READY); - attributes = clientTransportListener.filterTransport(attributes); - clientTransportListener.transportReady(); - if (readyTimeoutFuture != null) { - readyTimeoutFuture.cancel(false); - readyTimeoutFuture = null; + @GuardedBy("BinderClientTransport.this") + private void handleAuthResult(OneWayBinderProxy binder, Status authorization) { + if (inState(TransportState.SETUP)) { + if (!authorization.isOk()) { + shutdownInternal(authorization, true); + } else if (!setOutgoingBinder(binder)) { + shutdownInternal( + Status.UNAVAILABLE.withDescription("Failed to observe outgoing binder"), true); + } else { + // Check state again, since a failure inside setOutgoingBinder (or a callback it + // triggers), could have shut us down. + if (!isShutdown()) { + onHandshakeComplete(); } } } } } + @GuardedBy("this") + private void onHandshakeComplete() { + setState(TransportState.READY); + attributes = clientTransportListener.filterTransport(attributes); + clientTransportListener.transportReady(); + if (readyTimeoutFuture != null) { + readyTimeoutFuture.cancel(false); + readyTimeoutFuture = null; + } + } + private synchronized void handleAuthResult(Throwable t) { shutdownInternal( Status.INTERNAL.withDescription("Could not evaluate SecurityPolicy").withCause(t), true); @@ -400,6 +428,27 @@ protected void handlePingResponse(Parcel parcel) { pingTracker.onPingResponse(parcel.readInt()); } + /** + * An abstraction of the client handshake, used to transition off a problematic legacy approach. + */ + interface ClientHandshake { + /** + * Notifies the implementation that the binding has succeeded and we are now connected to the + * server 'endpointBinder'. + */ + @GuardedBy("this") + @MainThread + void onBound(OneWayBinderProxy endpointBinder); + + /** + * Notifies the implementation that we've received a valid SETUP_TRANSPORT transaction from a + * server that can be reached at 'serverBinder'. + */ + @GuardedBy("this") + @BinderThread + void handleSetupTransport(OneWayBinderProxy serverBinder); + } + private static ClientStream newFailingClientStream( Status failure, Attributes attributes, Metadata headers, ClientStreamTracer[] tracers) { StatsTraceContext statsTraceContext = From 24e3ba1f40d99739a81775bb16310cdc6e17be64 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 19 Aug 2025 17:21:15 -0700 Subject: [PATCH 15/24] Pre-factor out the guts of the BinderClientTransport handshake # Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java --- .../main/java/io/grpc/binder/internal/BinderTransport.java | 4 ++++ 1 file changed, 4 insertions(+) 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 6c89c56ffd4..b3987e1ee1f 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -21,12 +21,16 @@ import static com.google.common.util.concurrent.Futures.immediateFuture; import static io.grpc.binder.internal.TransactionUtils.newCallerFilteringHandler; +import android.content.Context; +import android.content.pm.ServiceInfo; +import android.os.Binder; import android.os.DeadObjectException; import android.os.IBinder; import android.os.Parcel; import android.os.RemoteException; import android.os.TransactionTooLargeException; import androidx.annotation.BinderThread; +import androidx.annotation.MainThread; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Verify; import com.google.common.util.concurrent.ListenableFuture; From 9ff81d9f460f1fdf933f1d2cb824f1627c0cecec Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 19 Aug 2025 17:21:15 -0700 Subject: [PATCH 16/24] Pre-factor out the guts of the BinderClientTransport handshake # Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java --- .../src/main/java/io/grpc/binder/internal/BinderTransport.java | 1 + 1 file changed, 1 insertion(+) 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 b3987e1ee1f..b4d4502b23e 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -21,6 +21,7 @@ import static com.google.common.util.concurrent.Futures.immediateFuture; import static io.grpc.binder.internal.TransactionUtils.newCallerFilteringHandler; +import android.content.ComponentName; import android.content.Context; import android.content.pm.ServiceInfo; import android.os.Binder; From 72024dfbffea2ede29746f6e1dda9125bc8ac5de Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 19 Aug 2025 17:31:34 -0700 Subject: [PATCH 17/24] ComponentName --- .../src/main/java/io/grpc/binder/internal/BinderTransport.java | 1 - 1 file changed, 1 deletion(-) 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 b4d4502b23e..b3987e1ee1f 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -21,7 +21,6 @@ import static com.google.common.util.concurrent.Futures.immediateFuture; import static io.grpc.binder.internal.TransactionUtils.newCallerFilteringHandler; -import android.content.ComponentName; import android.content.Context; import android.content.pm.ServiceInfo; import android.os.Binder; From 0e2e9bc920ead7a0863db699700436c39b83a703 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 19 Aug 2025 17:21:15 -0700 Subject: [PATCH 18/24] Pre-factor out the guts of the BinderClientTransport handshake # Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java --- .../src/main/java/io/grpc/binder/internal/BinderTransport.java | 1 + 1 file changed, 1 insertion(+) 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 b3987e1ee1f..b4d4502b23e 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -21,6 +21,7 @@ import static com.google.common.util.concurrent.Futures.immediateFuture; import static io.grpc.binder.internal.TransactionUtils.newCallerFilteringHandler; +import android.content.ComponentName; import android.content.Context; import android.content.pm.ServiceInfo; import android.os.Binder; From b60b721c53aa62cb7072ca67fdda4dbb4f10f79f Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 19 Aug 2025 17:31:34 -0700 Subject: [PATCH 19/24] ComponentName --- .../src/main/java/io/grpc/binder/internal/BinderTransport.java | 1 - 1 file changed, 1 deletion(-) 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 b4d4502b23e..b3987e1ee1f 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -21,7 +21,6 @@ import static com.google.common.util.concurrent.Futures.immediateFuture; import static io.grpc.binder.internal.TransactionUtils.newCallerFilteringHandler; -import android.content.ComponentName; import android.content.Context; import android.content.pm.ServiceInfo; import android.os.Binder; From 883e47128de3b1fbf46581cb0a6d9a59661aa1cd Mon Sep 17 00:00:00 2001 From: jdcormie Date: Sat, 30 Aug 2025 00:03:33 +0100 Subject: [PATCH 20/24] factor out the decorate() and wrap() calls common to all handshakes --- .../internal/BinderClientTransport.java | 122 +++++++++++++----- 1 file changed, 87 insertions(+), 35 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 144ad56eec3..16a4f60ef02 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java @@ -25,6 +25,10 @@ import android.os.IBinder; import android.os.Parcel; import android.os.Process; + +import androidx.annotation.BinderThread; +import androidx.annotation.MainThread; + import com.google.common.base.Ticker; import com.google.common.util.concurrent.FutureCallback; import com.google.common.util.concurrent.Futures; @@ -146,7 +150,9 @@ void releaseExecutors() { @Override public synchronized void onBound(IBinder binder) { - sendSetupTransaction(binderDecorator.decorate(OneWayBinderProxy.wrap(binder, offloadExecutor))); + OneWayBinderProxy binderProxy = OneWayBinderProxy.wrap(binder, offloadExecutor); + binderProxy = binderDecorator.decorate(binderProxy); + handshake.onBound(binderProxy); } @Override @@ -339,23 +345,8 @@ protected void handleSetupTransport(Parcel parcel) { shutdownInternal( Status.UNAVAILABLE.withDescription("Malformed SETUP_TRANSPORT data"), true); } else { - restrictIncomingBinderToCallsFrom(remoteUid); - attributes = setSecurityAttrs(attributes, remoteUid); - authResultFuture = checkServerAuthorizationAsync(remoteUid); - Futures.addCallback( - authResultFuture, - new FutureCallback() { - @Override - public void onSuccess(Status result) { - handleAuthResult(binder, result); - } - - @Override - public void onFailure(Throwable t) { - handleAuthResult(t); - } - }, - offloadExecutor); + OneWayBinderProxy binderProxy = OneWayBinderProxy.wrap(binder, offloadExecutor); + handshake.handleSetupTransport(binderProxy); } } } @@ -366,29 +357,69 @@ private ListenableFuture checkServerAuthorizationAsync(int remoteUid) { : Futures.submit(() -> securityPolicy.checkAuthorization(remoteUid), offloadExecutor); } - private synchronized void handleAuthResult(IBinder binder, Status authorization) { - if (inState(TransportState.SETUP)) { - if (!authorization.isOk()) { - shutdownInternal(authorization, true); - } else if (!setOutgoingBinder(OneWayBinderProxy.wrap(binder, offloadExecutor))) { - shutdownInternal( - Status.UNAVAILABLE.withDescription("Failed to observe outgoing binder"), true); - } else { - // Check state again, since a failure inside setOutgoingBinder (or a callback it - // triggers), could have shut us down. - if (!isShutdown()) { - setState(TransportState.READY); - attributes = clientTransportListener.filterTransport(attributes); - clientTransportListener.transportReady(); - if (readyTimeoutFuture != null) { - readyTimeoutFuture.cancel(false); - readyTimeoutFuture = null; + class LegacyClientHandshake implements ClientHandshake { + @Override + @MainThread + @GuardedBy("BinderClientTransport.this") + public void onBound(OneWayBinderProxy binder) { + sendSetupTransaction(binder); + } + + @Override + @BinderThread + @GuardedBy("BinderClientTransport.this") + public void handleSetupTransport(OneWayBinderProxy binder) { + int remoteUid = Binder.getCallingUid(); + attributes = setSecurityAttrs(attributes, remoteUid); + authResultFuture = checkServerAuthorizationAsync(remoteUid); + Futures.addCallback( + authResultFuture, + new FutureCallback() { + @Override + public void onSuccess(Status result) { + synchronized (BinderClientTransport.this) { + handleAuthResult(binder, result); + } + } + + @Override + public void onFailure(Throwable t) { + handleAuthResult(t); + } + }, + offloadExecutor); + } + + @GuardedBy("BinderClientTransport.this") + private void handleAuthResult(OneWayBinderProxy binder, Status authorization) { + if (inState(TransportState.SETUP)) { + if (!authorization.isOk()) { + shutdownInternal(authorization, true); + } else if (!setOutgoingBinder(binder)) { + shutdownInternal( + Status.UNAVAILABLE.withDescription("Failed to observe outgoing binder"), true); + } else { + // Check state again, since a failure inside setOutgoingBinder (or a callback it + // triggers), could have shut us down. + if (!isShutdown()) { + onHandshakeComplete(); } } } } } + @GuardedBy("this") + private void onHandshakeComplete() { + setState(TransportState.READY); + attributes = clientTransportListener.filterTransport(attributes); + clientTransportListener.transportReady(); + if (readyTimeoutFuture != null) { + readyTimeoutFuture.cancel(false); + readyTimeoutFuture = null; + } + } + private synchronized void handleAuthResult(Throwable t) { shutdownInternal( Status.INTERNAL.withDescription("Could not evaluate SecurityPolicy").withCause(t), true); @@ -400,6 +431,27 @@ protected void handlePingResponse(Parcel parcel) { pingTracker.onPingResponse(parcel.readInt()); } + /** + * An abstraction of the client handshake, used to transition off a problematic legacy approach. + */ + interface ClientHandshake { + /** + * Notifies the implementation that the binding has succeeded and we are now connected to the + * server 'endpointBinder'. + */ + @GuardedBy("this") + @MainThread + void onBound(OneWayBinderProxy endpointBinder); + + /** + * Notifies the implementation that we've received a valid SETUP_TRANSPORT transaction from a + * server that can be reached at 'serverBinder'. + */ + @GuardedBy("this") + @BinderThread + void handleSetupTransport(OneWayBinderProxy serverBinder); + } + private static ClientStream newFailingClientStream( Status failure, Attributes attributes, Metadata headers, ClientStreamTracer[] tracers) { StatsTraceContext statsTraceContext = From 25df7e8edb46a62966b3b92f440dcef19d628481 Mon Sep 17 00:00:00 2001 From: jdcormie Date: Tue, 16 Sep 2025 09:45:27 -0700 Subject: [PATCH 21/24] fix imports --- .../main/java/io/grpc/binder/internal/BinderTransport.java | 4 ---- 1 file changed, 4 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 b3987e1ee1f..6c89c56ffd4 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -21,16 +21,12 @@ import static com.google.common.util.concurrent.Futures.immediateFuture; import static io.grpc.binder.internal.TransactionUtils.newCallerFilteringHandler; -import android.content.Context; -import android.content.pm.ServiceInfo; -import android.os.Binder; import android.os.DeadObjectException; import android.os.IBinder; import android.os.Parcel; import android.os.RemoteException; import android.os.TransactionTooLargeException; import androidx.annotation.BinderThread; -import androidx.annotation.MainThread; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Verify; import com.google.common.util.concurrent.ListenableFuture; From 208633559c2a1fe5008ce3dd3d5393b4acfa768e Mon Sep 17 00:00:00 2001 From: jdcormie Date: Tue, 16 Sep 2025 11:30:54 -0700 Subject: [PATCH 22/24] interface -> abstract class --- .../grpc/binder/internal/BinderClientTransport.java | 12 ++++++------ 1 file changed, 6 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 8798eba39aa..3976ef206a7 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java @@ -356,7 +356,7 @@ private ListenableFuture checkServerAuthorizationAsync(int remoteUid) { : Futures.submit(() -> securityPolicy.checkAuthorization(remoteUid), offloadExecutor); } - class LegacyClientHandshake implements ClientHandshake { + class LegacyClientHandshake extends ClientHandshake { @Override @MainThread @GuardedBy("BinderClientTransport.this") @@ -434,22 +434,22 @@ protected void handlePingResponse(Parcel parcel) { /** * An abstraction of the client handshake, used to transition off a problematic legacy approach. */ - interface ClientHandshake { + abstract class ClientHandshake { /** * Notifies the implementation that the binding has succeeded and we are now connected to the * server 'endpointBinder'. */ - @GuardedBy("this") + @GuardedBy("BinderClientTransport.this") @MainThread - void onBound(OneWayBinderProxy endpointBinder); + abstract void onBound(OneWayBinderProxy endpointBinder); /** * Notifies the implementation that we've received a valid SETUP_TRANSPORT transaction from a * server that can be reached at 'serverBinder'. */ - @GuardedBy("this") + @GuardedBy("BinderClientTransport.this") @BinderThread - void handleSetupTransport(OneWayBinderProxy serverBinder); + abstract void handleSetupTransport(OneWayBinderProxy serverBinder); } private static ClientStream newFailingClientStream( From d53fe91df568fd7e6e8ca0a2ede4a414ca61cf89 Mon Sep 17 00:00:00 2001 From: jdcormie Date: Tue, 16 Sep 2025 11:41:36 -0700 Subject: [PATCH 23/24] sync comments --- .../java/io/grpc/binder/internal/BinderClientTransport.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 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 3976ef206a7..d6d5f8c977c 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java @@ -432,12 +432,14 @@ protected void handlePingResponse(Parcel parcel) { } /** - * An abstraction of the client handshake, used to transition off a problematic legacy approach. + * A base for all implementations of the client handshake. + * + *

Supports a clean migration away from the legacy approach, one client at a time. */ abstract class ClientHandshake { /** * Notifies the implementation that the binding has succeeded and we are now connected to the - * server 'endpointBinder'. + * server's "endpoint" which can be reached at 'endpointBinder'. */ @GuardedBy("BinderClientTransport.this") @MainThread From 01f46c784f36ccedc2540e58014836d9820b26ea Mon Sep 17 00:00:00 2001 From: jdcormie Date: Thu, 2 Oct 2025 10:43:56 -0700 Subject: [PATCH 24/24] address review comments Make ClientHandshake an interface Make the impl private/final --- .../binder/internal/BinderClientTransport.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 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 d6d5f8c977c..c7d954db1cc 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java @@ -73,6 +73,8 @@ public final class BinderClientTransport extends BinderTransport private final Executor offloadExecutor; private final SecurityPolicy securityPolicy; private final Bindable serviceBinding; + + @GuardedBy("this") private final ClientHandshake handshake; /** Number of ongoing calls which keep this transport "in-use". */ @@ -356,17 +358,17 @@ private ListenableFuture checkServerAuthorizationAsync(int remoteUid) { : Futures.submit(() -> securityPolicy.checkAuthorization(remoteUid), offloadExecutor); } - class LegacyClientHandshake extends ClientHandshake { + private final class LegacyClientHandshake implements ClientHandshake { @Override @MainThread - @GuardedBy("BinderClientTransport.this") + @GuardedBy("BinderClientTransport.this") // By way of @GuardedBy("this") `handshake` member. public void onBound(OneWayBinderProxy binder) { sendSetupTransaction(binder); } @Override @BinderThread - @GuardedBy("BinderClientTransport.this") + @GuardedBy("BinderClientTransport.this") // By way of @GuardedBy("this") `handshake` member. public void handleSetupTransport(OneWayBinderProxy binder) { int remoteUid = Binder.getCallingUid(); restrictIncomingBinderToCallsFrom(remoteUid); @@ -436,22 +438,20 @@ protected void handlePingResponse(Parcel parcel) { * *

Supports a clean migration away from the legacy approach, one client at a time. */ - abstract class ClientHandshake { + private interface ClientHandshake { /** * Notifies the implementation that the binding has succeeded and we are now connected to the * server's "endpoint" which can be reached at 'endpointBinder'. */ - @GuardedBy("BinderClientTransport.this") @MainThread - abstract void onBound(OneWayBinderProxy endpointBinder); + void onBound(OneWayBinderProxy endpointBinder); /** * Notifies the implementation that we've received a valid SETUP_TRANSPORT transaction from a * server that can be reached at 'serverBinder'. */ - @GuardedBy("BinderClientTransport.this") @BinderThread - abstract void handleSetupTransport(OneWayBinderProxy serverBinder); + void handleSetupTransport(OneWayBinderProxy serverBinder); } private static ClientStream newFailingClientStream(