From 72761f43f41acad5288be20cf139651a9a014925 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Tue, 9 Nov 2021 16:16:35 +0100 Subject: [PATCH] Fix failing to serialize Collection or Map with inaccessible constructor (#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 | 19 ++- .../gson/internal/ConstructorConstructor.java | 76 ++++++----- .../bind/ReflectiveTypeAdapterFactory.java | 7 +- .../gson/internal/bind/TypeAdapters.java | 31 ++++- .../internal/reflect/ReflectionAccessor.java | 78 ----------- .../internal/reflect/ReflectionHelper.java | 76 +++++++++++ .../com/google/gson/functional/EnumTest.java | 18 +-- .../gson/functional/ReflectionAccessTest.java | 123 ++++++++++++++++++ 8 files changed, 294 insertions(+), 134 deletions(-) delete mode 100644 gson/src/main/java/com/google/gson/internal/reflect/ReflectionAccessor.java create mode 100644 gson/src/main/java/com/google/gson/internal/reflect/ReflectionHelper.java create mode 100644 gson/src/test/java/com/google/gson/functional/ReflectionAccessTest.java diff --git a/gson/pom.xml b/gson/pom.xml index d676229276..cbaa685a62 100644 --- a/gson/pom.xml +++ b/gson/pom.xml @@ -1,4 +1,6 @@ - + 4.0.0 @@ -64,6 +66,21 @@ 1.6 + + org.apache.maven.plugins + maven-surefire-plugin + 3.0.0-M5 + + + + --illegal-access=deny + + org.apache.maven.plugins maven-javadoc-plugin diff --git a/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java b/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java index cbbecbefce..57f763f12a 100644 --- a/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java +++ b/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> instanceCreators; - private final ReflectionAccessor accessor = ReflectionAccessor.getInstance(); public ConstructorConstructor(Map> instanceCreators) { this.instanceCreators = instanceCreators; @@ -97,44 +96,53 @@ public ObjectConstructor get(TypeToken typeToken) { } private ObjectConstructor newDefaultConstructor(Class rawType) { + final Constructor constructor; try { - final Constructor constructor = rawType.getDeclaredConstructor(); - if (!accessor.makeAccessible(constructor)) { - return new ObjectConstructor() { - - 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() { + @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() { - - 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() { + @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; - } + } + }; } /** diff --git a/gson/src/main/java/com/google/gson/internal/bind/ReflectiveTypeAdapterFactory.java b/gson/src/main/java/com/google/gson/internal/bind/ReflectiveTypeAdapterFactory.java index 56d9cdb05e..8a14fabd99 100644 --- a/gson/src/main/java/com/google/gson/internal/bind/ReflectiveTypeAdapterFactory.java +++ b/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 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 fieldNames = getFieldNames(field); BoundField previous = null; diff --git a/gson/src/main/java/com/google/gson/internal/bind/TypeAdapters.java b/gson/src/main/java/com/google/gson/internal/bind/TypeAdapters.java index ede3daa041..4bc4c10795 100644 --- a/gson/src/main/java/com/google/gson/internal/bind/TypeAdapters.java +++ b/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; @@ -24,6 +25,8 @@ 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> extends TypeAdapte private final Map nameToConstant = new HashMap(); private final Map constantToName = new HashMap(); - public EnumTypeAdapter(Class classOfT) { + public EnumTypeAdapter(final Class 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() { + @Override public Field[] run() { + Field[] fields = classOfT.getDeclaredFields(); + ArrayList constantFieldsList = new ArrayList(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()) { diff --git a/gson/src/main/java/com/google/gson/internal/reflect/ReflectionAccessor.java b/gson/src/main/java/com/google/gson/internal/reflect/ReflectionAccessor.java deleted file mode 100644 index 5fca2c4907..0000000000 --- a/gson/src/main/java/com/google/gson/internal/reflect/ReflectionAccessor.java +++ /dev/null @@ -1,78 +0,0 @@ -/* - * 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.Method; - -/** - * Provides a replacement for {@link AccessibleObject#setAccessible(boolean)}, which may be used to - * avoid reflective access issues appeared in Java 9, like {@link java.lang.reflect.InaccessibleObjectException} - * thrown or warnings like - * 
- *   WARNING: An illegal reflective access operation has occurred
- *   WARNING: Illegal reflective access by ...
- *
- *

- * Works both for Java 9 and earlier Java versions. - */ -public final class ReflectionAccessor { - - // the singleton instance, use getInstance() to obtain - private static final ReflectionAccessor instance = new ReflectionAccessor(); - - private final MethodHandle trySetAccessible = getTrySetAccessibleMethod(); - - /** - * Does the same as {@code ao.setAccessible(true)}, but never throws - * {@link java.lang.reflect.InaccessibleObjectException} - * @return true if the object is accessible. - */ - public boolean makeAccessible(AccessibleObject ao) { - try { - if (trySetAccessible != null) { - return (boolean) trySetAccessible.invoke(ao); - } else if (!ao.isAccessible()) { // if trySetAccessible() is not available then neither is canAccess() - ao.setAccessible(true); - } - return true; - } catch (final Throwable error) { - return false; - } - } - - /** - * Obtains a {@link ReflectionAccessor} instance. - *

- * You may need one if a reflective operation in your code throws {@link java.lang.reflect.InaccessibleObjectException}. - * In such a case, use {@link ReflectionAccessor#makeAccessible(AccessibleObject)} on a field, method or constructor - * (instead of basic {@link AccessibleObject#setAccessible(boolean)}). - */ - public static ReflectionAccessor getInstance() { - return instance; - } - - private static MethodHandle getTrySetAccessibleMethod() { - try { - Method method = AccessibleObject.class.getMethod("trySetAccessible"); - return MethodHandles.publicLookup().unreflect(method); - } catch (Exception e) { - return null; - } - } -} diff --git a/gson/src/main/java/com/google/gson/internal/reflect/ReflectionHelper.java b/gson/src/main/java/com/google/gson/internal/reflect/ReflectionHelper.java new file mode 100644 index 0000000000..921b227af9 --- /dev/null +++ b/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; + } + } +} diff --git a/gson/src/test/java/com/google/gson/functional/EnumTest.java b/gson/src/test/java/com/google/gson/functional/EnumTest.java index 66b855ebfc..9486f08c09 100644 --- a/gson/src/test/java/com/google/gson/functional/EnumTest.java +++ b/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; + } } } diff --git a/gson/src/test/java/com/google/gson/functional/ReflectionAccessTest.java b/gson/src/test/java/com/google/gson/functional/ReflectionAccessTest.java new file mode 100644 index 0000000000..e262e10361 --- /dev/null +++ b/gson/src/test/java/com/google/gson/functional/ReflectionAccessTest.java @@ -0,0 +1,123 @@ +package com.google.gson.functional; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import com.google.gson.Gson; +import com.google.gson.GsonBuilder; +import com.google.gson.JsonIOException; +import com.google.gson.TypeAdapter; +import com.google.gson.stream.JsonReader; +import com.google.gson.stream.JsonWriter; +import java.io.IOException; +import java.lang.reflect.ReflectPermission; +import java.net.URL; +import java.net.URLClassLoader; +import java.security.Permission; +import java.util.Collections; +import java.util.concurrent.atomic.AtomicBoolean; +import org.junit.Test; + +public class ReflectionAccessTest { + @SuppressWarnings("unused") + private static class ClassWithPrivateMembers { + private String s; + + private ClassWithPrivateMembers() { + } + } + + private static Class loadClassWithDifferentClassLoader(Class c) throws Exception { + URL url = c.getProtectionDomain().getCodeSource().getLocation(); + URLClassLoader classLoader = new URLClassLoader(new URL[] { url }, null); + return classLoader.loadClass(c.getName()); + } + + @Test + public void testRestrictiveSecurityManager() throws Exception { + // Must use separate class loader, otherwise permission is not checked, see Class.getDeclaredFields() + Class clazz = loadClassWithDifferentClassLoader(ClassWithPrivateMembers.class); + + final Permission accessDeclaredMembers = new RuntimePermission("accessDeclaredMembers"); + final Permission suppressAccessChecks = new ReflectPermission("suppressAccessChecks"); + SecurityManager original = System.getSecurityManager(); + SecurityManager restrictiveManager = new SecurityManager() { + @Override + public void checkPermission(Permission perm) { + if (accessDeclaredMembers.equals(perm)) { + throw new SecurityException("Gson: no-member-access"); + } + if (suppressAccessChecks.equals(perm)) { + throw new SecurityException("Gson: no-suppress-access-check"); + } + } + }; + System.setSecurityManager(restrictiveManager); + + try { + Gson gson = new Gson(); + try { + // Getting reflection based adapter should fail + gson.getAdapter(clazz); + fail(); + } catch (SecurityException e) { + assertEquals("Gson: no-member-access", e.getMessage()); + } + + final AtomicBoolean wasReadCalled = new AtomicBoolean(false); + gson = new GsonBuilder() + .registerTypeAdapter(clazz, new TypeAdapter() { + @Override + public void write(JsonWriter out, Object value) throws IOException { + out.value("custom-write"); + } + + @Override + public Object read(JsonReader in) throws IOException { + in.skipValue(); + wasReadCalled.set(true); + return null; + }} + ) + .create(); + + assertEquals("\"custom-write\"", gson.toJson(null, clazz)); + assertNull(gson.fromJson("{}", clazz)); + assertTrue(wasReadCalled.get()); + } finally { + System.setSecurityManager(original); + } + } + + /** + * Test serializing an instance of a non-accessible internal class, but where + * Gson supports serializing one of its superinterfaces. + * + *

Here {@link Collections#emptyList()} is used which returns an instance + * of the internal class {@code java.util.Collections.EmptyList}. Gson should + * serialize the object as {@code List} despite the internal class not being + * accessible. + * + *

See https://github.com/google/gson/issues/1875 + */ + @Test + public void testSerializeInternalImplementationObject() { + Gson gson = new Gson(); + String json = gson.toJson(Collections.emptyList()); + assertEquals("[]", json); + + // But deserialization should fail + Class internalClass = Collections.emptyList().getClass(); + try { + gson.fromJson("{}", internalClass); + fail("Missing exception; test has to be run with `--illegal-access=deny`"); + } catch (JsonIOException expected) { + assertTrue(expected.getMessage().startsWith( + "Failed making private java.util.Collections$EmptyList() accessible; " + + "either change its visibility or write a custom InstanceCreator or TypeAdapter for its declaring type" + )); + } + } +}