mutation: Simplify the detach concept and make lists mutable

Detaching confusingly meant two things at once, but failed at both:
1. It was meant to rid fuzz test arguments of state shared with the
   mutator so that fuzz test body and future mutations couldn't
   affect each other. This wasn't working correctly for lists, which
   provided a shallowly immutable view on potentially mutable contents.
2. It also promised "mutable values where possible", which wasn't really
   well-defined and also doesn't match common use cases: If a function
   accepts a `List<Foo>`, it should generally be able to expect the list
   to permit modifications.

Instead, `detach` now always returns an object that shares no mutable
state, not even transitively, with the original object. Apart from
simplifying the implementation, this allows mutators to copy values by
applying `detach`, which previously could have resulted in shared state
that couldn't be mutated independently.

Since we are currently reading in the fuzz test arguments from the raw
byte representation in every iteration, there is no need to detach and
we can also return mutable lists directly. If we get rid of the
re-reads, we would have to detach, which is now verified by a new
consistency check in `ArgumentsMutator`.

We might be able to optimize e.g. `List<String>` to avoid all copies in
the future via a new annotation (e.g. `@Immutable`). The current
implementation would already avoid copying the strings and would only
recreate the list, which is most likely negligible overhead.

src/main/java/com/code_intelligence/jazzer/driver/FuzzTargetRunner.java

