From 9bf3caedf4a60c784762341459d4a7d9c482f9a8 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Fri, 22 May 2026 17:02:10 -0700 Subject: [PATCH 1/3] binder: Refactor BinderServerTransport creation to use a Builder Makes it easier to add new arguments with defaults in the future. TAG=agy CONV=e849839d-3cdc-4bcc-863d-9c8b6d2f47f6 --- .../io/grpc/binder/internal/BinderServer.java | 13 ++-- .../internal/BinderServerTransport.java | 59 ++++++++++++++++--- .../internal/BinderServerTransportTest.java | 44 +------------- 3 files changed, 60 insertions(+), 56 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderServer.java b/binder/src/main/java/io/grpc/binder/internal/BinderServer.java index f913775fcbe..6d1a4eff059 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderServer.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderServer.java @@ -181,12 +181,13 @@ public synchronized boolean handleTransaction(int code, Parcel parcel) { checkNotNull(executor, "Not started?")); // Create a new transport and let our listener know about it. BinderServerTransport transport = - BinderServerTransport.create( - executorServicePool, - attrsBuilder.build(), - streamTracerFactories, - clientBinderDecorator, - callbackBinder); + new BinderServerTransport.Builder() + .setExecutorServicePool(executorServicePool) + .setAttributes(attrsBuilder.build()) + .setStreamTracerFactories(streamTracerFactories) + .setBinderDecorator(clientBinderDecorator) + .setCallbackBinder(callbackBinder) + .build(); transport.start(listener.transportCreated(transport)); return true; } diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderServerTransport.java b/binder/src/main/java/io/grpc/binder/internal/BinderServerTransport.java index 784d833bdf5..56834ec62d6 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderServerTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderServerTransport.java @@ -15,7 +15,10 @@ */ package io.grpc.binder.internal; +import static com.google.common.base.Preconditions.checkNotNull; + import android.os.IBinder; +import com.google.common.collect.ImmutableList; import com.google.errorprone.annotations.concurrent.GuardedBy; import io.grpc.Attributes; import io.grpc.Grpc; @@ -56,21 +59,21 @@ private BinderServerTransport( * * @param binderDecorator used to decorate 'callbackBinder', for fault injection. */ - public static BinderServerTransport create( - ObjectPool executorServicePool, - Attributes attributes, - List streamTracerFactories, - OneWayBinderProxy.Decorator binderDecorator, - IBinder callbackBinder) { + private static BinderServerTransport create(Builder builder) { BinderServerTransport transport = new BinderServerTransport( - executorServicePool, attributes, streamTracerFactories, binderDecorator); + checkNotNull(builder.executorServicePool, "executorServicePool"), + builder.attributes, + builder.streamTracerFactories, + builder.binderDecorator); // TODO(jdcormie): Plumb in the Server's executor() and use it here instead. // No need to handle failure here because if 'callbackBinder' is already dead, we'll notice it // again in start() when we send the first transaction. synchronized (transport) { transport.setOutgoingBinder( - OneWayBinderProxy.wrap(callbackBinder, transport.getScheduledExecutorService())); + OneWayBinderProxy.wrap( + checkNotNull(builder.callbackBinder, "callbackBinder"), + transport.getScheduledExecutorService())); } return transport; } @@ -154,4 +157,44 @@ private static InternalLogId buildLogId(Attributes attributes) { return InternalLogId.allocate( BinderServerTransport.class, "from " + attributes.get(Grpc.TRANSPORT_ATTR_REMOTE_ADDR)); } + + /** Builder for {@link BinderServerTransport} instances. */ + public static final class Builder { + private ObjectPool executorServicePool; + private Attributes attributes = Attributes.EMPTY; + private List streamTracerFactories = ImmutableList.of(); + private OneWayBinderProxy.Decorator binderDecorator = OneWayBinderProxy.IDENTITY_DECORATOR; + private IBinder callbackBinder; + + public Builder() {} + + public Builder setExecutorServicePool(ObjectPool executorServicePool) { + this.executorServicePool = checkNotNull(executorServicePool, "executorServicePool"); + return this; + } + + public Builder setAttributes(Attributes attributes) { + this.attributes = checkNotNull(attributes, "attributes"); + return this; + } + + public Builder setStreamTracerFactories(List streamTracerFactories) { + this.streamTracerFactories = checkNotNull(streamTracerFactories, "streamTracerFactories"); + return this; + } + + public Builder setBinderDecorator(OneWayBinderProxy.Decorator binderDecorator) { + this.binderDecorator = checkNotNull(binderDecorator, "binderDecorator"); + return this; + } + + public Builder setCallbackBinder(IBinder callbackBinder) { + this.callbackBinder = checkNotNull(callbackBinder, "callbackBinder"); + return this; + } + + public BinderServerTransport build() { + return BinderServerTransport.create(this); + } + } } diff --git a/binder/src/test/java/io/grpc/binder/internal/BinderServerTransportTest.java b/binder/src/test/java/io/grpc/binder/internal/BinderServerTransportTest.java index d261ce43c8c..681cfc548d2 100644 --- a/binder/src/test/java/io/grpc/binder/internal/BinderServerTransportTest.java +++ b/binder/src/test/java/io/grpc/binder/internal/BinderServerTransportTest.java @@ -68,8 +68,8 @@ public void setUp() throws Exception { } // Provide defaults so that we can "include only relevant details in tests." - BinderServerTransportBuilder newBinderServerTransportBuilder() { - return new BinderServerTransportBuilder() + BinderServerTransport.Builder newBinderServerTransportBuilder() { + return new BinderServerTransport.Builder() .setExecutorServicePool(new FixedObjectPool<>(executorService)) .setAttributes(Attributes.EMPTY) .setStreamTracerFactories(ImmutableList.of()) @@ -126,44 +126,4 @@ public void testStartAfterShutdownNoIdle() throws Exception { assertThat(transportListener.isTerminated()).isTrue(); } - static class BinderServerTransportBuilder { - ObjectPool executorServicePool; - Attributes attributes; - List streamTracerFactories; - OneWayBinderProxy.Decorator binderDecorator; - IBinder callbackBinder; - - public BinderServerTransport build() { - return BinderServerTransport.create( - executorServicePool, attributes, streamTracerFactories, binderDecorator, callbackBinder); - } - - public BinderServerTransportBuilder setExecutorServicePool( - ObjectPool executorServicePool) { - this.executorServicePool = executorServicePool; - return this; - } - - public BinderServerTransportBuilder setAttributes(Attributes attributes) { - this.attributes = attributes; - return this; - } - - public BinderServerTransportBuilder setStreamTracerFactories( - List streamTracerFactories) { - this.streamTracerFactories = streamTracerFactories; - return this; - } - - public BinderServerTransportBuilder setBinderDecorator( - OneWayBinderProxy.Decorator binderDecorator) { - this.binderDecorator = binderDecorator; - return this; - } - - public BinderServerTransportBuilder setCallbackBinder(IBinder callbackBinder) { - this.callbackBinder = callbackBinder; - return this; - } - } } From bc98e4382e61c99a25f76b2d8429c1e8e439d848 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Fri, 22 May 2026 17:03:16 -0700 Subject: [PATCH 2/3] binder: Give tests a way to monkey with incoming transactions too. LeakSafeOneWayBinder.Decorator lets us intercept Binder transactions on the receiving side for testing. Unlike the existing OneWayBinderProxy Decorator (works on the sending side), this lets a test inject code on the same thread that calls onTransact(). We'll use this in a follow up commit to simulate the UID of the caller. TAG=agy CONV=e849839d-3cdc-4bcc-863d-9c8b6d2f47f6 --- .../internal/BinderClientTransport.java | 1 + .../BinderClientTransportFactory.java | 11 +++++++++++ .../internal/BinderServerTransport.java | 19 ++++++++++++++++--- .../grpc/binder/internal/BinderTransport.java | 4 +++- .../binder/internal/LeakSafeOneWayBinder.java | 11 +++++++++++ 5 files changed, 42 insertions(+), 4 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 58e7d7e2b31..a53b698b7ee 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java @@ -112,6 +112,7 @@ public BinderClientTransport( targetAddress, factory.inboundParcelablePolicy), factory.binderDecorator, + factory.inboundBinderDecorator, buildLogId(factory.sourceContext, targetAddress)); this.offloadExecutorPool = factory.offloadExecutorPool; this.securityPolicy = factory.securityPolicy; diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransportFactory.java b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransportFactory.java index 459e064ad9b..9c9f8bd7723 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransportFactory.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransportFactory.java @@ -51,6 +51,7 @@ public final class BinderClientTransportFactory implements ClientTransportFactor final BindServiceFlags bindServiceFlags; final InboundParcelablePolicy inboundParcelablePolicy; final OneWayBinderProxy.Decorator binderDecorator; + final LeakSafeOneWayBinder.Decorator inboundBinderDecorator; final long readyTimeoutMillis; final boolean preAuthorizeServers; // TODO(jdcormie): Default to true. final boolean useLegacyAuthStrategy; @@ -72,6 +73,7 @@ private BinderClientTransportFactory(Builder builder) { bindServiceFlags = checkNotNull(builder.bindServiceFlags); inboundParcelablePolicy = checkNotNull(builder.inboundParcelablePolicy); binderDecorator = checkNotNull(builder.binderDecorator); + inboundBinderDecorator = checkNotNull(builder.inboundBinderDecorator); readyTimeoutMillis = builder.readyTimeoutMillis; preAuthorizeServers = builder.preAuthorizeServers; useLegacyAuthStrategy = builder.useLegacyAuthStrategy; @@ -126,6 +128,7 @@ public static final class Builder implements ClientTransportFactoryBuilder { BindServiceFlags bindServiceFlags = BindServiceFlags.DEFAULTS; InboundParcelablePolicy inboundParcelablePolicy = InboundParcelablePolicy.DEFAULT; OneWayBinderProxy.Decorator binderDecorator = OneWayBinderProxy.IDENTITY_DECORATOR; + LeakSafeOneWayBinder.Decorator inboundBinderDecorator = LeakSafeOneWayBinder.IDENTITY_DECORATOR; long readyTimeoutMillis = 60_000; boolean preAuthorizeServers; boolean useLegacyAuthStrategy = true; // TODO(jdcormie): Default to false. @@ -191,6 +194,14 @@ public Builder setBinderDecorator(OneWayBinderProxy.Decorator binderDecorator) { return this; } + /** + * Decorates the incoming binder, for fault injection. + */ + public Builder setInboundBinderDecorator(LeakSafeOneWayBinder.Decorator inboundBinderDecorator) { + this.inboundBinderDecorator = checkNotNull(inboundBinderDecorator, "inboundBinderDecorator"); + return this; + } + /** * Limits how long it can take to for a new transport to become ready after being started. * diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderServerTransport.java b/binder/src/main/java/io/grpc/binder/internal/BinderServerTransport.java index 56834ec62d6..90ba317b25f 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderServerTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderServerTransport.java @@ -49,8 +49,14 @@ private BinderServerTransport( ObjectPool executorServicePool, Attributes attributes, List streamTracerFactories, - OneWayBinderProxy.Decorator binderDecorator) { - super(executorServicePool, attributes, binderDecorator, buildLogId(attributes)); + OneWayBinderProxy.Decorator binderDecorator, + LeakSafeOneWayBinder.Decorator inboundBinderDecorator) { + super( + executorServicePool, + attributes, + binderDecorator, + inboundBinderDecorator, + buildLogId(attributes)); this.streamTracerFactories = streamTracerFactories; } @@ -65,7 +71,8 @@ private static BinderServerTransport create(Builder builder) { checkNotNull(builder.executorServicePool, "executorServicePool"), builder.attributes, builder.streamTracerFactories, - builder.binderDecorator); + builder.binderDecorator, + builder.inboundBinderDecorator); // TODO(jdcormie): Plumb in the Server's executor() and use it here instead. // No need to handle failure here because if 'callbackBinder' is already dead, we'll notice it // again in start() when we send the first transaction. @@ -164,6 +171,7 @@ public static final class Builder { private Attributes attributes = Attributes.EMPTY; private List streamTracerFactories = ImmutableList.of(); private OneWayBinderProxy.Decorator binderDecorator = OneWayBinderProxy.IDENTITY_DECORATOR; + private LeakSafeOneWayBinder.Decorator inboundBinderDecorator = LeakSafeOneWayBinder.IDENTITY_DECORATOR; private IBinder callbackBinder; public Builder() {} @@ -188,6 +196,11 @@ public Builder setBinderDecorator(OneWayBinderProxy.Decorator binderDecorator) { return this; } + public Builder setInboundBinderDecorator(LeakSafeOneWayBinder.Decorator inboundBinderDecorator) { + this.inboundBinderDecorator = checkNotNull(inboundBinderDecorator, "inboundBinderDecorator"); + return this; + } + public Builder setCallbackBinder(IBinder callbackBinder) { this.callbackBinder = checkNotNull(callbackBinder, "callbackBinder"); return this; 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 2b7aa97bfd9..61494aadc7d 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -198,13 +198,15 @@ protected BinderTransport( ObjectPool executorServicePool, Attributes attributes, OneWayBinderProxy.Decorator binderDecorator, + LeakSafeOneWayBinder.Decorator inboundBinderDecorator, InternalLogId logId) { this.binderDecorator = binderDecorator; this.executorServicePool = executorServicePool; this.attributes = attributes; this.logId = logId; scheduledExecutorService = executorServicePool.getObject(); - incomingBinder = new LeakSafeOneWayBinder(this::handleTransaction); + incomingBinder = + new LeakSafeOneWayBinder(inboundBinderDecorator.decorate(this::handleTransaction)); ongoingCalls = new ConcurrentHashMap<>(); flowController = new FlowController(TRANSACTION_BYTES_WINDOW); } diff --git a/binder/src/main/java/io/grpc/binder/internal/LeakSafeOneWayBinder.java b/binder/src/main/java/io/grpc/binder/internal/LeakSafeOneWayBinder.java index c36bc7d5bd3..1a067fc59bc 100644 --- a/binder/src/main/java/io/grpc/binder/internal/LeakSafeOneWayBinder.java +++ b/binder/src/main/java/io/grpc/binder/internal/LeakSafeOneWayBinder.java @@ -63,6 +63,17 @@ public interface TransactionHandler { boolean handleTransaction(int code, Parcel data); } + /** + * Decorates a {@link TransactionHandler} to add behavior. + */ + @Internal + public interface Decorator { + TransactionHandler decorate(TransactionHandler input); + } + + /** A {@link Decorator} that does nothing. */ + public static final Decorator IDENTITY_DECORATOR = (x) -> x; + @Nullable private TransactionHandler handler; public LeakSafeOneWayBinder(TransactionHandler handler) { From feaf99818acf1d203da387b338ce479d6deab3e3 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Fri, 22 May 2026 17:04:00 -0700 Subject: [PATCH 3/3] binder: Add Robolectric UID propagation test helpers Add UID faking decorators using the new test support plumbing. This allow tests to configure fake UIDs for the client and server just once during the "arrange" phase. Use this new test infra to simplify a RobolectricBinderTransportTest auth test. TAG=agy CONV=e849839d-3cdc-4bcc-863d-9c8b6d2f47f6 --- .../RobolectricBinderTransportTest.java | 15 ++-- .../internal/RobolectricUidPropagation.java | 72 +++++++++++++++++++ 2 files changed, 80 insertions(+), 7 deletions(-) create mode 100644 binder/src/test/java/io/grpc/binder/internal/RobolectricUidPropagation.java 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 63c47bf4f19..c78c3fc76a5 100644 --- a/binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java +++ b/binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java @@ -252,16 +252,16 @@ protected String testAuthority(InternalServer server) { @Test public void clientAuthorizesServerUidsInOrder() throws Exception { - // TODO(jdcormie): In real Android, Binder#getCallingUid is thread-local but Robolectric only - // lets us fake value this *globally*. So the ShadowBinder#setCallingUid() here unrealistically - // affects the server's view of the client's uid too. For now this doesn't matter because this - // test never exercises server SecurityPolicy. - ShadowBinder.setCallingUid(EPHEMERAL_SERVER_UID); - serverPkgInfo.applicationInfo.uid = SERVER_APP_UID; shadowOf(application.getPackageManager()).installPackage(serverPkgInfo); shadowOf(application.getPackageManager()).addOrUpdateService(serviceInfo); - server = newServer(ImmutableList.of()); + server = + newServerBuilder() + .setClientBinderDecorator( + RobolectricUidPropagation.newUidPassingBinderDecorator(EPHEMERAL_SERVER_UID)) + .setStreamTracerFactories(ImmutableList.of()) + .build(); + registerServerWithRobolectric((BinderServer) server); server.start(serverListener); SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy(); @@ -270,6 +270,7 @@ public void clientAuthorizesServerUidsInOrder() throws Exception { .setFactory( newClientTransportFactoryBuilder() .setSecurityPolicy(securityPolicy) + .setInboundBinderDecorator(RobolectricUidPropagation.newUidRestoringBinderDecorator()) .buildClientTransportFactory()) .build(); runIfNotNull(client.start(mockClientTransportListener)); diff --git a/binder/src/test/java/io/grpc/binder/internal/RobolectricUidPropagation.java b/binder/src/test/java/io/grpc/binder/internal/RobolectricUidPropagation.java new file mode 100644 index 00000000000..3ca9d715f6e --- /dev/null +++ b/binder/src/test/java/io/grpc/binder/internal/RobolectricUidPropagation.java @@ -0,0 +1,72 @@ +/* + * Copyright 2026 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.internal; + +import android.os.Binder; +import android.os.Parcel; +import android.os.RemoteException; +import org.robolectric.shadows.ShadowBinder; + +/** + * Helpers for Robolectric tests to propagate calling UID across in-process binder transactions. + * + *

