Closed Marcono1234 closed 3 months ago
It's going to be difficult to be sure about workarounds for a missing AccessController
until we have a JDK that has a missing AccessController
.
I'm a bit puzzled about the code in question. Aren't enum constants public? So then we should be able just to use [Class.getFields()
](https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Class.html#getFields()) to read them? Technically that can still throw SecurityException
:
If a security manager, s, is present and the caller's class loader is not the same as or an ancestor of the class loader for the current class and invocation of
s.checkPackageAccess()
denies access to the package of this class.
I am inclined to think that that is going to affect hardly anybody. I'm also not sure we need to be able to call Field.get
, since the name of the constant is the name of the field.
It's going to be difficult to be sure about workarounds for a missing
AccessController
until we have a JDK that has a missingAccessController
.
I understood JEP 411 as if the idea was to remove SecurityManager
and related classes without adding any replacement. But you are right, it is not clear what exactly will happen; it seems there is no follow-up JEP yet.
I'm a bit puzzled about the code in question.
Coincidentally you were the one who suggested using AccessController#doPrivileged
in https://github.com/google/gson/pull/1495#discussion_r682178814 😅, but for the setAccessible
call.
I then later expanded it to only have one AccessController#doPrivileged
call for all fields instead of one per field.
I'm also not sure we need to be able to call
Field.get
, since the name of the constant is the name of the field.
Unfortunately not when ProGuard or R8 are involved. If I remember correctly, name()
still returns the expected value, but the field names might be obfuscated. And the enum values()
method to obtain all constants (which is used by Class.getEnumConstants()
) might have been removed by the code shrinker (at least #1147 mentions that).
Another option would also be to simply remove the AccessController#doPrivileged
call. There are only a few issues and pull requests related to SecurityManager
(#566, #1361, #1712), and maybe nowadays most users don't use a SecurityManager
.
But maybe we can wait with that until it becomes clearer which approach the JDK maintainers will take regarding removal of SecurityManager
.
I didn't express my point about the JDK very well. What I meant was that if we add a workaround that is supposed to function when there is no AccessController
class, we can't really test it until we have a JDK that really does have no AccessController
class.
Thanks for reminding me of the previous discussion, where indeed I noted that enum constants are public but enums themselves aren't necessarily. Still, the whole thing feels very complicated and I'm not sure it is worthwhile now. I would be somewhat inclined to rip it all out and see if anybody complains. If they do, we can put it back again in another release and perhaps also look at handling a missing AccessController
.
It seems usage of AccessController
actually makes a difference. Without it the following test testJdkEnum_SecurityManager
fails with AccessControlException: access denied ("java.lang.RuntimePermission" "accessDeclaredMembers")
.
(Though I am not completely sure if this is a proper SecurityManager
/ Policy
test setup.)
@Test
public void testJdkEnum() {
assertThat(gson.toJson(Thread.State.NEW)).isEqualTo("\"NEW\"");
assertThat(gson.fromJson("\"NEW\"", Thread.State.class)).isEqualTo(Thread.State.NEW);
}
@SuppressWarnings("removal") // java.lang.SecurityManager deprecation in Java 17
@Test
public void testJdkEnum_SecurityManager() {
// Skip for newer Java versions where `System.setSecurityManager` is unsupported
assumeTrue(Runtime.version().feature() <= 17);
var gsonProtectionDomain = Gson.class.getProtectionDomain();
var testProtectionDomain = getClass().getProtectionDomain();
assertWithMessage(
"Gson code and test code should have different ProtectionDomain; otherwise test might"
+ " not work properly")
.that(gsonProtectionDomain)
.isNotEqualTo(testProtectionDomain);
Policy originalPolicy = Policy.getPolicy();
SecurityManager originalSecurityManager = System.getSecurityManager();
try {
Policy.setPolicy(
new Policy() {
@Override
public boolean implies(ProtectionDomain domain, Permission permission) {
// Allow removing the SecurityManager again
if (permission instanceof RuntimePermission
&& permission.getName().equals("setSecurityManager")) {
return true;
}
if (domain.equals(gsonProtectionDomain)) {
return true;
}
return originalPolicy.implies(domain, permission);
}
});
System.setSecurityManager(new SecurityManager());
assertThat(gson.toJson(Thread.State.NEW)).isEqualTo("\"NEW\"");
assertThat(gson.fromJson("\"NEW\"", Thread.State.class)).isEqualTo(Thread.State.NEW);
} finally {
System.setSecurityManager(originalSecurityManager);
Policy.setPolicy(originalPolicy);
}
}
However, it seems there are a few other places which fail when a custom SecurityManager
is used:
com.google.gson.reflect.TypeToken.isCapturingTypeVariablesForbidden()
Due to it's usage of System#getProperty
Collection
and Map
adaptercom.google.gson.internal.$Gson$Types.equals(Type, Type)
This calls TypeVariable.getGenericDeclaration()
and apparently the implementation sun.reflect.generics.reflectiveObjects.TypeVariableImpl
performs undocumented access checks. (Or my test setup is wrong?)So unless my test setup is wrong or flawed this suggests that usage with a SecurityManager
was already cumbersome, or was not working in some cases. So maybe removing the AccessController
usage might be fine since most users don't rely on it?
Yes, the more I think about it, the more I agree with my earlier comment about just ripping out the AccessController
usage. :-)
It may be that there are people who are depending on it and who are keeping up to date with Gson versions but not JDK versions. If so we can learn about their use cases when they complain after the next release, and then we can decide what to do. And if there is in fact nobody depending on the AccessController
then the code will be that much simpler without it.
Problem solved by the feature
JEP 411 marked the
SecurityManager
and related classes such asjava.security.AccessController
as 'deprecated for removal' since JDK 17.Currently the Gson code uses
AccessController
: https://github.com/google/gson/blob/91d22b33e267b0fd699bdc3e830b7dc5561a88e4/gson/src/main/java/com/google/gson/internal/bind/TypeAdapters.java#L969This could become a problem when a future JDK version actually removes the
SecurityManager
and related classes, and Gson users cannot upgrade then.Side note: Gson's unit test
ReflectionAccessTest.testRestrictiveSecurityManager()
also uses the deprecatedSecurityManager
class, but maybe we should keep this for now to make sure Gson's behavior for JDK versions withSecurityManager
is correct. Once we want to build with JDK versions withoutSecurityManager
we can investigate solutions for this, e.g. moving the test into a separate test class which is excluded when building with newer JDK versions.Feature description
We should already consider solutions for the case that
AccessController
does not exist, to avoid any issues if users want to upgrade to future JDK versions.One solution might be to wrap the usage of
AccessController
inside a try-catch which catchesNoClassDefFoundError
and in that case falls back to executing the code to access the fields outside the scope ofAccessController#doPrivileged
. (To be verified that this actually works as desired.)Another alternative might be to refactor the code to first check with
Class#forName
or similar ifAccessController
exists (and remember the result).It might also be good to add a unit test which verifies that the
AccessController#doPrivileged
usage actually has the desired effect. It seems that is currently not covered by any test (?). Then we could be more confident that whatever change we make does not break the code for users who have a restrictiveSecurityManager
on an older JDK.