Fix failing to serialize Collection or Map with inaccessible construc…

…tor (google#1902)

* Remove UnsafeReflectionAccessor

Revert google#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 google#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

@@ -39,7 +39,7 @@
 
 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;
 
 /**
@@ -48,7 +48,6 @@
 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,44 +96,53 @@ 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 (!accessor.makeAccessible(constructor)) {
-          return new ObjectConstructor<T>() {
-
-            private final String message = rawType.getCanonicalName() + " has no accessible constructor, can only be serialized";
+      constructor = rawType.getDeclaredConstructor();
+    } catch (NoSuchMethodException e) {
+      return null;
+    }
 
-            @Override
-            public T construct() {
-              throw new RuntimeException(message);
-            }
-          };
+    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>() {
+        @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);
         }
-        return new ObjectConstructor<T>() {
-
-            private final String message = "Failed to invoke " + constructor + " with no args";
+      };
+    }
 
-            @SuppressWarnings("unchecked") // T is the same raw type as is requested
-        @Override public T construct() {
-          try {
-            return (T) constructor.newInstance((Object[]) null);
-          } catch (InstantiationException e) {
-            // TODO: JsonParseException ?
-              throw new RuntimeException(message, e);
-          } catch (InvocationTargetException e) {
-              final Throwable cause = e.getTargetException();
+    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) {
+            final Throwable cause = e.getTargetException();
             // TODO: JsonParseException ?
-              throw cause instanceof RuntimeException
-                    ? (RuntimeException) cause
-                    : new RuntimeException(message, cause);
-          } catch (IllegalAccessException e) {
-            throw new InvalidStateException(e);
-          }
+            throw cause instanceof RuntimeException
+                  ? (RuntimeException) cause
+                  : new RuntimeException("Failed to invoke " + constructor + " with no args", cause);
+        } catch (IllegalAccessException e) {
+          throw new InvalidStateException(e);
         }
-      };
-    } catch (NoSuchMethodException e) {
-      return null;
-    }
+      }
+    };
   }
 
   /**

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

@@ -29,7 +29,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;
@@ -51,7 +51,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,
@@ -157,9 +156,7 @@ private Map<String, BoundField> getBoundFields(Gson context, TypeToken<?> type,
         if (!serialize && !deserialize) {
           continue;
         }
-        if (!accessor.makeAccessible(field)) {
-          throw new InvalidStateException("Cannot access field " + 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,13 +17,16 @@
 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;
 import java.net.InetAddress;
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.net.URL;
+import java.security.AccessController;
+import java.security.PrivilegedAction;
 import java.util.ArrayList;
 import java.util.BitSet;
 import java.util.Calendar;
@@ -734,17 +737,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;
+        // 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;
           }
-          field.setAccessible(true);
+        });
+        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,76 @@
+/*
+ * Copyright (C) 2017 The Gson authors
+ *
+ * Licensed 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 com.google.gson.internal.reflect;
+
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.MethodHandles;
+import java.lang.reflect.AccessibleObject;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Method;
+
+import com.google.gson.JsonIOException;
+
+public class ReflectionHelper {
+  private ReflectionHelper() { }
+
+  private static final MethodHandle trySetAccessible = getTrySetAccessibleMethod();
+
+  /**
+   * Tries making the member accessible, wrapping any thrown exception in a
+   * {@link JsonIOException} with descriptive message.
+   *
+   * @param ao member to make accessible
+   */
+  public static void makeAccessible(AccessibleObject ao) {
+    try {
+      if (trySetAccessible != null) {
+        trySetAccessible.invoke(ao);
+      } else if (!ao.isAccessible()) {  // if trySetAccessible() is not available then neither is canAccess()
+        ao.setAccessible(true);
+      }
+    } catch (Throwable error) {
+        throw new JsonIOException("Failed making " + ao + " accessible;" +
+                                  " either change its visibility or write a custom "
+                                  + (ao instanceof Constructor ? "InstanceCreator or " : "")
+                                  + "TypeAdapter for its declaring type", error);
+    }
+  }
+
+  /**
+   * Tries making the member accessible
+   *
+   * @param ao member to make accessible
+   * @return exception message; {@code null} if successful, non-{@code null} if
+   *    unsuccessful
+   */
+  public static String tryMakeAccessible(AccessibleObject ao) {
+    try {
+      makeAccessible(ao);  
+      return null;
+    } catch (JsonIOException error) {
+      return error.getMessage();
+    }
+  }
+
+  private static MethodHandle getTrySetAccessibleMethod() {
+    try {
+        Method method = AccessibleObject.class.getMethod("trySetAccessible");
+        return MethodHandles.publicLookup().unreflect(method);
+    } catch (Exception e) {
+      return null;
+    }
+  }
+}

gson/src/test/java/com/google/gson/functional/EnumTest.java

@@ -200,17 +200,17 @@ public enum Gender {
   }
 
   public void testEnumClassWithFields() {
-	  assertEquals("\"RED\"", gson.toJson(Color.RED));
-	  assertEquals("red", gson.fromJson("RED", Color.class).value);
+    assertEquals("\"RED\"", gson.toJson(Color.RED));
+    assertEquals("red", gson.fromJson("RED", Color.class).value);
   }
 
   public enum Color {
-	  RED("red", 1), BLUE("blue", 2), GREEN("green", 3);
-	  String value;
-	  int index;
-	  private Color(String value, int index) {
-		  this.value = value;
-		  this.index = index;
-	  }
+    RED("red", 1), BLUE("blue", 2), GREEN("green", 3);
+    String value;
+    int index;
+    private Color(String value, int index) {
+      this.value = value;
+      this.index = index;
+    }
   }
 }