Fix failing to serialize Collection or Map with inaccessible construc…

…tor (#1902)

* Remove UnsafeReflectionAccessor

Revert #1218

Usage of sun.misc.Unsafe to change internal AccessibleObject.override field
to suppress JPMS warnings goes against the intentions of the JPMS and does not
work anymore in newer versions, see #1540.
Therefore remove it and instead create a descriptive exception when making a
member accessible fails. If necessary users can also still use `java` command
line flags to open external modules.

* Fix failing to serialize Collection or Map with inaccessible constructor

Also remove tests which rely on Java implementation details.

* Don't keep reference to access exception of ConstructorConstructor

This also avoids a confusing stack trace, since the previously caught
exception might have had a complete unrelated stack trace.

* Remove Maven toolchain requirement

* Address review feedback

* Add back test for Security Manager

gson/pom.xml

@@ -1,4 +1,6 @@
-<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+  xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+  xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
   <modelVersion>4.0.0</modelVersion>
 
   <parent>
@@ -64,6 +66,21 @@
           <target>1.6</target>
         </configuration>
       </plugin>
+      <plugin>
+        <groupId>org.apache.maven.plugins</groupId>
+        <artifactId>maven-surefire-plugin</artifactId>
+        <version>3.0.0-M5</version>
+        <configuration>
+          <!-- Deny illegal access, this is required for ReflectionAccessTest -->
+          <!-- Requires Java >= 9; Important: In case future Java versions 
+            don't support this flag anymore, don't remove it unless CI also runs with 
+            that Java version. Ideally would use toolchain to specify that this should 
+            run with e.g. Java 11, but Maven toolchain requirements (unlike Gradle ones) 
+            don't seem to be portable (every developer would have to set up toolchain 
+            configuration locally). -->
+          <argLine>--illegal-access=deny</argLine>
+        </configuration>
+      </plugin>
       <plugin>
         <groupId>org.apache.maven.plugins</groupId>
         <artifactId>maven-javadoc-plugin</artifactId>

gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java

@@ -40,15 +40,14 @@
 
 import com.google.gson.InstanceCreator;
 import com.google.gson.JsonIOException;
