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