Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix failing to serialize Collection or Map with inaccessible constructor #1902

Conversation

Marcono1234
Copy link
Collaborator

Fixes #1875

Note that this introduces an incompatibility:
Projects which previously relied on UnsafeReflectionAccessor making internal members accessible will now encounter warnings or exceptions when the Java Platform Module System does not allow access. These projects should write their own TypeAdapters, or temporarily use java --add-opens ....
This "resolves" #1540, in the sense that that issue is not a problem with Gson, and that this pull request further strengthens this standpoint.
(In theory this change could be omitted from this pull request while still solving #1875, but it would render the added unit test ineffective.)

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.
Also remove tests which rely on Java implementation details.
@google-cla google-cla bot added the cla: yes label Jun 4, 2021
This also avoids a confusing stack trace, since the previously caught
exception might have had a complete unrelated stack trace.
// 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);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JsonIOException thrown here and in ReflectionHelper is not fitting very well, but Gson has currently no better fitting exception type.
(maybe the whole Gson exception hierarchy should be refactored in the future)

@Marcono1234
Copy link
Collaborator Author

@eamonnmcmanus could you please have a look at this pull request? The fixed issue (#1875) causes problems for some legit usages of certain JDK types.

If you want I can also split off the removal of UnsafeReflectionAccessor; though ultimately it should be removed either way because it circumvents JDK module system restrictions and is ineffective in the latest JDK versions anyway (see pull request description).

import com.google.gson.annotations.SerializedName;

@SuppressWarnings("serial")
public final class ThrowableFunctionalTest extends TestCase {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has been removed because it relies on JDK internals.

@@ -53,21 +53,6 @@ public void testRecursiveResolveSimple() {
assertNotNull(adapter);
}

/**
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have been removed because they rely on JDK internals.

Copy link
Member

@eamonnmcmanus eamonnmcmanus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very helpful, thanks! I have mostly nits. Also a merge is needed.

// 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();
Copy link
Collaborator Author

@Marcono1234 Marcono1234 Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have changed this to also run getDeclaredFields() in privileged context because it can throw a SecurityException as well.

Not directly related to the other changes of this pull request, but I was having a look at other cases where a SecurityManager could be an issue.
Sorry for this delayed change after the pull request approval. If you want I can also put that in a separate pull request.

@Marcono1234 Marcono1234 force-pushed the marcono1234/inaccessible-object-exception branch from eafcffc to 9140460 Compare November 9, 2021 00:43
@Marcono1234
Copy link
Collaborator Author

Sorry for these changes after your approval; I tried adding back the Maven toolchain usage, but unfortunately even with version range it apparently requires to explicitly configure toolchains. Therefore I have removed that commit again and force-pushed; you can see the changes by clicking on the "force-pushed" text here:
force-pushed text screenshot

Also just to be sure there is no misunderstanding; these changes might cause backward compability issues for Gson users running < JDK 12 who rely on Gson's Unsafe usage for making fields and constructors accessible to which they should not have any access, because this pull request removes that code.
However, on the other hand these versions < JDK 12 will by default permit illegal access for the module system (was changed in Java 16 to deny by default, see JDK-8255363), so unless the users explicitly changed this behavior, the worse thing they will experience is warnings about illegal access on the console.

@eamonnmcmanus
Copy link
Member

This looks fine now.

@eamonnmcmanus eamonnmcmanus merged commit b0595c5 into google:master Nov 9, 2021
@Marcono1234 Marcono1234 deleted the marcono1234/inaccessible-object-exception branch November 10, 2021 12:41
tibor-universe pushed a commit to getuniverse/gson that referenced this pull request Nov 21, 2021
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

InaccessibleObjectException with JDK 16
2 participants