-import com.google.gson.internal.reflect.ReflectionAccessor;
+import com.google.gson.internal.reflect.ReflectionHelper;
 import com.google.gson.reflect.TypeToken;
 
 /**
  * Returns a function that can construct an instance of a requested type.
  */
 public final class ConstructorConstructor {
   private final Map<Type, InstanceCreator<?>> instanceCreators;
-  private final ReflectionAccessor accessor = ReflectionAccessor.getInstance();
 
   public ConstructorConstructor(Map<Type, InstanceCreator<?>> instanceCreators) {
     this.instanceCreators = instanceCreators;
@@ -97,33 +96,52 @@ public <T> ObjectConstructor<T> get(TypeToken<T> typeToken) {
   }
 
   private <T> ObjectConstructor<T> newDefaultConstructor(Class<? super T> rawType) {
+    final Constructor<? super T> constructor;
     try {
-      final Constructor<? super T> constructor = rawType.getDeclaredConstructor();
-      if (!constructor.isAccessible()) {
-        accessor.makeAccessible(constructor);
-      }
+      constructor = rawType.getDeclaredConstructor();
+    } catch (NoSuchMethodException e) {
+      return null;
+    }
+
+    final String exceptionMessage = ReflectionHelper.tryMakeAccessible(constructor);
+    if (exceptionMessage != null) {
+      /*
+       * Create ObjectConstructor which throws exception.
+       * This keeps backward compatibility (compared to returning `null` which
+       * would then choose another way of creating object).
+       * And it supports types which are only serialized but not deserialized
+       * (compared to directly throwing exception here), e.g. when runtime type
+       * of object is inaccessible, but compile-time type is accessible.
+       */
       return new ObjectConstructor<T>() {
-        @SuppressWarnings("unchecked") // T is the same raw type as is requested
-        @Override public T construct() {
-          try {
-            Object[] args = null;
-            return (T) constructor.newInstance(args);
-          } catch (InstantiationException e) {
-            // TODO: JsonParseException ?
-            throw new RuntimeException("Failed to invoke " + constructor + " with no args", e);
-          } catch (InvocationTargetException e) {
-            // TODO: don't wrap if cause is unchecked!
-            // TODO: JsonParseException ?
-            throw new RuntimeException("Failed to invoke " + constructor + " with no args",
-                e.getTargetException());
-          } catch (IllegalAccessException e) {
-            throw new AssertionError(e);
-          }
+        @Override
+        public T construct() {
+          // New exception is created every time to avoid keeping reference
+          // to exception with potentially long stack trace, causing a
+          // memory leak
+          throw new JsonIOException(exceptionMessage);
         }
       };
-    } catch (NoSuchMethodException e) {
-      return null;
     }
+
+    return new ObjectConstructor<T>() {
+      @SuppressWarnings("unchecked") // T is the same raw type as is requested
+      @Override public T construct() {
+        try {
+          return (T) constructor.newInstance();
+        } catch (InstantiationException e) {
+          // TODO: JsonParseException ?
+          throw new RuntimeException("Failed to invoke " + constructor + " with no args", e);
+        } catch (InvocationTargetException e) {
+          // TODO: don't wrap if cause is unchecked!
+          // TODO: JsonParseException ?
+          throw new RuntimeException("Failed to invoke " + constructor + " with no args",
+              e.getTargetException());
+        } catch (IllegalAccessException e) {
+          throw new AssertionError(e);
+        }
+      }
+    };
   }
 
   /**

gson/src/main/java/com/google/gson/internal/bind/ReflectiveTypeAdapterFactory.java

@@ -28,7 +28,7 @@
 import com.google.gson.internal.Excluder;
 import com.google.gson.internal.ObjectConstructor;
 import com.google.gson.internal.Primitives;
-import com.google.gson.internal.reflect.ReflectionAccessor;
+import com.google.gson.internal.reflect.ReflectionHelper;
 import com.google.gson.reflect.TypeToken;
 import com.google.gson.stream.JsonReader;
 import com.google.gson.stream.JsonToken;
@@ -50,7 +50,6 @@ public final class ReflectiveTypeAdapterFactory implements TypeAdapterFactory {
   private final FieldNamingStrategy fieldNamingPolicy;
   private final Excluder excluder;
   private final JsonAdapterAnnotationTypeAdapterFactory jsonAdapterFactory;
-  private final ReflectionAccessor accessor = ReflectionAccessor.getInstance();
 
   public ReflectiveTypeAdapterFactory(ConstructorConstructor constructorConstructor,
       FieldNamingStrategy fieldNamingPolicy, Excluder excluder,
@@ -156,7 +155,7 @@ private Map<String, BoundField> getBoundFields(Gson context, TypeToken<?> type,
         if (!serialize && !deserialize) {
           continue;
         }
-        accessor.makeAccessible(field);
+        ReflectionHelper.makeAccessible(field);
         Type fieldType = $Gson$Types.resolve(type.getType(), raw, field.getGenericType());
         List<String> fieldNames = getFieldNames(field);
         BoundField previous = null;

gson/src/main/java/com/google/gson/internal/bind/TypeAdapters.java

@@ -17,6 +17,7 @@
 package com.google.gson.internal.bind;
 
 import java.io.IOException;
+import java.lang.reflect.AccessibleObject;
 import java.lang.reflect.Field;
 import java.math.BigDecimal;
 import java.math.BigInteger;
@@ -759,22 +760,31 @@ private static final class EnumTypeAdapter<T extends Enum<T>> extends TypeAdapte
     private final Map<String, T> nameToConstant = new HashMap<String, T>();
     private final Map<T, String> constantToName = new HashMap<T, String>();
 
-    public EnumTypeAdapter(Class<T> classOfT) {
+    public EnumTypeAdapter(final Class<T> classOfT) {
       try {
-        for (final Field field : classOfT.getDeclaredFields()) {
-          if (!field.isEnumConstant()) {
-            continue;
-          }
-          AccessController.doPrivileged(new PrivilegedAction<Void>() {
-            @Override public Void run() {
-              field.setAccessible(true);
-              return null;
+        // Uses reflection to find enum constants to work around name mismatches for obfuscated classes
+        // Reflection access might throw SecurityException, therefore run this in privileged context;
+        // should be acceptable because this only retrieves enum constants, but does not expose anything else
+        Field[] constantFields = AccessController.doPrivileged(new PrivilegedAction<Field[]>() {
+          @Override public Field[] run() {
+            Field[] fields = classOfT.getDeclaredFields();
+            ArrayList<Field> constantFieldsList = new ArrayList<Field>(fields.length);
+            for (Field f : fields) {
+              if (f.isEnumConstant()) {
+                constantFieldsList.add(f);
+              }
             }
-          });
+
+            Field[] constantFields = constantFieldsList.toArray(new Field[0]);
+            AccessibleObject.setAccessible(constantFields, true);
+            return constantFields;
+          }
+        });
+        for (Field constantField : constantFields) {
           @SuppressWarnings("unchecked")
-          T constant = (T)(field.get(null));
+          T constant = (T)(constantField.get(null));
           String name = constant.name();
-          SerializedName annotation = field.getAnnotation(SerializedName.class);
+          SerializedName annotation = constantField.getAnnotation(SerializedName.class);
           if (annotation != null) {
             name = annotation.value();
             for (String alternate : annotation.alternate()) {

gson/src/main/java/com/google/gson/internal/reflect/ReflectionHelper.java

@@ -0,0 +1,66 @@
+package com.google.gson.internal.reflect;
+
+import com.google.gson.JsonIOException;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Field;
+
+public class ReflectionHelper {
+  private ReflectionHelper() { }
+
+  /**
+   * Tries making the field accessible, wrapping any thrown exception in a
+   * {@link JsonIOException} with descriptive message.
+   *
+   * @param field field to make accessible
+   * @throws JsonIOException if making the field accessible fails
+   */
+  public static void makeAccessible(Field field) throws JsonIOException {
+    try {
+      field.setAccessible(true);
+    } catch (Exception exception) {
+      throw new JsonIOException("Failed making field '" + field.getDeclaringClass().getName() + "#"
+          + field.getName() + "' accessible; either change its visibility or write a custom "
+          + "TypeAdapter for its declaring type", exception);
+    }
+  }
+
+  /**
+   * Creates a string representation for a constructor.
+   * E.g.: {@code java.lang.String#String(char[], int, int)}
+   */
+  private static String constructorToString(Constructor<?> constructor) {
+    StringBuilder stringBuilder = new StringBuilder(constructor.getDeclaringClass().getName())
+      .append('#')
+      .append(constructor.getDeclaringClass().getSimpleName())
+      .append('(');
+    Class<?>[] parameters = constructor.getParameterTypes();
+    for (int i = 0; i < parameters.length; i++) {
+      if (i > 0) {
+        stringBuilder.append(", ");
+      }
+      stringBuilder.append(parameters[i].getSimpleName());
+    }
+
+    return stringBuilder.append(')').toString();
+  }
+
+  /**
+   * Tries making the constructor accessible, returning an exception message
+   * if this fails.
+   *
+   * @param constructor constructor to make accessible
+   * @return exception message; {@code null} if successful, non-{@code null} if
+   *    unsuccessful
+   */
+  public static String tryMakeAccessible(Constructor<?> constructor) {
+    try {
+      constructor.setAccessible(true);
+      return null;
+    } catch (Exception exception) {
+      return "Failed making constructor '" + constructorToString(constructor) + "' accessible; "
+          + "either change its visibility or write a custom InstanceCreator or TypeAdapter for its declaring type: "
+          // Include the message since it might contain more detailed information
+          + exception.getMessage();
+    }
+  }
+}