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; } 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/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 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 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();