merge

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<Key<?>, Object> newdata;
 
@@ -225,8 +226,11 @@ private Builder(Attributes base) {
 
     private IdentityHashMap<Key<?>, 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 <T> Builder set(Key<T> key, T value) {
      * @return this
      */
     public <T> Builder discard(Key<T> key) {
-      if (base.data.containsKey(key)) {
-        IdentityHashMap<Key<?>, 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<Key<?>, Object> entry : base.data.entrySet()) {
-          if (!newdata.containsKey(entry.getKey())) {
-            newdata.put(entry.getKey(), entry.getValue());
-          }
-        }
         base = new Attributes(newdata);
         newdata = null;
       }

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,

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

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<Boolean> 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

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<String, String> 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();