@@ -190,7 +190,9 @@ private static int runOne(long dataPtr, int dataLength) {
     }
     try {
       if (Opt.experimentalMutator) {
-        mutator.invoke();
+        // No need to detach as we are currently reading in the mutator state from bytes in every
+        // iteration.
+        mutator.invoke(false);
       } else if (fuzzTargetInstance == null) {
         fuzzTargetMethod.invoke(argument);
       } else {

src/main/java/com/code_intelligence/jazzer/mutation/ArgumentsMutator.java

@@ -21,13 +21,11 @@
 import static com.code_intelligence.jazzer.mutation.support.InputStreamSupport.extendWithReadExactly;
 import static com.code_intelligence.jazzer.mutation.support.Preconditions.require;
 import static com.code_intelligence.jazzer.mutation.support.StreamSupport.toArrayOrEmpty;
-import static com.code_intelligence.jazzer.mutation.support.StreamSupport.toBooleanArray;
 import static java.lang.String.format;
 import static java.util.Arrays.stream;
 import static java.util.Objects.requireNonNull;
 import static java.util.stream.Collectors.joining;
 
-import com.code_intelligence.jazzer.mutation.annotation.SafeToMutate;
 import com.code_intelligence.jazzer.mutation.api.MutatorFactory;
 import com.code_intelligence.jazzer.mutation.api.PseudoRandom;
 import com.code_intelligence.jazzer.mutation.api.SerializingMutator;
@@ -36,31 +34,37 @@
 import com.code_intelligence.jazzer.mutation.engine.SeededPseudoRandom;
 import com.code_intelligence.jazzer.mutation.mutator.Mutators;
 import com.code_intelligence.jazzer.mutation.support.InputStreamSupport.ReadExactlyInputStream;
+import com.code_intelligence.jazzer.mutation.support.Preconditions;
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.io.OutputStream;
 import java.io.UncheckedIOException;
-import java.lang.annotation.Annotation;
 import java.lang.reflect.AnnotatedType;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
-import java.util.Arrays;
 import java.util.Optional;
 
 public final class ArgumentsMutator {
   private final Object instance;
   private final Method method;
   private final ProductMutator productMutator;
-  private final boolean[] shouldDetach;
   private Object[] arguments;
 
-  private ArgumentsMutator(
-      Object instance, Method method, ProductMutator productMutator, boolean[] shouldDetach) {
+  /**
+   * True if the arguments array has already been passed to a user-provided function or exposed
+   * via {@link #getArguments()} without going through {@link ProductMutator#detach(Object[])}.
+   * In this case the arguments may have been modified externally, which interferes with mutation,
+   * or could have been stored in static state that would be affected by future mutations.
+   * Arguments should either be detached or not be reused after being exposed, which is enforced by
+   * this variable.
+   */
+  private boolean argumentsExposed;
+
+  private ArgumentsMutator(Object instance, Method method, ProductMutator productMutator) {
     this.instance = instance;
     this.method = method;
     this.productMutator = productMutator;
-    this.shouldDetach = shouldDetach;
   }
 
   public static boolean canMutate(Method method) {
@@ -72,13 +76,6 @@ private static String prettyPrintMethod(Method method) {
         stream(method.getAnnotatedParameterTypes()).map(Object::toString).collect(joining(", ")));
   }
 
-  public static ArgumentsMutator forMethodOrThrow(Method method) {
-    return forMethod(Mutators.newFactory(), null, method)
-        .orElseThrow(()
-                         -> new IllegalArgumentException(
-                             "Failed to construct mutator for " + prettyPrintMethod(method)));
-  }
-
   public static ArgumentsMutator forInstanceMethodOrThrow(Object instance, Method method) {
     return forInstanceMethod(Mutators.newFactory(), instance, method)
         .orElseThrow(()
@@ -97,14 +94,6 @@ public static Optional<ArgumentsMutator> forMethod(Method method) {
     return forMethod(Mutators.newFactory(), null, method);
   }
 
-  public static Optional<ArgumentsMutator> forInstanceMethod(Object instance, Method method) {
-    return forInstanceMethod(Mutators.newFactory(), instance, method);
-  }
-
-  public static Optional<ArgumentsMutator> forStaticMethod(Method method) {
-    return forStaticMethod(Mutators.newFactory(), method);
-  }
-
   public static Optional<ArgumentsMutator> forInstanceMethod(
       MutatorFactory mutatorFactory, Object instance, Method method) {
     require(!isStatic(method), "method must not be static");
@@ -137,14 +126,7 @@ private static ArgumentsMutator create(
       Object instance, Method method, ProductMutator productMutator) {
     method.setAccessible(true);
 
-    boolean[] shouldDetach = toBooleanArray(
-        stream(method.getParameterAnnotations()).map(ArgumentsMutator::isMutabilityRequested));
-
-    return new ArgumentsMutator(instance, method, productMutator, shouldDetach);
-  }
-
-  private static boolean isMutabilityRequested(Annotation[] annotations) {
-    return stream(annotations).anyMatch(annotation -> annotation instanceof SafeToMutate);
+    return new ArgumentsMutator(instance, method, productMutator);
   }
 
   private static boolean isStatic(Method method) {
@@ -159,6 +141,7 @@ public boolean read(ByteArrayInputStream data) {
     try {
       ReadExactlyInputStream is = extendWithReadExactly(data);
       arguments = productMutator.readExclusive(is);
+      argumentsExposed = false;
       return is.isConsumedExactly();
     } catch (IOException e) {
       throw new UncheckedIOException(e);
@@ -169,6 +152,7 @@ public boolean read(ByteArrayInputStream data) {
    * @throws UncheckedIOException if the underlying OutputStream throws
    */
   public void write(OutputStream data) {
+    failIfArgumentsExposed();
     try {
       productMutator.writeExclusive(arguments, data);
     } catch (IOException e) {
@@ -182,26 +166,30 @@ public void init(long seed) {
 
   void init(PseudoRandom prng) {
     arguments = productMutator.init(prng);
+    argumentsExposed = false;
   }
 
   public void mutate(long seed) {
     mutate(new SeededPseudoRandom(seed));
   }
 
   void mutate(PseudoRandom prng) {
+    failIfArgumentsExposed();
     // TODO: Sometimes mutate the entire byte representation of the current value with libFuzzer's
     //  dictionary and TORC mutations.
     productMutator.mutate(arguments, prng);
   }
 
-  public void invoke() throws Throwable {
-    // TODO: Sometimes hash the serialized value before and after the invocation and check that the
-    //  hashes match to catch fuzz tests that mutate mutable inputs (e.g. byte[]).
-    //  Alternatively, always detach arguments and instead of the SafeToMutate annotation have a
-    //  Mutable annotation that can be used to e.g. receive a mutable implementation of List. This
-    //  is always safe, but could incur additional overhead for arrays.
+  public void invoke(boolean detach) throws Throwable {
+    Object[] invokeArguments;
+    if (detach) {
+      invokeArguments = productMutator.detach(arguments);
+    } else {
+      invokeArguments = arguments;
+      argumentsExposed = true;
+    }
     try {
-      method.invoke(instance, productMutator.detachSelectively(arguments, shouldDetach));
+      method.invoke(instance, invokeArguments);
     } catch (IllegalAccessException e) {
       throw new IllegalStateException("method should have been made accessible", e);
     } catch (InvocationTargetException e) {
@@ -210,11 +198,17 @@ public void invoke() throws Throwable {
   }
 
   public Object[] getArguments() {
-    return Arrays.copyOf(arguments, arguments.length);
+    argumentsExposed = true;
+    return arguments;
   }
 
   @Override
   public String toString() {
     return "Arguments" + productMutator;
   }
+
+  private void failIfArgumentsExposed() {
+    Preconditions.check(!argumentsExposed,
+        "Arguments have previously been exposed to user-provided code without calling #detach and may have been modified");
+  }
 }

src/main/java/com/code_intelligence/jazzer/mutation/api/Detacher.java

@@ -31,8 +31,9 @@ public interface Detacher<T> {
    *   <li>MUST return an instance that {@link Object#equals(Object)} the argument;
    *   <li>MUST return an instance that cannot be used to mutate the state of the argument through
    *   its API (ignoring uses of {@link sun.misc.Unsafe});
-   *   <li>SHOULD return an instance that is as mutable as its type allows (e.g. return a mutable
-   * implementation of {@code List});
+   *   <li>MUST return an instance that is not affected by any changes to the original value made
+   *   by any mutator;</li>
+   *   <li>MUST be accepted by mutator methods just like the original value;</li>
    *   <li>MAY return the argument itself if it is deeply immutable.
    * </ul>
    *

src/main/java/com/code_intelligence/jazzer/mutation/combinator/ProductMutator.java

@@ -108,18 +108,6 @@ public Object[] detach(Object[] value) {
     return clone;
   }
 
-  public Object[] detachSelectively(Object[] value, boolean[] shouldDetach) {
-    Object[] clone = new Object[mutators.length];
-    for (int i = 0; i < mutators.length; i++) {
-      if (shouldDetach[i]) {
-        clone[i] = mutators[i].detach(value[i]);
-      } else {
-        clone[i] = value[i];
-      }
-    }
-    return clone;
-  }
-
   @Override
   public String toDebugString(Predicate<Debuggable> isInCycle) {
     return stream(mutators)

src/main/java/com/code_intelligence/jazzer/mutation/mutator/collection/ListMutatorFactory.java

@@ -29,11 +29,9 @@
 import java.io.DataOutputStream;
 import java.io.IOException;
 import java.lang.reflect.AnnotatedType;
-import java.util.AbstractList;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Optional;
-import java.util.RandomAccess;
 import java.util.function.Predicate;
 import java.util.stream.Collectors;
 
@@ -68,9 +66,7 @@ public List<T> read(DataInputStream in) throws IOException {
       for (int i = 0; i < size; i++) {
         list.add(elementMutator.read(in));
       }
-      // Wrap in an immutable view for additional protection against accidental mutation in fuzz
-      // tests.
-      return toImmutableListView(list);
+      return list;
     }
 
     @Override
@@ -83,24 +79,20 @@ public void write(List<T> list, DataOutputStream out) throws IOException {
 
     @Override
     protected List<T> makeDefaultInstance() {
-      // Wrap in an immutable view for additional protection against accidental mutation in fuzz
-      // tests.
-      return toImmutableListView(new ArrayList<>(maxInitialSize()));
+      return new ArrayList<>(maxInitialSize());
     }
 
     @Override
-    public void initInPlace(List<T> reference, PseudoRandom prng) {
+    public void initInPlace(List<T> list, PseudoRandom prng) {
       int targetSize = prng.closedRange(minInitialSize(), maxInitialSize());
-      List<T> list = underlyingMutableList(reference);
       list.clear();
       for (int i = 0; i < targetSize; i++) {
         list.add(elementMutator.init(prng));
       }
     }
 
     @Override
-    public void mutateInPlace(List<T> reference, PseudoRandom prng) {
-      List<T> list = underlyingMutableList(reference);
+    public void mutateInPlace(List<T> list, PseudoRandom prng) {
       if (list.isEmpty()) {
         list.add(elementMutator.init(prng));
       } else if (!prng.trueInOneOutOf(4)) {
@@ -132,46 +124,5 @@ private int minInitialSize() {
     private int maxInitialSize() {
       return min(maxSize, minSize + 1);
     }
-
-    private List<T> underlyingMutableList(List<T> value) {
-      if (value instanceof ImmutableListView<?>) {
-        // An immutable list view created by us, so we know how to get back at the mutable list.
-        return ((ImmutableListView<T>) value).asMutableList();
-      } else {
-        // Any kind of list created by someone else (for example using us as a general purpose
-        // InPlaceMutator), so assume it is mutable.
-        return value;
-      }
-    }
-
-    private List<T> toImmutableListView(List<T> value) {
-      if (value instanceof ImmutableListView) {
-        return value;
-      } else {
-        return new ImmutableListView<>(value);
-      }
-    }
-  }
-
-  private static final class ImmutableListView<T> extends AbstractList<T> implements RandomAccess {
-    private final List<T> mutableList;
-
-    ImmutableListView(List<T> mutableList) {
-      this.mutableList = mutableList;
-    }
-
-    List<T> asMutableList() {
-      return mutableList;
-    }
-
-    @Override
-    public T get(int i) {
-      return mutableList.get(i);
-    }
-
-    @Override
-    public int size() {
-      return mutableList.size();
-    }
   }
 }