From ca8f4dfa6d76b3fe9dafcea6e43a4428ec5c167d Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 26 Aug 2025 03:27:49 -0700 Subject: [PATCH 1/4] binder: Improve error descriptions for ServiceConnection callbacks (#12263) Non-experts don't really know what these ServiceConnection callback names mean (eg b/437170499). Use the Status description to explain them a bit. Compare to https://github.com/grpc/grpc-java/pull/11628 --- .../grpc/binder/internal/ServiceBinding.java | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/internal/ServiceBinding.java b/binder/src/main/java/io/grpc/binder/internal/ServiceBinding.java index 42d69d27a2e..3885afb19a8 100644 --- a/binder/src/main/java/io/grpc/binder/internal/ServiceBinding.java +++ b/binder/src/main/java/io/grpc/binder/internal/ServiceBinding.java @@ -356,19 +356,32 @@ public void onServiceConnected(ComponentName className, IBinder binder) { @Override @MainThread public void onServiceDisconnected(ComponentName name) { - unbindInternal(Status.UNAVAILABLE.withDescription("onServiceDisconnected: " + name)); + unbindInternal( + Status.UNAVAILABLE.withDescription( + "Server process crashed, exited or was killed (onServiceDisconnected): " + name)); } @Override @MainThread public void onNullBinding(ComponentName name) { - unbindInternal(Status.UNIMPLEMENTED.withDescription("onNullBinding: " + name)); + unbindInternal( + Status.UNIMPLEMENTED.withDescription( + "Remote Service returned null from onBind() for " + + bindIntent + + " (onNullBinding): " + + name)); } @Override @MainThread public void onBindingDied(ComponentName name) { - unbindInternal(Status.UNAVAILABLE.withDescription("onBindingDied: " + name)); + unbindInternal( + Status.UNAVAILABLE.withDescription( + "Remote Service component " + + name.getClassName() + + " was disabled, or its package " + + name.getPackageName() + + " was disabled, force-stopped, replaced or uninstalled (onBindingDied).")); } @VisibleForTesting From 695014adfd33aa8facb721e358a485d08a23b76f Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Thu, 14 Aug 2025 07:23:01 -0700 Subject: [PATCH 2/4] api: Reduce allocations of Attributes.Builder Modifying existing Attributes was virtually guaranteed to allocate within build(), just because `base` was not considered for the new map size. Discard was also allocation-heavy because it often created a new map. Using a regular copy-on-write approach is enough to avoid the unnecessary allocations in both cases. This was noticed in a profile that included xds's AddressFilter.setPathFilter(), where Attributes.Builder.build() allocated 6x the memory of Attributes.Builder.data(). b/435208946#comment41 --- api/src/main/java/io/grpc/Attributes.java | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/api/src/main/java/io/grpc/Attributes.java b/api/src/main/java/io/grpc/Attributes.java index de00e63554c..c8550d176b4 100644 --- a/api/src/main/java/io/grpc/Attributes.java +++ b/api/src/main/java/io/grpc/Attributes.java @@ -215,6 +215,7 @@ public int hashCode() { * The helper class to build an Attributes instance. */ public static final class Builder { + // Exactly one of base and newdata will be set private Attributes base; private IdentityHashMap, Object> newdata; @@ -225,8 +226,11 @@ private Builder(Attributes base) { private IdentityHashMap, Object> data(int size) { if (newdata == null) { - newdata = new IdentityHashMap<>(size); + newdata = new IdentityHashMap<>(base.data.size() + size); + newdata.putAll(base.data); + base = null; } + assert base == null; return newdata; } @@ -243,12 +247,11 @@ public Builder set(Key key, T value) { * @return this */ public Builder discard(Key key) { - if (base.data.containsKey(key)) { - IdentityHashMap, Object> newBaseData = new IdentityHashMap<>(base.data); - newBaseData.remove(key); - base = new Attributes(newBaseData); - } - if (newdata != null) { + if (base != null) { + if (base.data.containsKey(key)) { + data(0).remove(key); + } + } else { newdata.remove(key); } return this; @@ -264,11 +267,6 @@ public Builder setAll(Attributes other) { */ public Attributes build() { if (newdata != null) { - for (Map.Entry, Object> entry : base.data.entrySet()) { - if (!newdata.containsKey(entry.getKey())) { - newdata.put(entry.getKey(), entry.getValue()); - } - } base = new Attributes(newdata); newdata = null; } From afef4fe097f695db76f59f0d4fedc04eb2c1815b Mon Sep 17 00:00:00 2001 From: Alex Panchenko <440271+panchenko@users.noreply.github.com> Date: Thu, 28 Aug 2025 05:57:10 +0200 Subject: [PATCH 3/4] servlet: extract ServletServerStream.serializeHeaders() method (#12299) --- .../io/grpc/servlet/ServletServerStream.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/servlet/src/main/java/io/grpc/servlet/ServletServerStream.java b/servlet/src/main/java/io/grpc/servlet/ServletServerStream.java index caab5caa8a9..0182f302698 100644 --- a/servlet/src/main/java/io/grpc/servlet/ServletServerStream.java +++ b/servlet/src/main/java/io/grpc/servlet/ServletServerStream.java @@ -42,6 +42,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.Map; +import java.util.function.BiConsumer; import java.util.function.Supplier; import java.util.logging.Logger; import javax.annotation.Nullable; @@ -122,9 +123,13 @@ private void writeHeadersToServletResponse(Metadata metadata) { resp.setStatus(HttpServletResponse.SC_OK); resp.setContentType(CONTENT_TYPE_GRPC); + serializeHeaders(metadata, resp::addHeader); + } + + private static void serializeHeaders(Metadata metadata, BiConsumer consumer) { byte[][] serializedHeaders = TransportFrameUtil.toHttp2Headers(metadata); for (int i = 0; i < serializedHeaders.length; i += 2) { - resp.addHeader( + consumer.accept( new String(serializedHeaders[i], StandardCharsets.US_ASCII), new String(serializedHeaders[i + 1], StandardCharsets.US_ASCII)); } @@ -277,13 +282,8 @@ public void writeTrailers(Metadata trailers, boolean headersSent, Status status) if (!headersSent) { writeHeadersToServletResponse(trailers); } else { - byte[][] serializedHeaders = TransportFrameUtil.toHttp2Headers(trailers); - for (int i = 0; i < serializedHeaders.length; i += 2) { - String key = new String(serializedHeaders[i], StandardCharsets.US_ASCII); - String newValue = new String(serializedHeaders[i + 1], StandardCharsets.US_ASCII); - trailerSupplier.get().computeIfPresent(key, (k, v) -> v + "," + newValue); - trailerSupplier.get().putIfAbsent(key, newValue); - } + serializeHeaders(trailers, + (k, v) -> trailerSupplier.get().merge(k, v, (oldV, newV) -> oldV + "," + newV)); } writer.complete(); From c643e68aa1faf7f1aabf8e789030c44323fc4f77 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Thu, 28 Aug 2025 01:06:48 -0700 Subject: [PATCH 4/4] binder: REMOTE_UID must hold exactly the uid passed to the SecurityPolicy and never change (#12314) `attributes = setSecurityAttrs(attributes, remoteUid);` should not run for : - a malformed SETUP_TRANSPORT transaction - a rogue SETUP_TRANSPORT transaction that arrives post-TransportState.SETUP --- .../internal/BinderClientTransport.java | 2 +- .../RobolectricBinderTransportTest.java | 42 ++++++++++++++++++- 2 files changed, 42 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 95bd531aa41..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,7 +330,6 @@ 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(); @@ -340,6 +339,7 @@ protected void handleSetupTransport(Parcel parcel) { shutdownInternal( Status.UNAVAILABLE.withDescription("Malformed SETUP_TRANSPORT data"), true); } else { + attributes = setSecurityAttrs(attributes, remoteUid); authResultFuture = checkServerAuthorizationAsync(remoteUid); Futures.addCallback( authResultFuture, 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..8f1209f389b 100644 --- a/binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java +++ b/binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java @@ -16,7 +16,11 @@ package io.grpc.binder.internal; +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; @@ -28,6 +32,8 @@ 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; @@ -38,9 +44,11 @@ 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; @@ -109,7 +117,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 +272,38 @@ 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()); + } + @Test @Ignore("See BinderTransportTest#socketStats.") @Override