From b9745a26443c820036acdd77827125795e84a291 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Fri, 24 Oct 2025 15:50:39 -0700 Subject: [PATCH] Let the server know when the client fails to authorize it. Avoids handshake timeout on the server side in this case. Also has the nice side effect that the client handshake code doesn't have to pass the server's binder from async function to function as an argument. --- .../internal/BinderClientTransportTest.java | 15 ++++ .../internal/BinderClientTransport.java | 28 +++---- .../RobolectricBinderTransportTest.java | 24 ++++++ .../java/io/grpc/binder/FakeDeadBinder.java | 74 +++++++++++++++++++ 4 files changed, 124 insertions(+), 17 deletions(-) create mode 100644 binder/src/testFixtures/java/io/grpc/binder/FakeDeadBinder.java diff --git a/binder/src/androidTest/java/io/grpc/binder/internal/BinderClientTransportTest.java b/binder/src/androidTest/java/io/grpc/binder/internal/BinderClientTransportTest.java index 0038a054854..ee28f132f79 100644 --- a/binder/src/androidTest/java/io/grpc/binder/internal/BinderClientTransportTest.java +++ b/binder/src/androidTest/java/io/grpc/binder/internal/BinderClientTransportTest.java @@ -39,6 +39,7 @@ import io.grpc.Status.Code; import io.grpc.binder.AndroidComponentAddress; import io.grpc.binder.BinderServerBuilder; +import io.grpc.binder.FakeDeadBinder; import io.grpc.binder.HostServices; import io.grpc.binder.SecurityPolicy; import io.grpc.binder.internal.OneWayBinderProxies.BlackHoleOneWayBinderProxy; @@ -358,6 +359,20 @@ public void testTxnFailurePostSetup() throws Exception { assertThat(streamStatus.getCause()).isSameInstanceAs(doe); } + @Test + public void testServerBinderDeadOnArrival() throws Exception { + BlockingBinderDecorator decorator = new BlockingBinderDecorator<>(); + transport = new BinderClientTransportBuilder().setBinderDecorator(decorator).build(); + transport.start(transportListener).run(); + decorator.putNextResult(decorator.takeNextRequest()); // Server's "Endpoint" Binder. + OneWayBinderProxy unusedServerBinder = decorator.takeNextRequest(); + decorator.putNextResult( + OneWayBinderProxy.wrap(new FakeDeadBinder(), offloadServicePool.getObject())); + Status clientStatus = transportListener.awaitShutdown(); + assertThat(clientStatus.getCode()).isEqualTo(Code.UNAVAILABLE); + assertThat(clientStatus.getDescription()).contains("Failed to observe outgoing binder"); + } + @Test public void testBlackHoleEndpointConnectTimeout() throws Exception { BlockingBinderDecorator decorator = new BlockingBinderDecorator<>(); 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 c2447daf751..c6aa195544e 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java @@ -87,7 +87,6 @@ public final class BinderClientTransport extends BinderTransport @GuardedBy("this") private ScheduledFuture readyTimeoutFuture; // != null iff timeout scheduled. - /** * Constructs a new transport instance. * @@ -324,6 +323,9 @@ protected void handleSetupTransport(Parcel parcel) { } else if (binder == null) { shutdownInternal( Status.UNAVAILABLE.withDescription("Malformed SETUP_TRANSPORT data"), true); + } else if (!setOutgoingBinder(OneWayBinderProxy.wrap(binder, offloadExecutor))) { + shutdownInternal( + Status.UNAVAILABLE.withDescription("Failed to observe outgoing binder"), true); } else { restrictIncomingBinderToCallsFrom(remoteUid); attributes = setSecurityAttrs(attributes, remoteUid); @@ -334,7 +336,7 @@ protected void handleSetupTransport(Parcel parcel) { new FutureCallback() { @Override public void onSuccess(Status result) { - handleAuthResult(binder, result); + handleAuthResult(result); } @Override @@ -353,24 +355,17 @@ private ListenableFuture checkServerAuthorizationAsync(int remoteUid) { : Futures.submit(() -> securityPolicy.checkAuthorization(remoteUid), offloadExecutor); } - private synchronized void handleAuthResult(IBinder binder, Status authorization) { + private synchronized void handleAuthResult(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; - } + setState(TransportState.READY); + attributes = clientTransportListener.filterTransport(attributes); + clientTransportListener.transportReady(); + if (readyTimeoutFuture != null) { + readyTimeoutFuture.cancel(false); + readyTimeoutFuture = null; } } } @@ -387,7 +382,6 @@ protected void handlePingResponse(Parcel parcel) { pingTracker.onPingResponse(parcel.readInt()); } - private static ClientStream newFailingClientStream( Status failure, Attributes attributes, Metadata headers, ClientStreamTracer[] tracers) { StatsTraceContext statsTraceContext = diff --git a/binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java b/binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java index d3d73f0e9eb..6beb92c1691 100644 --- a/binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java +++ b/binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java @@ -43,6 +43,7 @@ import androidx.test.core.content.pm.ApplicationInfoBuilder; import androidx.test.core.content.pm.PackageInfoBuilder; import com.google.common.collect.ImmutableList; +import com.google.common.truth.TruthJUnit; import io.grpc.Attributes; import io.grpc.InternalChannelz.SocketStats; import io.grpc.ServerStreamTracer; @@ -353,6 +354,29 @@ static void sendShutdownTransportTransactionAsUid(ClientTransport client, int se } } + @Test + public void clientReportsAuthzErrorToServer() throws Exception { + server.start(serverListener); + client = + newClientTransportBuilder() + .setFactory( + newClientTransportFactoryBuilder() + .setSecurityPolicy(SecurityPolicies.permissionDenied("test")) + .buildClientTransportFactory()) + .build(); + runIfNotNull(client.start(mockClientTransportListener)); + verify(mockClientTransportListener, timeout(TIMEOUT_MS)) + .transportShutdown(statusCaptor.capture()); + assertThat(statusCaptor.getValue().getCode()).isEqualTo(Status.Code.PERMISSION_DENIED); + + TruthJUnit.assume().that(preAuthServersParam).isFalse(); + + MockServerTransportListener serverTransportListener = + serverListener.takeListenerOrFail(TIMEOUT_MS, MILLISECONDS); + serverTransportListener.waitForTermination(TIMEOUT_MS, MILLISECONDS); + assertThat(serverTransportListener.isTerminated()).isTrue(); + } + @Test @Override // We don't quite pass the official/abstract version of this test yet because diff --git a/binder/src/testFixtures/java/io/grpc/binder/FakeDeadBinder.java b/binder/src/testFixtures/java/io/grpc/binder/FakeDeadBinder.java new file mode 100644 index 00000000000..b1658c13812 --- /dev/null +++ b/binder/src/testFixtures/java/io/grpc/binder/FakeDeadBinder.java @@ -0,0 +1,74 @@ +/* + * Copyright 2025 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.binder; + +import android.os.DeadObjectException; +import android.os.IBinder; +import android.os.IInterface; +import android.os.Parcel; +import android.os.RemoteException; +import java.io.FileDescriptor; + +/** An {@link IBinder} that behaves as if its hosting process has died, for testing. */ +public class FakeDeadBinder implements IBinder { + @Override + public boolean isBinderAlive() { + return false; + } + + @Override + public IInterface queryLocalInterface(String descriptor) { + return null; + } + + @Override + public String getInterfaceDescriptor() throws RemoteException { + throw new DeadObjectException(); + } + + @Override + public boolean pingBinder() { + return false; + } + + @Override + public void dump(FileDescriptor fd, String[] args) throws RemoteException { + throw new DeadObjectException(); + } + + @Override + public void dumpAsync(FileDescriptor fd, String[] args) throws RemoteException { + throw new DeadObjectException(); + } + + @Override + public boolean transact(int code, Parcel data, Parcel reply, int flags) throws RemoteException { + throw new DeadObjectException(); + } + + @Override + public void linkToDeath(DeathRecipient r, int flags) throws RemoteException { + throw new DeadObjectException(); + } + + @Override + public boolean unlinkToDeath(DeathRecipient deathRecipient, int flags) { + // No need to check whether 'deathRecipient' was ever actually passed to linkToDeath(): Per our + // API contract, if "the IBinder has already died" we never throw and always return false. + return false; + } +}