Because Robolectric tests run client and server in a single process, Android's automatic + * cross-process UID propagation does not occur. This class provides decorators to simulate this + * propagation by passing the fake calling UID inside the transaction {@link Parcel}. + * + *

The outgoing decorator prepends the UID to the parcel, and the incoming decorator extracts + * it and sets it as the calling UID for the duration of the transaction handling. + */ +public final class RobolectricUidPropagation { + private RobolectricUidPropagation() {} + + /** + * Returns a decorator that prepends the given UID to all outgoing transactions. + */ + public static OneWayBinderProxy.Decorator newUidPassingBinderDecorator(int uid) { + return input -> new OneWayBinderProxy(input.getDelegate()) { + @Override + public void transact(int code, ParcelHolder data) throws RemoteException { + try (ParcelHolder newData = ParcelHolder.obtain()) { + newData.get().writeInt(uid); + Parcel srcParcel = data.get(); + newData.get().appendFrom(srcParcel, 0, srcParcel.dataSize()); + data.close(); // We consume the original data. + input.transact(code, newData); + } + } + }; + } + + /** + * Returns a decorator that reads a prepended UID from incoming transactions + * and sets it as the calling UID using Robolectric's ShadowBinder. + */ + public static LeakSafeOneWayBinder.Decorator newUidRestoringBinderDecorator() { + return input -> (code, data) -> { + int uid = data.readInt(); + int originalUid = Binder.getCallingUid(); + try { + // TODO(jdcormie): Use the new thread-safe Robolectric API (https://github.com/robolectric/robolectric/commit/892a5f2a535606c96142010c295f5f6b0bdd6bfb) once it is released. + ShadowBinder.setCallingUid(uid); + return input.handleTransaction(code, data); + } finally { + ShadowBinder.setCallingUid(originalUid); + } + }; + } +}