Use DetailedArrayReferenceValue consistently for particular arrays in…

… ExecutingInvocationUnit

Summary:
This solves the problem of particular array values being possibly `ParticularReferenceValue` or `DetailedArrayReferenceValue` based on how they are created. Now the latter is used consistently, also when a method tracked by `ExecutingInvocationUnit` returns a array of references.

Other inconsistencies of how array are handled are still present and not addressed on purpose, since that behavior is present deeply in the PE code. For example, `ExecutingInvocationUnit#createNonParticularValue` still invokes `ValueFactory#createReferenceValue` even if the argument is an array, instead of `createArrayReferenceValue`, resulting in having an `IdentifiedReferenceValue` instead of an `IdentifiedArrayReferenceValue`. This is because values created outside the invocation unit use methods from `BasicValueFactory` and not overriden by the more advanced value factories, which also never end up invoking `ValueFactory#createArrayReferenceValue`. Trying to change this behavior to have consistency would probably be a longer process, so I'm keeping the code as it is.

NB: the updated behavior will result in `ArrayReferenceValue` being created if `ArrayReferenceValueFactory` is used instead of `DetailedArrayValueFactory` and only in the case an array is returned by an executor (since `createValue` actually invokes `createArrayReferenceValue`), this inconsistency does not happen if `DetailedArrayValueFactory` is used with `ExecutingInvocationUnit`.

In general the fix is quite hacky because it relies on `DetailedArrayValueFactory` calling the `referenceValueFactory` from `ParticularValueFactory` directly, since it needs to bypass the behavior of `IdentifiedValueFactory` because of some improper inheritance.

base/src/main/java/proguard/evaluation/ExecutingInvocationUnit.java

@@ -514,17 +514,15 @@ private MethodResult createFallbackResult(MethodExecutionInfo methodInfo) {
       }
     }
 
