[GEODE-10674] Refactor ClientProxyMembershipID read path

geode-core/src/main/java/org/apache/geode/internal/InternalDataSerializer.java

@@ -2492,6 +2492,60 @@ public static String readString(DataInput in, byte header) throws IOException {
     return StaticSerialization.readString(in, header);
   }
 
+  private static final boolean READ_ALL_PROPERTY_VALUE_TYPES =
+      Boolean.getBoolean(GeodeGlossary.GEMFIRE_PREFIX + "readAllPropertyValueTypes");
+
+  /**
+   * Reads a {@link Properties} written by {@link DataSerializer#writeProperties} whose keys and
+   * values use only the simple encodings expected for connection credentials: strings, byte
+   * arrays and nulls. An entry encoded in any other form is rejected with an {@link IOException}.
+   * Set {@code gemfire.readAllPropertyValueTypes} to read value types other than strings and byte
+   * arrays, for deployments whose {@code AuthInitialize} stores such values.
+   */
+  public static Properties readStringProperties(DataInput in)
+      throws IOException, ClassNotFoundException {
+    if (READ_ALL_PROPERTY_VALUE_TYPES) {
+      return DataSerializer.readProperties(in);
+    }
+    checkIn(in);
+    int size = readArrayLength(in);
+    if (size == -1) {
+      return null;
+    }
+    Properties props = new Properties();
+    for (int index = 0; index < size; index++) {
+      Object key = readSimplePropertyValue(in);
+      Object value = readSimplePropertyValue(in);
+      props.put(key, value);
+    }
+    return props;
+  }
+
+  private static Object readSimplePropertyValue(DataInput in) throws IOException {
+    checkIn(in);
+    DSCODE dscode = DscodeHelper.toDSCODE(in.readByte());
+    if (dscode == null) {
+      throw new IOException("Unexpected wire format while reading properties");
+    }
+    switch (dscode) {
+      case NULL:
+      case NULL_STRING:
+        return null;
+      case STRING:
+        return readStringUTFFromDataInput(in);
+      case HUGE_STRING:
+        return readHugeStringFromDataInput(in);
+      case STRING_BYTES:
+        return readStringBytesFromDataInput(in, in.readUnsignedShort());
+      case HUGE_STRING_BYTES:
+        return readStringBytesFromDataInput(in, in.readInt());
+      case BYTE_ARRAY:
+        return readByteArray(in);
+      default:
+        throw new IOException("Unexpected wire format while reading properties");
+    }
+  }
+
   /**
    * Just like readObject but make sure and pdx deserialized is not a PdxInstance.
    *

geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientProxyMembershipID.java

@@ -37,8 +37,10 @@
 import org.apache.geode.distributed.DurableClientAttributes;
 import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
 import org.apache.geode.internal.HeapDataOutputStream;
+import org.apache.geode.internal.InternalDataSerializer;
 import org.apache.geode.internal.net.SocketCreator;
 import org.apache.geode.internal.serialization.ByteArrayDataInput;
+import org.apache.geode.internal.serialization.DSCODE;
 import org.apache.geode.internal.serialization.DataSerializableFixedID;
 import org.apache.geode.internal.serialization.DeserializationContext;
 import org.apache.geode.internal.serialization.KnownVersion;
@@ -547,10 +549,22 @@ public KnownVersion[] getSerializationVersions() {
   public static ClientProxyMembershipID readCanonicalized(DataInput in)
       throws IOException, ClassNotFoundException {
 
-    ClientProxyMembershipID result = DataSerializer.readObject(in);
+    // The field is always written with DataSerializer.writeObject of a
+    // ClientProxyMembershipID (a fixed-id type) or a null, so the only shapes that appear
+    // on the wire are a NULL marker or a byte fixed-id header for this type. Read those two
+    // shapes directly instead of going through the general object reader.
+    final byte header = in.readByte();
+    if (header == DSCODE.NULL.toByte()) {
+      return null;
+    }
+    if (header != DSCODE.DS_FIXED_ID_BYTE.toByte() || in.readByte() != CLIENT_PROXY_MEMBERSHIPID) {
+      throw new IOException("Unexpected wire format for ClientProxyMembershipID");
+    }
+    final ClientProxyMembershipID result = new ClientProxyMembershipID();
+    InternalDataSerializer.invokeFromData(result, in);
     // We can't canonicalize if we have no identity.
     // I only saw this happen in unit tests that serialize "new ClientProxyMembershipID()".
-    if (result == null || result.identity == null) {
+    if (result.identity == null) {
       return result;
     }
     return result.canonicalReference();

geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/EncryptorImpl.java

@@ -66,6 +66,7 @@
 import org.apache.geode.distributed.DistributedSystem;
 import org.apache.geode.distributed.internal.DistributionConfig;
 import org.apache.geode.internal.HeapDataOutputStream;
+import org.apache.geode.internal.InternalDataSerializer;
 import org.apache.geode.internal.cache.tier.Encryptor;
 import org.apache.geode.internal.logging.InternalLogWriter;
 import org.apache.geode.internal.serialization.ByteArrayDataInput;
@@ -687,7 +688,7 @@ static Properties getDecryptedCredentials(DataInputStream dis, DataOutputStream
 
       byte[] credentialBytes = decrypt.doFinal(encBytes);
       ByteArrayDataInput dinp = new ByteArrayDataInput(credentialBytes);
-      credentials = DataSerializer.readProperties(dinp);
+      credentials = InternalDataSerializer.readStringProperties(dinp);
       byte[] challengeRes = DataSerializer.readByteArray(dinp);
       // Check the challenge string
       if (!Arrays.equals(challenge, challengeRes)) {

geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/Handshake.java

@@ -38,6 +38,7 @@
 import org.apache.geode.distributed.DistributedSystem;
 import org.apache.geode.internal.ClassLoadUtils;
 import org.apache.geode.internal.HeapDataOutputStream;
+import org.apache.geode.internal.InternalDataSerializer;
 import org.apache.geode.internal.cache.tier.ConnectionProxy;
 import org.apache.geode.internal.logging.InternalLogWriter;
 import org.apache.geode.internal.security.CallbackInstantiator;
@@ -445,9 +446,9 @@ public static Properties readCredentials(DataInputStream dis, DataOutputStream d
       throwIfMissingRequiredCredentials(requireAuthentication, secureMode != CREDENTIALS_NONE);
       if (secureMode == CREDENTIALS_NORMAL) {
         if (requireAuthentication) {
-          credentials = DataSerializer.readProperties(dis);
+          credentials = InternalDataSerializer.readStringProperties(dis);
         } else {
-          DataSerializer.readProperties(dis); // ignore the credentials
+          InternalDataSerializer.readStringProperties(dis); // ignore the credentials
         }
       } else if (secureMode == CREDENTIALS_DHENCRYPT) {
         credentials = EncryptorImpl.getDecryptedCredentials(dis, dos, system, requireAuthentication,

geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java

@@ -48,7 +48,6 @@
 import org.jetbrains.annotations.TestOnly;
 
 import org.apache.geode.CancelException;
-import org.apache.geode.DataSerializer;
 import org.apache.geode.SystemFailure;
 import org.apache.geode.annotations.VisibleForTesting;
 import org.apache.geode.annotations.internal.MakeNotStatic;
@@ -59,6 +58,7 @@
 import org.apache.geode.distributed.DistributedSystem;
 import org.apache.geode.internal.Assert;
 import org.apache.geode.internal.HeapDataOutputStream;
+import org.apache.geode.internal.InternalDataSerializer;
 import org.apache.geode.internal.cache.EventID;
 import org.apache.geode.internal.cache.InternalCache;
 import org.apache.geode.internal.cache.tier.Acceptor;
@@ -1202,7 +1202,7 @@ long getUniqueId(Message message, long existingUniqueId) throws Exception {
 
     Properties credentials;
     try (ByteArrayDataInput dinp = new ByteArrayDataInput(credBytes)) {
-      credentials = DataSerializer.readProperties(dinp);
+      credentials = InternalDataSerializer.readStringProperties(dinp);
     }
 
     // When here, security is enforced on server, if login returns a subject, then it's the newly

geode-core/src/test/java/org/apache/geode/internal/InternalDataSerializerPropertiesTest.java

@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You 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 org.apache.geode.internal;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.ByteArrayOutputStream;
+import java.io.DataOutputStream;
+import java.io.IOException;
+import java.io.Serializable;
+import java.util.Properties;
+
+import org.junit.Test;
+
+import org.apache.geode.DataSerializer;
+import org.apache.geode.internal.serialization.ByteArrayDataInput;
+
+public class InternalDataSerializerPropertiesTest {
+
+  @Test
+  public void readStringPropertiesReadsStringAndByteArrayValues() throws Exception {
+    final Properties written = new Properties();
+    written.put("user", "alice");
+    written.put("token", new byte[] {1, 2, 3});
+    final ByteArrayOutputStream bytes = new ByteArrayOutputStream();
+    DataSerializer.writeProperties(written, new DataOutputStream(bytes));
+
+    final Properties read =
+        InternalDataSerializer.readStringProperties(new ByteArrayDataInput(bytes.toByteArray()));
+
+    assertThat(read.get("user")).isEqualTo("alice");
+    assertThat((byte[]) read.get("token")).containsExactly(1, 2, 3);
+  }
+
+  @Test
+  public void readStringPropertiesRejectsOtherValueTypes() throws Exception {
+    ValueProbe.deserialized = false;
+    final Properties written = new Properties();
+    written.put("user", new ValueProbe());
+    final ByteArrayOutputStream bytes = new ByteArrayOutputStream();
+    DataSerializer.writeProperties(written, new DataOutputStream(bytes));
+
+    assertThatThrownBy(() -> InternalDataSerializer
+        .readStringProperties(new ByteArrayDataInput(bytes.toByteArray())))
+            .isInstanceOf(IOException.class);
+    assertThat(ValueProbe.deserialized).isFalse();
+  }
+
+  public static class ValueProbe implements Serializable {
+    static boolean deserialized;
+
+    private void readObject(java.io.ObjectInputStream in)
+        throws IOException, ClassNotFoundException {
+      in.defaultReadObject();
+      deserialized = true;
+    }
+  }
+}

geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/ClientProxyMembershipIDTest.java

@@ -16,19 +16,24 @@
 package org.apache.geode.internal.cache.tier.sockets;
 
 
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 import java.io.ByteArrayOutputStream;
+import java.io.DataOutputStream;
 import java.io.IOException;
 import java.io.ObjectInput;
 import java.io.ObjectInputStream;
 import java.io.ObjectOutput;
 import java.io.ObjectOutputStream;
+import java.io.Serializable;
 
 import org.junit.Test;
 
+import org.apache.geode.DataSerializer;
 import org.apache.geode.internal.NullDataOutputStream;
 import org.apache.geode.internal.serialization.ByteArrayDataInput;
+import org.apache.geode.internal.serialization.DSCODE;
 
 public class ClientProxyMembershipIDTest {
 
@@ -61,4 +66,49 @@ public void readExternalThrowsIfIdentityIsTooShort() throws IOException {
         .hasMessageContaining("identity length is too small");
   }
 
+
+  @Test
+  public void readCanonicalizedRejectsUnexpectedHeader() throws Exception {
+    ProbeObject.deserialized = false;
+
+    final ByteArrayOutputStream bytes = new ByteArrayOutputStream();
+    bytes.write(DSCODE.SERIALIZABLE.toByte());
+    final ObjectOutputStream objectOut = new ObjectOutputStream(bytes);
+    objectOut.writeObject(new ProbeObject());
+    objectOut.close();
+    final ByteArrayDataInput in = new ByteArrayDataInput(bytes.toByteArray());
+
+    assertThatThrownBy(() -> ClientProxyMembershipID.readCanonicalized(in))
+        .isInstanceOf(IOException.class);
+    assertThat(ProbeObject.deserialized).isFalse();
+  }
+
+  @Test
+  public void readCanonicalizedReadsNull() throws Exception {
+    final ByteArrayOutputStream bytes = new ByteArrayOutputStream();
+    DataSerializer.writeObject(null, new DataOutputStream(bytes));
+    final ByteArrayDataInput in = new ByteArrayDataInput(bytes.toByteArray());
+
+    assertThat(ClientProxyMembershipID.readCanonicalized(in)).isNull();
+  }
+
+  @Test
+  public void readCanonicalizedReadsFixedIdForm() throws Exception {
+    final ClientProxyMembershipID written = new ClientProxyMembershipID();
+    final ByteArrayOutputStream bytes = new ByteArrayOutputStream();
+    DataSerializer.writeObject(written, new DataOutputStream(bytes));
+    final ByteArrayDataInput in = new ByteArrayDataInput(bytes.toByteArray());
+
+    assertThat(ClientProxyMembershipID.readCanonicalized(in)).isNotNull();
+  }
+
+  public static class ProbeObject implements Serializable {
+    static boolean deserialized;
+
+    private void readObject(java.io.ObjectInputStream in)
+        throws IOException, ClassNotFoundException {
+      in.defaultReadObject();
+      deserialized = true;
+    }
+  }
 }