From bd7fcab8bfb4c82943a063f77c2674dc916b745a Mon Sep 17 00:00:00 2001 From: John Cormie Date: Wed, 27 Aug 2025 10:20:54 +0100 Subject: [PATCH 1/3] Don't let a second SETUP_TRANSPORT change the REMOTE_UID attribute. --- .../internal/BinderClientTransport.java | 51 ++++++----- .../RobolectricBinderTransportTest.java | 88 +++++++++++++++---- .../grpc/internal/AbstractTransportTest.java | 2 +- 3 files changed, 95 insertions(+), 46 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..c87b2cd9c45 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java @@ -330,32 +330,31 @@ void notifyTerminated() { @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(); - 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 { - 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); - } + 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 if (authResultFuture != null) { + shutdownInternal(Status.UNAVAILABLE.withDescription("Too many SETUP_TRANSPORTs"), true); + } else { + 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); } } 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 7275c47d51c..6b65231a938 100644 --- a/binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java +++ b/binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java @@ -16,44 +16,37 @@ package io.grpc.binder.internal; +import static android.os.Process.myUid; import static com.google.common.truth.Truth.assertThat; -import static java.util.concurrent.TimeUnit.MILLISECONDS; import static org.mockito.Mockito.never; import static org.mockito.Mockito.timeout; import static org.mockito.Mockito.verify; import static org.robolectric.Shadows.shadowOf; +import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static io.grpc.binder.internal.BinderTransport.REMOTE_UID; +import static io.grpc.binder.internal.BinderTransport.SETUP_TRANSPORT; +import static io.grpc.binder.internal.BinderTransport.WIRE_FORMAT_VERSION; import android.app.Application; import android.content.Intent; import android.content.pm.ApplicationInfo; import android.content.pm.PackageInfo; import android.content.pm.ServiceInfo; +import android.os.Binder; +import android.os.Parcel; + import androidx.test.core.app.ApplicationProvider; import androidx.test.core.content.pm.ApplicationInfoBuilder; import androidx.test.core.content.pm.PackageInfoBuilder; + import com.google.common.collect.ImmutableList; -import io.grpc.Attributes; -import io.grpc.ServerStreamTracer; -import io.grpc.Status; -import io.grpc.binder.AndroidComponentAddress; -import io.grpc.binder.ApiConstants; -import io.grpc.binder.AsyncSecurityPolicy; -import io.grpc.binder.internal.SettableAsyncSecurityPolicy.AuthRequest; -import io.grpc.internal.AbstractTransportTest; -import io.grpc.internal.ClientTransportFactory.ClientTransportOptions; -import io.grpc.internal.GrpcUtil; -import io.grpc.internal.InternalServer; -import io.grpc.internal.ManagedClientTransport; -import io.grpc.internal.ObjectPool; -import io.grpc.internal.SharedResourcePool; -import java.util.List; -import java.util.concurrent.Executor; -import java.util.concurrent.ScheduledExecutorService; + import org.junit.Before; import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.junit.MockitoJUnit; import org.mockito.junit.MockitoRule; @@ -64,6 +57,27 @@ import org.robolectric.annotation.LooperMode.Mode; import org.robolectric.shadows.ShadowBinder; +import java.util.List; +import java.util.concurrent.Executor; +import java.util.concurrent.ScheduledExecutorService; + +import io.grpc.Attributes; +import io.grpc.ServerStreamTracer; +import io.grpc.Status; +import io.grpc.binder.AndroidComponentAddress; +import io.grpc.binder.ApiConstants; +import io.grpc.binder.AsyncSecurityPolicy; +import io.grpc.binder.SecurityPolicies; +import io.grpc.binder.internal.SettableAsyncSecurityPolicy.AuthRequest; +import io.grpc.internal.AbstractTransportTest; +import io.grpc.internal.ClientTransportFactory.ClientTransportOptions; +import io.grpc.internal.ConnectionClientTransport; +import io.grpc.internal.GrpcUtil; +import io.grpc.internal.InternalServer; +import io.grpc.internal.ManagedClientTransport; +import io.grpc.internal.ObjectPool; +import io.grpc.internal.SharedResourcePool; + /** * All of the AbstractTransportTest cases applied to {@link BinderTransport} running in a * Robolectric environment. @@ -109,7 +123,7 @@ public static ImmutableList data() { public void setUp() { serverAppInfo = ApplicationInfoBuilder.newBuilder().setPackageName("the.server.package").build(); - serverAppInfo.uid = android.os.Process.myUid(); + serverAppInfo.uid = myUid(); serverPkgInfo = PackageInfoBuilder.newBuilder() .setPackageName(serverAppInfo.packageName) @@ -264,6 +278,42 @@ public void eagAttributeCanOverrideChannelPreAuthServerSetting() throws Exceptio verify(mockClientTransportListener, timeout(TIMEOUT_MS)).transportReady(); } + @Test + public void clientIgnoresDuplicateSetupTransaction() throws Exception { + server.start(serverListener); + client = + newClientTransportBuilder() + .setFactory( + newClientTransportFactoryBuilder() + .setSecurityPolicy(SecurityPolicies.internalOnly()) + .buildClientTransportFactory()) + .build(); + runIfNotNull(client.start(mockClientTransportListener)); + verify(mockClientTransportListener, timeout(TIMEOUT_MS)).transportReady(); + + assertThat(((ConnectionClientTransport) client).getAttributes().get(REMOTE_UID)) + .isEqualTo(myUid()); + + Parcel setupParcel = Parcel.obtain(); + try { + setupParcel.writeInt(WIRE_FORMAT_VERSION); + setupParcel.writeStrongBinder(new Binder()); + setupParcel.setDataPosition(0); + ShadowBinder.setCallingUid(1 + myUid()); + ((BinderClientTransport) client).handleTransaction(SETUP_TRANSPORT, setupParcel); + } finally { + ShadowBinder.setCallingUid(myUid()); + setupParcel.recycle(); + } + + assertThat(((ConnectionClientTransport) client).getAttributes().get(REMOTE_UID)) + .isEqualTo(myUid()); + + ArgumentCaptor statusCaptor = ArgumentCaptor.forClass(Status.class); + verify(mockClientTransportListener, timeout(TIMEOUT_MS)).transportShutdown(statusCaptor.capture()); + assertCodeEquals(null, Status.UNAVAILABLE, statusCaptor.getValue()); + } + @Test @Ignore("See BinderTransportTest#socketStats.") @Override diff --git a/core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java b/core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java index c60174b2faf..0d8ee6a966f 100644 --- a/core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java +++ b/core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java @@ -2114,7 +2114,7 @@ methodDescriptor, new Metadata(), callOptions, * Only assert that the Status.Code matches, but provide the entire actual result in case the * assertion fails. */ - private static void assertCodeEquals(String message, Status expected, Status actual) { + protected static void assertCodeEquals(String message, Status expected, Status actual) { if (expected == null) { fail("expected should not be null"); } From 3c64363af57636c4a18d313cbca81be7ad4a3b42 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Wed, 27 Aug 2025 19:02:35 +0100 Subject: [PATCH 2/3] back off --- .../internal/BinderClientTransport.java | 51 +++++++++--------- .../RobolectricBinderTransportTest.java | 52 ++++++++----------- 2 files changed, 47 insertions(+), 56 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 c87b2cd9c45..82c9e17b871 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java @@ -330,31 +330,32 @@ void notifyTerminated() { @GuardedBy("this") protected void handleSetupTransport(Parcel parcel) { int remoteUid = Binder.getCallingUid(); - 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 if (authResultFuture != null) { - shutdownInternal(Status.UNAVAILABLE.withDescription("Too many SETUP_TRANSPORTs"), true); - } else { - 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); + 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 { + 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); + } } } 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 6b65231a938..8f1209f389b 100644 --- a/binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java +++ b/binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java @@ -18,14 +18,14 @@ import static android.os.Process.myUid; import static com.google.common.truth.Truth.assertThat; +import static io.grpc.binder.internal.BinderTransport.REMOTE_UID; +import static io.grpc.binder.internal.BinderTransport.SETUP_TRANSPORT; +import static io.grpc.binder.internal.BinderTransport.WIRE_FORMAT_VERSION; +import static java.util.concurrent.TimeUnit.MILLISECONDS; import static org.mockito.Mockito.never; import static org.mockito.Mockito.timeout; import static org.mockito.Mockito.verify; import static org.robolectric.Shadows.shadowOf; -import static java.util.concurrent.TimeUnit.MILLISECONDS; -import static io.grpc.binder.internal.BinderTransport.REMOTE_UID; -import static io.grpc.binder.internal.BinderTransport.SETUP_TRANSPORT; -import static io.grpc.binder.internal.BinderTransport.WIRE_FORMAT_VERSION; import android.app.Application; import android.content.Intent; @@ -34,33 +34,10 @@ import android.content.pm.ServiceInfo; import android.os.Binder; import android.os.Parcel; - import androidx.test.core.app.ApplicationProvider; import androidx.test.core.content.pm.ApplicationInfoBuilder; import androidx.test.core.content.pm.PackageInfoBuilder; - import com.google.common.collect.ImmutableList; - -import org.junit.Before; -import org.junit.Ignore; -import org.junit.Rule; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.mockito.ArgumentCaptor; -import org.mockito.Mock; -import org.mockito.junit.MockitoJUnit; -import org.mockito.junit.MockitoRule; -import org.robolectric.ParameterizedRobolectricTestRunner; -import org.robolectric.ParameterizedRobolectricTestRunner.Parameter; -import org.robolectric.ParameterizedRobolectricTestRunner.Parameters; -import org.robolectric.annotation.LooperMode; -import org.robolectric.annotation.LooperMode.Mode; -import org.robolectric.shadows.ShadowBinder; - -import java.util.List; -import java.util.concurrent.Executor; -import java.util.concurrent.ScheduledExecutorService; - import io.grpc.Attributes; import io.grpc.ServerStreamTracer; import io.grpc.Status; @@ -77,6 +54,23 @@ import io.grpc.internal.ManagedClientTransport; import io.grpc.internal.ObjectPool; import io.grpc.internal.SharedResourcePool; +import java.util.List; +import java.util.concurrent.Executor; +import java.util.concurrent.ScheduledExecutorService; +import org.junit.Before; +import org.junit.Ignore; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnit; +import org.mockito.junit.MockitoRule; +import org.robolectric.ParameterizedRobolectricTestRunner; +import org.robolectric.ParameterizedRobolectricTestRunner.Parameter; +import org.robolectric.ParameterizedRobolectricTestRunner.Parameters; +import org.robolectric.annotation.LooperMode; +import org.robolectric.annotation.LooperMode.Mode; +import org.robolectric.shadows.ShadowBinder; /** * All of the AbstractTransportTest cases applied to {@link BinderTransport} running in a @@ -308,10 +302,6 @@ public void clientIgnoresDuplicateSetupTransaction() throws Exception { assertThat(((ConnectionClientTransport) client).getAttributes().get(REMOTE_UID)) .isEqualTo(myUid()); - - ArgumentCaptor statusCaptor = ArgumentCaptor.forClass(Status.class); - verify(mockClientTransportListener, timeout(TIMEOUT_MS)).transportShutdown(statusCaptor.capture()); - assertCodeEquals(null, Status.UNAVAILABLE, statusCaptor.getValue()); } @Test From e2fcff8b74d0b5b5c3782cb1111644d56707572a Mon Sep 17 00:00:00 2001 From: John Cormie Date: Wed, 27 Aug 2025 19:08:51 +0100 Subject: [PATCH 3/3] protected --- .../java/io/grpc/internal/AbstractTransportTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java b/core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java index 0d8ee6a966f..c60174b2faf 100644 --- a/core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java +++ b/core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java @@ -2114,7 +2114,7 @@ methodDescriptor, new Metadata(), callOptions, * Only assert that the Status.Code matches, but provide the entire actual result in case the * assertion fails. */ - protected static void assertCodeEquals(String message, Status expected, Status actual) { + private static void assertCodeEquals(String message, Status expected, Status actual) { if (expected == null) { fail("expected should not be null"); }