From 84afa36ad01e4d719843ecc2e0a7b6c9d33b613d Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Fri, 4 Jun 2021 00:24:23 +0200 Subject: [PATCH 1/6] 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. --- .../gson/internal/ConstructorConstructor.java | 7 +- .../bind/ReflectiveTypeAdapterFactory.java | 5 +- .../reflect/PreJava9ReflectionAccessor.java | 33 ------- .../internal/reflect/ReflectionAccessor.java | 54 ------------ .../internal/reflect/ReflectionHelper.java | 25 ++++++ .../reflect/UnsafeReflectionAccessor.java | 86 ------------------- .../reflect/UnsafeReflectionAccessorTest.java | 51 ----------- 7 files changed, 29 insertions(+), 232 deletions(-) delete mode 100644 gson/src/main/java/com/google/gson/internal/reflect/PreJava9ReflectionAccessor.java 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 delete mode 100644 gson/src/main/java/com/google/gson/internal/reflect/UnsafeReflectionAccessor.java delete mode 100644 gson/src/test/java/com/google/gson/internal/reflect/UnsafeReflectionAccessorTest.java 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 5fab460105..e64c575301 100644 --- a/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java +++ b/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java @@ -40,7 +40,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; @@ -99,9 +98,7 @@ public ObjectConstructor get(TypeToken typeToken) { private ObjectConstructor newDefaultConstructor(Class rawType) { try { final Constructor constructor = rawType.getDeclaredConstructor(); - if (!constructor.isAccessible()) { - accessor.makeAccessible(constructor); - } + ReflectionHelper.makeAccessible(constructor); return new ObjectConstructor() { @SuppressWarnings("unchecked") // T is the same raw type as is requested @Override public T construct() { 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 777e7dee35..21c049e23c 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 @@ -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 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 fieldNames = getFieldNames(field); BoundField previous = null; diff --git a/gson/src/main/java/com/google/gson/internal/reflect/PreJava9ReflectionAccessor.java b/gson/src/main/java/com/google/gson/internal/reflect/PreJava9ReflectionAccessor.java deleted file mode 100644 index 325274e224..0000000000 --- a/gson/src/main/java/com/google/gson/internal/reflect/PreJava9ReflectionAccessor.java +++ /dev/null @@ -1,33 +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.reflect.AccessibleObject; - -/** - * A basic implementation of {@link ReflectionAccessor} which is suitable for Java 8 and below. - *

- * This implementation just calls {@link AccessibleObject#setAccessible(boolean) setAccessible(true)}, which worked - * fine before Java 9. - */ -final class PreJava9ReflectionAccessor extends ReflectionAccessor { - - /** {@inheritDoc} */ - @Override - public void makeAccessible(AccessibleObject ao) { - ao.setAccessible(true); - } -} 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 6816feaf2b..0000000000 --- a/gson/src/main/java/com/google/gson/internal/reflect/ReflectionAccessor.java +++ /dev/null @@ -1,54 +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.reflect.AccessibleObject; - -import com.google.gson.internal.JavaVersion; - -/** - * 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 abstract class ReflectionAccessor { - - // the singleton instance, use getInstance() to obtain - private static final ReflectionAccessor instance = JavaVersion.getMajorJavaVersion() < 9 ? new PreJava9ReflectionAccessor() : new UnsafeReflectionAccessor(); - - /** - * Does the same as {@code ao.setAccessible(true)}, but never throws - * {@link java.lang.reflect.InaccessibleObjectException} - */ - public abstract void makeAccessible(AccessibleObject ao); - - /** - * Obtains a {@link ReflectionAccessor} instance suitable for the current Java version. - *

- * You may need one 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; - } -} 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..a113204687 --- /dev/null +++ b/gson/src/main/java/com/google/gson/internal/reflect/ReflectionHelper.java @@ -0,0 +1,25 @@ +package com.google.gson.internal.reflect; + +import java.lang.reflect.AccessibleObject; + +import com.google.gson.JsonIOException; + +public class ReflectionHelper { + private ReflectionHelper() { } + + /** + * Tries making the object accessible, wrapping any thrown exception in a + * {@link JsonIOException} with descriptive message. + * + * @param object Object to make accessible + * @throws JsonIOException If making the object accessible fails + */ + public static void makeAccessible(AccessibleObject object) throws JsonIOException { + try { + object.setAccessible(true); + } catch (Exception exception) { + throw new JsonIOException("Failed making '" + object + "' accessible; either change its visibility " + + "or write a custom TypeAdapter for its declaring type", exception); + } + } +} diff --git a/gson/src/main/java/com/google/gson/internal/reflect/UnsafeReflectionAccessor.java b/gson/src/main/java/com/google/gson/internal/reflect/UnsafeReflectionAccessor.java deleted file mode 100644 index 749335b77a..0000000000 --- a/gson/src/main/java/com/google/gson/internal/reflect/UnsafeReflectionAccessor.java +++ /dev/null @@ -1,86 +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.reflect.AccessibleObject; -import java.lang.reflect.Field; -import java.lang.reflect.Method; - -import com.google.gson.JsonIOException; - -/** - * An implementation of {@link ReflectionAccessor} based on {@link Unsafe}. - *

- * NOTE: This implementation is designed for Java 9. Although it should work with earlier Java releases, it is better to - * use {@link PreJava9ReflectionAccessor} for them. - */ -@SuppressWarnings({"unchecked", "rawtypes"}) -final class UnsafeReflectionAccessor extends ReflectionAccessor { - - private static Class unsafeClass; - private final Object theUnsafe = getUnsafeInstance(); - private final Field overrideField = getOverrideField(); - - /** {@inheritDoc} */ - @Override - public void makeAccessible(AccessibleObject ao) { - boolean success = makeAccessibleWithUnsafe(ao); - if (!success) { - try { - // unsafe couldn't be found, so try using accessible anyway - ao.setAccessible(true); - } catch (SecurityException e) { - throw new JsonIOException("Gson couldn't modify fields for " + ao - + "\nand sun.misc.Unsafe not found.\nEither write a custom type adapter," - + " or make fields accessible, or include sun.misc.Unsafe.", e); - } - } - } - - // Visible for testing only - boolean makeAccessibleWithUnsafe(AccessibleObject ao) { - if (theUnsafe != null && overrideField != null) { - try { - Method method = unsafeClass.getMethod("objectFieldOffset", Field.class); - long overrideOffset = (Long) method.invoke(theUnsafe, overrideField); // long overrideOffset = theUnsafe.objectFieldOffset(overrideField); - Method putBooleanMethod = unsafeClass.getMethod("putBoolean", Object.class, long.class, boolean.class); - putBooleanMethod.invoke(theUnsafe, ao, overrideOffset, true); // theUnsafe.putBoolean(ao, overrideOffset, true); - return true; - } catch (Exception ignored) { // do nothing - } - } - return false; - } - - private static Object getUnsafeInstance() { - try { - unsafeClass = Class.forName("sun.misc.Unsafe"); - Field unsafeField = unsafeClass.getDeclaredField("theUnsafe"); - unsafeField.setAccessible(true); - return unsafeField.get(null); - } catch (Exception e) { - return null; - } - } - - private static Field getOverrideField() { - try { - return AccessibleObject.class.getDeclaredField("override"); - } catch (NoSuchFieldException e) { - return null; - } - } -} diff --git a/gson/src/test/java/com/google/gson/internal/reflect/UnsafeReflectionAccessorTest.java b/gson/src/test/java/com/google/gson/internal/reflect/UnsafeReflectionAccessorTest.java deleted file mode 100644 index d5caaf5375..0000000000 --- a/gson/src/test/java/com/google/gson/internal/reflect/UnsafeReflectionAccessorTest.java +++ /dev/null @@ -1,51 +0,0 @@ -/* - * Copyright (C) 2018 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 static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; - -import java.lang.reflect.Field; - -import org.junit.Test; - -/** - * Unit tests for {@link UnsafeReflectionAccessor} - * - * @author Inderjeet Singh - */ -public class UnsafeReflectionAccessorTest { - - @Test - public void testMakeAccessibleWithUnsafe() throws Exception { - UnsafeReflectionAccessor accessor = new UnsafeReflectionAccessor(); - Field field = ClassWithPrivateFinalFields.class.getDeclaredField("a"); - try { - boolean success = accessor.makeAccessibleWithUnsafe(field); - assertTrue(success); - } catch (Exception e) { - fail("Unsafe didn't work on the JDK"); - } - } - - @SuppressWarnings("unused") - private static final class ClassWithPrivateFinalFields { - private final String a; - public ClassWithPrivateFinalFields(String a) { - this.a = a; - } - } -} From 9620747bffdca3c5c7e6deeedb5ecb5b29393d03 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Fri, 4 Jun 2021 00:45:45 +0200 Subject: [PATCH 2/6] Fix failing to serialize Collection or Map with inaccessible constructor Also remove tests which rely on Java implementation details. --- gson/pom.xml | 16 ++++- .../gson/internal/ConstructorConstructor.java | 58 +++++++++++------ .../internal/reflect/ReflectionHelper.java | 55 +++++++++++++--- .../functional/DefaultTypeAdaptersTest.java | 1 - .../gson/functional/ReflectionAccessTest.java | 44 +++++++++++++ .../functional/ThrowableFunctionalTest.java | 65 ------------------- .../bind/RecursiveTypesResolveTest.java | 14 ---- 7 files changed, 145 insertions(+), 108 deletions(-) create mode 100644 gson/src/test/java/com/google/gson/functional/ReflectionAccessTest.java delete mode 100644 gson/src/test/java/com/google/gson/functional/ThrowableFunctionalTest.java diff --git a/gson/pom.xml b/gson/pom.xml index 83f52b3a29..b5b17a4a64 100644 --- a/gson/pom.xml +++ b/gson/pom.xml @@ -17,9 +17,22 @@ test - + + + org.apache.maven.plugins + maven-surefire-plugin + 3.0.0-M5 + + + + 11 + + + --illegal-access=deny + + org.apache.maven.plugins maven-javadoc-plugin @@ -46,6 +59,7 @@ org.apache.maven.plugins maven-jar-plugin + 3.2.0 ${project.build.outputDirectory}/META-INF/MANIFEST.MF 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 e64c575301..e114907c6e 100644 --- a/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java +++ b/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java @@ -96,31 +96,51 @@ public ObjectConstructor get(TypeToken typeToken) { } private ObjectConstructor newDefaultConstructor(Class rawType) { + final Constructor constructor; + try { + constructor = rawType.getDeclaredConstructor(); + } catch (NoSuchMethodException e) { + return null; + } + try { - final Constructor constructor = rawType.getDeclaredConstructor(); ReflectionHelper.makeAccessible(constructor); + } catch (final JsonIOException accessibilityException) { + /* + * Create ObjectConstructor which rethrows 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 rethrowing exception), e.g. when runtime type + * of object is inaccessible, but compile-time type is accessible. + */ return new ObjectConstructor() { - @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() { + throw accessibilityException; } }; - } catch (NoSuchMethodException e) { - return null; } + + return new ObjectConstructor() { + @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); + } + } + }; } /** 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 index a113204687..9af167dc3b 100644 --- a/gson/src/main/java/com/google/gson/internal/reflect/ReflectionHelper.java +++ b/gson/src/main/java/com/google/gson/internal/reflect/ReflectionHelper.java @@ -1,6 +1,7 @@ package com.google.gson.internal.reflect; -import java.lang.reflect.AccessibleObject; +import java.lang.reflect.Constructor; +import java.lang.reflect.Field; import com.google.gson.JsonIOException; @@ -8,18 +9,56 @@ public class ReflectionHelper { private ReflectionHelper() { } /** - * Tries making the object accessible, wrapping any thrown exception in a + * Tries making the field accessible, wrapping any thrown exception in a * {@link JsonIOException} with descriptive message. * - * @param object Object to make accessible - * @throws JsonIOException If making the object accessible fails + * @param field Field to make accessible + * @throws JsonIOException If making the field accessible fails */ - public static void makeAccessible(AccessibleObject object) throws JsonIOException { + public static void makeAccessible(Field field) throws JsonIOException { try { - object.setAccessible(true); + field.setAccessible(true); } catch (Exception exception) { - throw new JsonIOException("Failed making '" + object + "' accessible; either change its visibility " - + "or write a custom TypeAdapter for its declaring type", 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, wrapping any thrown exception in a + * {@link JsonIOException} with descriptive message. + * + * @param constructor Constructor to make accessible + * @throws JsonIOException If making the constructor accessible fails + */ + public static void makeAccessible(Constructor constructor) throws JsonIOException { + try { + constructor.setAccessible(true); + } catch (Exception exception) { + throw new JsonIOException("Failed making constructor '" + constructorToString(constructor) + + "' accessible; either change its visibility or write a custom InstanceCreator for its " + + "declaring type", exception); } } } diff --git a/gson/src/test/java/com/google/gson/functional/DefaultTypeAdaptersTest.java b/gson/src/test/java/com/google/gson/functional/DefaultTypeAdaptersTest.java index b7307c6fce..40449f7ac0 100644 --- a/gson/src/test/java/com/google/gson/functional/DefaultTypeAdaptersTest.java +++ b/gson/src/test/java/com/google/gson/functional/DefaultTypeAdaptersTest.java @@ -178,7 +178,6 @@ public void testNullSerialization() throws Exception { testNullSerializationAndDeserialization(Time.class); testNullSerializationAndDeserialization(Timestamp.class); testNullSerializationAndDeserialization(java.sql.Date.class); - testNullSerializationAndDeserialization(Enum.class); testNullSerializationAndDeserialization(Class.class); } 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..616d101323 --- /dev/null +++ b/gson/src/test/java/com/google/gson/functional/ReflectionAccessTest.java @@ -0,0 +1,44 @@ +package com.google.gson.functional; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; + +import java.util.Collections; + +import org.junit.Test; + +import com.google.gson.Gson; +import com.google.gson.JsonIOException; + +public class ReflectionAccessTest { + /** + * 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) { + assertEquals( + "Failed making constructor 'java.util.Collections$EmptyList#EmptyList()' accessible; " + + "either change its visibility or write a custom InstanceCreator for its declaring type", + expected.getMessage() + ); + } + } +} diff --git a/gson/src/test/java/com/google/gson/functional/ThrowableFunctionalTest.java b/gson/src/test/java/com/google/gson/functional/ThrowableFunctionalTest.java deleted file mode 100644 index f6ae748a56..0000000000 --- a/gson/src/test/java/com/google/gson/functional/ThrowableFunctionalTest.java +++ /dev/null @@ -1,65 +0,0 @@ -// Copyright (C) 2014 Trymph Inc. -package com.google.gson.functional; - -import java.io.IOException; - -import junit.framework.TestCase; - -import com.google.gson.Gson; -import com.google.gson.annotations.SerializedName; - -@SuppressWarnings("serial") -public final class ThrowableFunctionalTest extends TestCase { - private final Gson gson = new Gson(); - - public void testExceptionWithoutCause() { - RuntimeException e = new RuntimeException("hello"); - String json = gson.toJson(e); - assertTrue(json.contains("hello")); - - e = gson.fromJson("{'detailMessage':'hello'}", RuntimeException.class); - assertEquals("hello", e.getMessage()); - } - - public void testExceptionWithCause() { - Exception e = new Exception("top level", new IOException("io error")); - String json = gson.toJson(e); - assertTrue(json.contains("{\"detailMessage\":\"top level\",\"cause\":{\"detailMessage\":\"io error\"")); - - e = gson.fromJson("{'detailMessage':'top level','cause':{'detailMessage':'io error'}}", Exception.class); - assertEquals("top level", e.getMessage()); - assertTrue(e.getCause() instanceof Throwable); // cause is not parameterized so type info is lost - assertEquals("io error", e.getCause().getMessage()); - } - - public void testSerializedNameOnExceptionFields() { - MyException e = new MyException(); - String json = gson.toJson(e); - assertTrue(json.contains("{\"my_custom_name\":\"myCustomMessageValue\"")); - } - - public void testErrorWithoutCause() { - OutOfMemoryError e = new OutOfMemoryError("hello"); - String json = gson.toJson(e); - assertTrue(json.contains("hello")); - - e = gson.fromJson("{'detailMessage':'hello'}", OutOfMemoryError.class); - assertEquals("hello", e.getMessage()); - } - - public void testErrornWithCause() { - Error e = new Error("top level", new IOException("io error")); - String json = gson.toJson(e); - assertTrue(json.contains("top level")); - assertTrue(json.contains("io error")); - - e = gson.fromJson("{'detailMessage':'top level','cause':{'detailMessage':'io error'}}", Error.class); - assertEquals("top level", e.getMessage()); - assertTrue(e.getCause() instanceof Throwable); // cause is not parameterized so type info is lost - assertEquals("io error", e.getCause().getMessage()); - } - - private static final class MyException extends Throwable { - @SerializedName("my_custom_name") String myCustomMessage = "myCustomMessageValue"; - } -} diff --git a/gson/src/test/java/com/google/gson/internal/bind/RecursiveTypesResolveTest.java b/gson/src/test/java/com/google/gson/internal/bind/RecursiveTypesResolveTest.java index 730183fbee..f9f1cd9975 100644 --- a/gson/src/test/java/com/google/gson/internal/bind/RecursiveTypesResolveTest.java +++ b/gson/src/test/java/com/google/gson/internal/bind/RecursiveTypesResolveTest.java @@ -52,20 +52,6 @@ public void testRecursiveResolveSimple() { assertNotNull(adapter); } - /** - * Real-world samples, found in Issues #603 and #440. - */ - - public void testIssue603PrintStream() { - TypeAdapter adapter = new Gson().getAdapter(PrintStream.class); - assertNotNull(adapter); - } - - public void testIssue440WeakReference() throws Exception { - TypeAdapter adapter = new Gson().getAdapter(WeakReference.class); - assertNotNull(adapter); - } - /** * Tests belows check the behaviour of the methods changed for the fix. */ From 3acc2e4c42276eab1d24964068cb187fbc36f399 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Fri, 4 Jun 2021 02:22:25 +0200 Subject: [PATCH 3/6] 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. --- .../gson/internal/ConstructorConstructor.java | 14 +++++----- .../internal/reflect/ReflectionHelper.java | 27 ++++++++++++------- .../gson/functional/ReflectionAccessTest.java | 8 +++--- 3 files changed, 29 insertions(+), 20 deletions(-) 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 e114907c6e..d5590c6e46 100644 --- a/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java +++ b/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java @@ -103,21 +103,23 @@ private ObjectConstructor newDefaultConstructor(Class rawType) return null; } - try { - ReflectionHelper.makeAccessible(constructor); - } catch (final JsonIOException accessibilityException) { + final String exceptionMessage = ReflectionHelper.tryMakeAccessible(constructor); + if (exceptionMessage != null) { /* - * Create ObjectConstructor which rethrows exception. + * 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 rethrowing exception), e.g. when runtime type + * (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() { - throw accessibilityException; + // 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); } }; } 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 index 9af167dc3b..fb8305dd62 100644 --- a/gson/src/main/java/com/google/gson/internal/reflect/ReflectionHelper.java +++ b/gson/src/main/java/com/google/gson/internal/reflect/ReflectionHelper.java @@ -12,8 +12,10 @@ 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 + * @param field + * Field to make accessible + * @throws JsonIOException + * If making the field accessible fails */ public static void makeAccessible(Field field) throws JsonIOException { try { @@ -46,19 +48,24 @@ private static String constructorToString(Constructor constructor) { } /** - * Tries making the constructor accessible, wrapping any thrown exception in a - * {@link JsonIOException} with descriptive message. + * Tries making the constructor accessible, returning an exception message + * if this fails. * - * @param constructor Constructor to make accessible - * @throws JsonIOException If making the constructor accessible fails + * @param constructor + * Constructor to make accessible + * @return + * Exception message; {@code null} if successful, non-{@code null} if + * unsuccessful */ - public static void makeAccessible(Constructor constructor) throws JsonIOException { + public static String tryMakeAccessible(Constructor constructor) { try { constructor.setAccessible(true); + return null; } catch (Exception exception) { - throw new JsonIOException("Failed making constructor '" + constructorToString(constructor) - + "' accessible; either change its visibility or write a custom InstanceCreator for its " - + "declaring type", exception); + return "Failed making constructor '" + constructorToString(constructor) + "' accessible; " + + "either change its visibility or write a custom InstanceCreator for its declaring type: " + // Include the message since it might contain more detailed information + + exception.getMessage(); } } } diff --git a/gson/src/test/java/com/google/gson/functional/ReflectionAccessTest.java b/gson/src/test/java/com/google/gson/functional/ReflectionAccessTest.java index 616d101323..58493c9e86 100644 --- a/gson/src/test/java/com/google/gson/functional/ReflectionAccessTest.java +++ b/gson/src/test/java/com/google/gson/functional/ReflectionAccessTest.java @@ -1,6 +1,7 @@ package com.google.gson.functional; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import java.util.Collections; @@ -34,11 +35,10 @@ public void testSerializeInternalImplementationObject() { gson.fromJson("{}", internalClass); fail("Missing exception; test has to be run with `--illegal-access=deny`"); } catch (JsonIOException expected) { - assertEquals( + assertTrue(expected.getMessage().startsWith( "Failed making constructor 'java.util.Collections$EmptyList#EmptyList()' accessible; " - + "either change its visibility or write a custom InstanceCreator for its declaring type", - expected.getMessage() - ); + + "either change its visibility or write a custom InstanceCreator for its declaring type" + )); } } } From 6819419f8cc97e461a74cefc29baa6d50d7d242f Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Fri, 4 Jun 2021 02:31:14 +0200 Subject: [PATCH 4/6] Remove Maven toolchain requirement --- gson/pom.xml | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/gson/pom.xml b/gson/pom.xml index b5b17a4a64..848cd4e552 100644 --- a/gson/pom.xml +++ b/gson/pom.xml @@ -25,11 +25,13 @@ maven-surefire-plugin 3.0.0-M5 - - - 11 - + --illegal-access=deny From 2dfccd21fe9bb3a264e62d39691f9f23261adbdf Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Mon, 8 Nov 2021 18:59:54 +0100 Subject: [PATCH 5/6] Address review feedback --- .../gson/internal/ConstructorConstructor.java | 3 +-- .../internal/reflect/ReflectionHelper.java | 19 +++++++------------ .../gson/functional/ReflectionAccessTest.java | 10 ++++------ 3 files changed, 12 insertions(+), 20 deletions(-) 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 d5590c6e46..9ef0d39a1a 100644 --- a/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java +++ b/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java @@ -128,8 +128,7 @@ public T construct() { @SuppressWarnings("unchecked") // T is the same raw type as is requested @Override public T construct() { try { - Object[] args = null; - return (T) constructor.newInstance(args); + return (T) constructor.newInstance(); } catch (InstantiationException e) { // TODO: JsonParseException ? throw new RuntimeException("Failed to invoke " + constructor + " with no args", e); 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 index fb8305dd62..a74de3025b 100644 --- a/gson/src/main/java/com/google/gson/internal/reflect/ReflectionHelper.java +++ b/gson/src/main/java/com/google/gson/internal/reflect/ReflectionHelper.java @@ -1,10 +1,9 @@ package com.google.gson.internal.reflect; +import com.google.gson.JsonIOException; import java.lang.reflect.Constructor; import java.lang.reflect.Field; -import com.google.gson.JsonIOException; - public class ReflectionHelper { private ReflectionHelper() { } @@ -12,10 +11,8 @@ 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 + * @param field field to make accessible + * @throws JsonIOException if making the field accessible fails */ public static void makeAccessible(Field field) throws JsonIOException { try { @@ -44,17 +41,15 @@ private static String constructorToString(Constructor constructor) { stringBuilder.append(parameters[i].getSimpleName()); } - return stringBuilder.append(")").toString(); + 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 + * @param constructor constructor to make accessible + * @return exception message; {@code null} if successful, non-{@code null} if * unsuccessful */ public static String tryMakeAccessible(Constructor constructor) { @@ -63,7 +58,7 @@ public static String tryMakeAccessible(Constructor constructor) { return null; } catch (Exception exception) { return "Failed making constructor '" + constructorToString(constructor) + "' accessible; " - + "either change its visibility or write a custom InstanceCreator for its declaring type: " + + "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(); } diff --git a/gson/src/test/java/com/google/gson/functional/ReflectionAccessTest.java b/gson/src/test/java/com/google/gson/functional/ReflectionAccessTest.java index 58493c9e86..2e225ce489 100644 --- a/gson/src/test/java/com/google/gson/functional/ReflectionAccessTest.java +++ b/gson/src/test/java/com/google/gson/functional/ReflectionAccessTest.java @@ -4,12 +4,10 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; -import java.util.Collections; - -import org.junit.Test; - import com.google.gson.Gson; import com.google.gson.JsonIOException; +import java.util.Collections; +import org.junit.Test; public class ReflectionAccessTest { /** @@ -36,8 +34,8 @@ public void testSerializeInternalImplementationObject() { fail("Missing exception; test has to be run with `--illegal-access=deny`"); } catch (JsonIOException expected) { assertTrue(expected.getMessage().startsWith( - "Failed making constructor 'java.util.Collections$EmptyList#EmptyList()' accessible; " - + "either change its visibility or write a custom InstanceCreator for its declaring type" + "Failed making constructor 'java.util.Collections$EmptyList#EmptyList()' accessible; " + + "either change its visibility or write a custom InstanceCreator or TypeAdapter for its declaring type" )); } } From 9140460bd5e577427369d47b70e41411a8f1cb47 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Tue, 9 Nov 2021 01:30:47 +0100 Subject: [PATCH 6/6] Add back test for Security Manager --- .../gson/internal/bind/TypeAdapters.java | 34 +++++--- .../com/google/gson/functional/EnumTest.java | 30 ++++--- .../gson/functional/ReflectionAccessTest.java | 81 +++++++++++++++++++ 3 files changed, 117 insertions(+), 28 deletions(-) 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 1b6b0118b6..81870bc9c4 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; @@ -759,22 +760,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; - } - AccessController.doPrivileged(new PrivilegedAction() { - @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() { + @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; + } + }); + 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/test/java/com/google/gson/functional/EnumTest.java b/gson/src/test/java/com/google/gson/functional/EnumTest.java index 66b855ebfc..8a1c6e12c6 100644 --- a/gson/src/test/java/com/google/gson/functional/EnumTest.java +++ b/gson/src/test/java/com/google/gson/functional/EnumTest.java @@ -16,12 +16,6 @@ package com.google.gson.functional; -import java.lang.reflect.Type; -import java.util.ArrayList; -import java.util.Collection; -import java.util.EnumSet; -import java.util.Set; - import com.google.gson.Gson; import com.google.gson.GsonBuilder; import com.google.gson.JsonDeserializationContext; @@ -34,7 +28,11 @@ import com.google.gson.annotations.SerializedName; import com.google.gson.common.MoreAsserts; import com.google.gson.reflect.TypeToken; - +import java.lang.reflect.Type; +import java.util.ArrayList; +import java.util.Collection; +import java.util.EnumSet; +import java.util.Set; import junit.framework.TestCase; /** * Functional tests for Java 5.0 enums. @@ -200,17 +198,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 index 2e225ce489..ece351240a 100644 --- a/gson/src/test/java/com/google/gson/functional/ReflectionAccessTest.java +++ b/gson/src/test/java/com/google/gson/functional/ReflectionAccessTest.java @@ -1,15 +1,96 @@ 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.