-    // TODO: update to handle array of references the same way as primitive arrays
-    if (ClassUtil.internalArrayTypeDimensionCount(type) == 1
-        && isInternalPrimitiveType(ClassUtil.internalTypeFromArrayType(type))
-        && concreteValue != null) {
+    if (ClassUtil.internalArrayTypeDimensionCount(type) == 1 && concreteValue != null) {
       if (concreteValue instanceof Model) {
         throw new IllegalStateException(
             "Modeled arrays are not supported by ExecutingInvocationUnit");
       }
       return valueFactory.createArrayReferenceValue(
-          type,
-          null, // NB the class is null just because we are filtering for primitive arrays
+          // This method expects the type of the content of the array
+          type.substring(1),
+          referencedClass, // Might be null (for primitive arrays)
           valueFactory.createIntegerValue(Array.getLength(concreteValue)),
           concreteValue);
     }

base/src/main/java/proguard/evaluation/value/ArrayReferenceValueFactory.java

@@ -28,10 +28,17 @@
 public class ArrayReferenceValueFactory extends TypedReferenceValueFactory {
   // Implementations for ReferenceValue.
 
+  @Override
   public ReferenceValue createArrayReferenceValue(
       String type, Clazz referencedClass, IntegerValue arrayLength) {
     return type == null
         ? REFERENCE_VALUE_NULL
         : new ArrayReferenceValue(TypeConstants.ARRAY + type, referencedClass, false, arrayLength);
   }
+
+  @Override
+  public ReferenceValue createArrayReferenceValue(
+      String type, Clazz referencedClass, IntegerValue arrayLength, Object elementValues) {
+    return createArrayReferenceValue(type, referencedClass, arrayLength);
+  }
 }

base/src/main/java/proguard/evaluation/value/DetailedArrayReferenceValue.java

@@ -28,8 +28,6 @@
 /**
  * This {@link IdentifiedArrayReferenceValue} represents an identified array reference value with
  * its elements.
- *
- * @author Eric Lafortune
  */
 public class DetailedArrayReferenceValue extends IdentifiedArrayReferenceValue {
   private static final int MAXIMUM_STORED_ARRAY_LENGTH = 64;

base/src/main/java/proguard/evaluation/value/DetailedArrayValueFactory.java

@@ -17,17 +17,37 @@
  */
 package proguard.evaluation.value;
 
-import proguard.classfile.*;
+import proguard.classfile.Clazz;
+import proguard.classfile.TypeConstants;
+import proguard.classfile.util.ClassUtil;
+import proguard.evaluation.value.object.AnalyzedObject;
+import proguard.evaluation.value.object.AnalyzedObjectFactory;
 
 /**
  * This identified value factory creates array reference values that also represent their elements,
  * in as far as possible.
- *
- * @author Eric Lafortune
  */
 public class DetailedArrayValueFactory extends IdentifiedValueFactory {
-  // Implementations for ReferenceValue.
 
+  /** Creates a new DetailedArrayValueFactory, which does not track reference values. */
+  @Deprecated
+  public DetailedArrayValueFactory() {
+    this(new TypedReferenceValueFactory());
+  }
+
+  /**
+   * Creates a new DetailedArrayValueFactory, which uses the given value factory for non-array
+   * reference construction.
+   */
+  public DetailedArrayValueFactory(ValueFactory referenceValueFactory) {
+    // This is quite ugly, but unavoidable without refactoring the value factories hierarchy. Since
+    // what's currently going on is that DetailedArrayReferenceValue is a ParticularValueFactory
+    // that overrides all the methods where arrayReferenceValueFactory is used, even if we pass an
+    // ArrayReferenceValueFactory to super this will never be used.
+    super(new UnusableArrayValueFactory(), referenceValueFactory);
+  }
+
+  @Override
   public ReferenceValue createArrayReferenceValue(
       String type, Clazz referencedClass, IntegerValue arrayLength) {
     return type == null
@@ -49,14 +69,17 @@ public ReferenceValue createArrayReferenceValue(
                 generateReferenceId());
   }
 
+  @Override
   public ReferenceValue createArrayReferenceValue(
       String type, Clazz referencedClass, IntegerValue arrayLength, Object elementValues) {
 
     if (type == null) return TypedReferenceValueFactory.REFERENCE_VALUE_NULL;
 
+    String arrayType = TypeConstants.ARRAY + type;
+
     if (!arrayLength.isParticular()) {
       return new IdentifiedArrayReferenceValue(
-          type, referencedClass, false, arrayLength, this, generateReferenceId());
+          arrayType, referencedClass, false, arrayLength, this, generateReferenceId());
     }
     if (!elementValues.getClass().isArray()
         || elementValues.getClass().getComponentType().isArray()) {
@@ -66,38 +89,38 @@ public ReferenceValue createArrayReferenceValue(
 
     DetailedArrayReferenceValue detailedArray =
         new DetailedArrayReferenceValue(
-            type, referencedClass, false, arrayLength, this, generateReferenceId());
-    if (elementValues.getClass().isArray()) {
-      switch (type.charAt(1)) // 0 is the array char
-      {
-        case TypeConstants.BOOLEAN:
-          storeBooleanArray(detailedArray, (boolean[]) elementValues);
-          break;
-        case TypeConstants.BYTE:
-          storeByteArray(detailedArray, (byte[]) elementValues);
-          break;
-        case TypeConstants.CHAR:
-          storeCharArray(detailedArray, (char[]) elementValues);
-          break;
-        case TypeConstants.SHORT:
-          storeShortArray(detailedArray, (short[]) elementValues);
-          break;
-        case TypeConstants.INT:
-          storeIntArray(detailedArray, (int[]) elementValues);
-          break;
-        case TypeConstants.LONG:
-          storeLongArray(detailedArray, (long[]) elementValues);
-          break;
-        case TypeConstants.FLOAT:
-          storeFloatArray(detailedArray, (float[]) elementValues);
-          break;
-        case TypeConstants.DOUBLE:
-          storeDoubleArray(detailedArray, (double[]) elementValues);
-          break;
-        default:
-          storeObjectArray(detailedArray, (Object[]) elementValues);
-      }
+            arrayType, referencedClass, false, arrayLength, this, generateReferenceId());
+
+    switch (arrayType.charAt(1)) // 0 is the array char
+    {
+      case TypeConstants.BOOLEAN:
+        storeBooleanArray(detailedArray, (boolean[]) elementValues);
+        break;
+      case TypeConstants.BYTE:
+        storeByteArray(detailedArray, (byte[]) elementValues);
+        break;
+      case TypeConstants.CHAR:
+        storeCharArray(detailedArray, (char[]) elementValues);
+        break;
+      case TypeConstants.SHORT:
+        storeShortArray(detailedArray, (short[]) elementValues);
+        break;
+      case TypeConstants.INT:
+        storeIntArray(detailedArray, (int[]) elementValues);
+        break;
+      case TypeConstants.LONG:
+        storeLongArray(detailedArray, (long[]) elementValues);
+        break;
+      case TypeConstants.FLOAT:
+        storeFloatArray(detailedArray, (float[]) elementValues);
+        break;
+      case TypeConstants.DOUBLE:
+        storeDoubleArray(detailedArray, (double[]) elementValues);
+        break;
+      default:
+        storeObjectArray(detailedArray, (Object[]) elementValues);
     }
+
     return detailedArray;
   }
 
@@ -150,11 +173,48 @@ private void storeDoubleArray(DetailedArrayReferenceValue detailedArray, double[
     }
   }
 
-  private void storeObjectArray(DetailedArrayReferenceValue detailedArray, Object[] elementValues) {
-    for (int i = 0; i < elementValues.length; i++) {
-      detailedArray.arrayStore(
-          createIntegerValue(i),
-          createReferenceValue(detailedArray.referencedClass, elementValues[i]));
+  private void storeObjectArray(DetailedArrayReferenceValue detailedArray, Object[] elements) {
+    for (int i = 0; i < elements.length; i++) {
+      Object element = elements[i];
+      Value elementValue;
+      if (element == null) {
+        elementValue = createReferenceValueNull();
+      } else {
+        Clazz referencedClass = detailedArray.referencedClass;
+        AnalyzedObject object =
+            AnalyzedObjectFactory.create(
+                element,
+                ClassUtil.internalTypeFromClassName(referencedClass.getName()),
+                referencedClass);
+        // Call on referenceValueFactory instead of "this" to avoid the behavior from
+        // IdentifiedValueFactory (from which DetailedArrayValueFactory should probably not
+        // inherit).
+        elementValue =
+            referenceValueFactory.createReferenceValue(
+                referencedClass, ClassUtil.isExtendable(referencedClass), false, object);
+      }
+      detailedArray.arrayStore(createIntegerValue(i), elementValue);
+    }
+  }
+
+  /**
+   * A value factory that should never be used, used as a placeholder for the arrayValueFactory in
+   * {@link ParticularValueFactory}, since all the methods calling it should be overridden by {@link
+   * DetailedArrayValueFactory}.
+   */
+  private static class UnusableArrayValueFactory extends ArrayReferenceValueFactory {
+    @Override
+    public ReferenceValue createArrayReferenceValue(
+        String type, Clazz referencedClass, IntegerValue arrayLength) {
+      throw new IllegalStateException(
+          "This value factory should never be used, DetailedArrayValueFactory should override all methods calling this");
+    }
+
+    @Override
+    public ReferenceValue createArrayReferenceValue(
+        String type, Clazz referencedClass, IntegerValue arrayLength, Object elementValues) {
+      throw new IllegalStateException(
+          "This value factory should never be used, DetailedArrayValueFactory should override all methods calling this");
     }
   }
 }

base/src/main/java/proguard/evaluation/value/IdentifiedValueFactory.java

@@ -33,6 +33,30 @@ public class IdentifiedValueFactory extends ParticularValueFactory {
   private final AtomicInteger doubleID = new AtomicInteger(0);
   private static final AtomicInteger referenceIdProvider = new AtomicInteger(0);
 
+  /** Creates a new IdentifiedValueFactory which does not keep track of particular references. */
+  public IdentifiedValueFactory() {
+    super();
+  }
+
+  /**
+   * Creates a new IdentifiedValueFactory, which uses the given valuefactory for both array and
+   * non-array reference construction.
+   */
+  public IdentifiedValueFactory(ValueFactory referenceValueFactory) {
+    super(referenceValueFactory);
+  }
+
+  /**
+   * Creates a new IdentifiedValueFactory.
+   *
+   * @param arrayReferenceValueFactory the valuefactory to delegate new array references to.
+   * @param referenceValueFactory the valuefactory to delegate new references to.
+   */
+  public IdentifiedValueFactory(
+      ValueFactory arrayReferenceValueFactory, ValueFactory referenceValueFactory) {
+    super(arrayReferenceValueFactory, referenceValueFactory);
+  }
+
   // Implementations for BasicValueFactory.
 
   public IntegerValue createIntegerValue() {
@@ -121,9 +145,7 @@ public ReferenceValue createReferenceValueForId(
       boolean mayBeNull,
       Object id,
       Object value) {
-    return type == null
-        ? TypedReferenceValueFactory.REFERENCE_VALUE_NULL
-        : new IdentifiedReferenceValue(type, referencedClass, mayBeExtension, mayBeNull, this, id);
+    return this.createReferenceValueForId(type, referencedClass, mayBeExtension, mayBeNull, id);
   }
 
   public ReferenceValue createArrayReferenceValue(

base/src/main/java/proguard/evaluation/value/ParticularReferenceValue.java

@@ -28,6 +28,8 @@
  * with an associated value. E.g., a String with the value "HelloWorld".
  */
 public class ParticularReferenceValue extends IdentifiedReferenceValue {
+  public static boolean ARRAY_EXCEPTIONS =
+      System.getProperty("proguard.particularvalue.arrayexceptions") != null;
 
   // The actual value of the object.
   private final AnalyzedObject objectValue;
@@ -51,6 +53,12 @@ public ParticularReferenceValue(
         value.getType(),
         "ParticularReferenceValue should not be created with a 'NullObject', a 'TypedReferenceValue' with null type is expected in that case");
 
+    if (ARRAY_EXCEPTIONS
+        && proguard.classfile.util.ClassUtil.isInternalArrayType(value.getType())) {
+      throw new IllegalStateException(
+          "ParticularReferenceValue should not be used for arrays use DetailedArrayReferenceValue instead");
+    }
+
     this.objectValue = value;
   }
 

base/src/main/java/proguard/evaluation/value/ParticularValueFactory.java

@@ -48,9 +48,11 @@ public class ParticularValueFactory extends BasicValueFactory implements ValueFa
   private static final long POS_ZERO_DOUBLE_BITS = Double.doubleToLongBits(0.0);
 
   private final ValueFactory arrayReferenceValueFactory;
-  // if set to true, a ParticularReferenceValue will be created for each reference. Otherwise, this
-  // will be delegated to the referenceValueFactory.
-  private final ValueFactory referenceValueFactory;
+  // This is protected to be usable directly by children, which should ideally be not allowed, but
+  // this is a hacky fix for improper inheritance currently taking place (i.e.,
+  // DetailedArrayReferenceValue inheriting from IdentifiedReferenceValue, which inherits from this
+  // class).
+  protected final ValueFactory referenceValueFactory;
 
   /** Creates a new ParticularValueFactory which does not keep track of particular references. */
   public ParticularValueFactory() {

base/src/main/java/proguard/evaluation/value/object/AnalyzedObjectFactory.java

@@ -4,8 +4,10 @@
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import proguard.classfile.Clazz;
+import proguard.classfile.TypeConstants;
 import proguard.classfile.util.ClassUtil;
 import proguard.classfile.util.InitializedClassUtil;
+import proguard.evaluation.value.ParticularReferenceValue;
 import proguard.evaluation.value.Value;
 
 /** Factory methods to create {@link AnalyzedObject}. */
@@ -63,8 +65,9 @@ public static AnalyzedObject create(
       return createModeled(model);
     }
 
-    // TODO create ArrayObject if value is an array once we fix the code using
-    // ParticularReferenceValue for arrays
+    if (ParticularReferenceValue.ARRAY_EXCEPTIONS) {
+      checkNotArray(value, type);
+    }
 
     checkValidPrecise(value, type, referencedClass);
     return createPrecise(value);
@@ -114,6 +117,12 @@ private static void checkValidModel(Model model, Clazz referencedClass) {
     }
   }
 
+  private static void checkNotArray(Object value, String type) {
+    if (value.getClass().isArray() || TypeConstants.ARRAY == type.charAt(0)) {
+      throw new IllegalStateException("Arrays are supported only via ArrayModel");
+    }
+  }
+
   private static void checkValidPrecise(Object value, String type, Clazz referencedClass) {
     String objectType = ClassUtil.internalType(value.getClass().getTypeName());
     if (referencedClass == null && !type.equals(objectType)) {

base/src/test/kotlin/proguard/DetailedArrayTest.kt

@@ -23,7 +23,7 @@ import proguard.testutils.PartialEvaluatorUtil
 import proguard.util.PartialEvaluatorHelper
 
 class DetailedArrayTest : FreeSpec({
-    val valueFactory: ValueFactory = ParticularValueFactory(DetailedArrayValueFactory(), ParticularReferenceValueFactory())
+    val valueFactory: ValueFactory = ParticularValueFactory(DetailedArrayValueFactory(ParticularReferenceValueFactory()), ParticularReferenceValueFactory())
 
     "Array values evaluated correctly" - {
 
@@ -418,7 +418,7 @@ class DetailedArrayTest : FreeSpec({
             javacArguments = listOf("-source", "8", "-target", "8"),
         )
 
-        val invocationsWithStack = PartialEvaluatorHelper.evaluateMethod("A", "functions", "()V", programClassPool, libraryClassPool, DetailedArrayValueFactory())
+        val invocationsWithStack = PartialEvaluatorHelper.evaluateMethod("A", "functions", "()V", programClassPool, libraryClassPool, DetailedArrayValueFactory(ParticularReferenceValueFactory()))
 
         "Primitive byte array evaluated correctly" {
             val value = invocationsWithStack[14]!!.stack[0]

base/src/test/kotlin/proguard/ParticularReferenceTest.kt

@@ -21,6 +21,7 @@ import proguard.classfile.editor.ClassBuilder
 import proguard.evaluation.BasicInvocationUnit
 import proguard.evaluation.PartialEvaluator
 import proguard.evaluation.ParticularReferenceValueFactory
+import proguard.evaluation.value.ArrayReferenceValue
 import proguard.evaluation.value.ArrayReferenceValueFactory
 import proguard.evaluation.value.IdentifiedReferenceValue
 import proguard.evaluation.value.ParticularIntegerValue
@@ -763,9 +764,9 @@ class ParticularReferenceTest : FreeSpec({
 
         "String array evaluated correctly" {
             val value = invocationsWithStack[14]!!.stack[0]
-            value.shouldBeInstanceOf<ParticularReferenceValue>()
+            // NB: this is not an IdentifiedArrayReferenceValue because ArrayReferenceValueFactory does not support identified arrays
+            value.shouldBeInstanceOf<ArrayReferenceValue>()
             value.type shouldBe "[Ljava/lang/String;"
-            value.value() shouldBe arrayOf("42", "43")
         }
     }