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.

Sign up for GitHub

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

Jump to bottom

False positives when using java.util.Random with JDK 17-ea17 #177

Closed
yrodiere opened this issue Apr 12, 2021 · 12 comments · Fixed by #178
Closed

False positives when using java.util.Random with JDK 17-ea17 #177

yrodiere opened this issue Apr 12, 2021 · 12 comments · Fixed by #178
Assignees
@uschindler
Labels
bug
Milestone
3.2

Comments

@yrodiere
Copy link

Running the forbidden-apis Maven plugin with JDK 17-ea17 leads to false positives.

Apparently, uses of allowed classes are mistaken for uses of their (forbidden) inner classes, leading to

For example, I created a reproducer using java.util.Random, with forbiddenapis configured to forbid the bundled signatures jdk-internal-1.8.

Everything should work fine, but when running with Maven 3.6.3 and JDK 17-ea17, we get an error stating "Forbidden class use: jdk.internal.util.random.RandomSupport$AbstractSpliteratorGenerator", even though that class is never used in the source code.

Full output:

$ mvn clean install
[INFO] Scanning for projects...
[INFO] 
[INFO] --< org.hibernate.playground.forbiddenapis:forbiddenapis-playground >---
[INFO] Building forbiddenapis-playground 1.0-SNAPSHOT
[INFO] --------------------------------[ jar ]---------------------------------
[INFO] 
[INFO] --- maven-clean-plugin:2.5:clean (default-clean) @ forbiddenapis-playground ---
[INFO] Deleting /home/yrodiere/workspaces/testcases/forbiddenapis-playground/target
[INFO] 
[INFO] --- maven-resources-plugin:2.6:resources (default-resources) @ forbiddenapis-playground ---
[WARNING] Using platform encoding (UTF-8 actually) to copy filtered resources, i.e. build is platform dependent!
[INFO] Copying 0 resource
[INFO] 
[INFO] --- maven-compiler-plugin:3.1:compile (default-compile) @ forbiddenapis-playground ---
[INFO] Changes detected - recompiling the module!
[WARNING] File encoding has not been set, using platform encoding UTF-8, i.e. build is platform dependent!
[INFO] Compiling 1 source file to /home/yrodiere/workspaces/testcases/forbiddenapis-playground/target/classes
[INFO] 
[INFO] --- forbiddenapis:2.6:check (verify-forbidden-apis) @ forbiddenapis-playground ---
[INFO] Scanning for classes to check...
[INFO] Reading bundled API signatures: jdk-internal-1.8
[INFO] Loading classes to check...
[INFO] Scanning classes for violations...
[ERROR] Forbidden class use: jdk.internal.util.random.RandomSupport$AbstractSpliteratorGenerator [non-public internal runtime class in Java 1.8]
[ERROR]   in org.hibernate.playground.forbiddenapis.RandomGenerator (RandomGenerator.java, field declaration of 'r')
[ERROR] Scanned 1 class file(s) for forbidden API invocations (in 0.02s), 1 error(s).
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  0.665 s
[INFO] Finished at: 2021-04-12T16:34:33+02:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal de.thetaphi:forbiddenapis:2.6:check (verify-forbidden-apis) on project forbiddenapis-playground: Check for forbidden API calls failed, see log. -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException

For reference, it used to work with JDK 17-ea13:

$ mvn clean install
[INFO] Scanning for projects...
[INFO] 
[INFO] --< org.hibernate.playground.forbiddenapis:forbiddenapis-playground >---
[INFO] Building forbiddenapis-playground 1.0-SNAPSHOT
[INFO] --------------------------------[ jar ]---------------------------------
[INFO] 
[INFO] --- maven-clean-plugin:2.5:clean (default-clean) @ forbiddenapis-playground ---
[INFO] Deleting /home/yrodiere/workspaces/testcases/forbiddenapis-playground/target
[INFO] 
[INFO] --- maven-resources-plugin:2.6:resources (default-resources) @ forbiddenapis-playground ---
[WARNING] Using platform encoding (UTF-8 actually) to copy filtered resources, i.e. build is platform dependent!
[INFO] Copying 0 resource
[INFO] 
[INFO] --- maven-compiler-plugin:3.1:compile (default-compile) @ forbiddenapis-playground ---
[INFO] Changes detected - recompiling the module!
[WARNING] File encoding has not been set, using platform encoding UTF-8, i.e. build is platform dependent!
[INFO] Compiling 1 source file to /home/yrodiere/workspaces/testcases/forbiddenapis-playground/target/classes
[INFO] 
[INFO] --- forbiddenapis:2.6:check (verify-forbidden-apis) @ forbiddenapis-playground ---
[INFO] Scanning for classes to check...
[INFO] Reading bundled API signatures: jdk-internal-1.8
[INFO] Loading classes to check...
[INFO] Scanning classes for violations...
[INFO] Scanned 1 class file(s) for forbidden API invocations (in 0.02s), 0 error(s).
[INFO] 
[INFO] --- maven-resources-plugin:2.6:testResources (default-testResources) @ forbiddenapis-playground ---
[WARNING] Using platform encoding (UTF-8 actually) to copy filtered resources, i.e. build is platform dependent!
[INFO] skip non existing resourceDirectory /home/yrodiere/workspaces/testcases/forbiddenapis-playground/src/test/resources
[INFO] 
[INFO] --- maven-compiler-plugin:3.1:testCompile (default-testCompile) @ forbiddenapis-playground ---
[INFO] No sources to compile
[INFO] 
[INFO] --- maven-surefire-plugin:2.12.4:test (default-test) @ forbiddenapis-playground ---
[INFO] No tests to run.
[INFO] 
[INFO] --- maven-jar-plugin:2.4:jar (default-jar) @ forbiddenapis-playground ---
[INFO] Building jar: /home/yrodiere/workspaces/testcases/forbiddenapis-playground/target/forbiddenapis-playground-1.0-SNAPSHOT.jar
[INFO] 
[INFO] --- maven-install-plugin:2.4:install (default-install) @ forbiddenapis-playground ---
[INFO] Installing /home/yrodiere/workspaces/testcases/forbiddenapis-playground/target/forbiddenapis-playground-1.0-SNAPSHOT.jar to /home/yrodiere/.m2/repository/org/hibernate/playground/forbiddenapis/forbiddenapis-playground/1.0-SNAPSHOT/forbiddenapis-playground-1.0-SNAPSHOT.jar
[INFO] Installing /home/yrodiere/workspaces/testcases/forbiddenapis-playground/pom.xml to /home/yrodiere/.m2/repository/org/hibernate/playground/forbiddenapis/forbiddenapis-playground/1.0-SNAPSHOT/forbiddenapis-playground-1.0-SNAPSHOT.pom
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  0.847 s

I suspected that JDK17 might have introduced some cutting edge magic that inlines constructors somehow, but apparently not: decompiling the class files with CFR yields the original source code.

So I suspect forbiddenapis may be doing something that doesn't work with JDK17-ea17 anymore.
Decompiling the file that triggers the error shows that it's not some cutting edge JDK17 magic
@yrodiere
Copy link
Author

Note: Upgrading to forbiddenapis 3.1 doesn't solve the problem.

@uschindler
Copy link
Member

uschindler commented Apr 12, 2021 •
edited
Loading

Hi,

this is a bug in forbiddenapis, triggered by the following commit in openjdk: openjdk/jdk@a0ec2cb#diff-fdfca19a8fa8e73366160f4afaf99a1ded21423338d18b71662eda1be8c933d4R88

It will affect all code that accesses java.util.Random.

The reason for this is that forbiddenapis goes up the class hierarchy to find all superclasses and superinterfaces. Unfortunately, java.util.Random now extends and invisible internal class (jdk.internal.util.random.RandomSupport.AbstractSpliteratorGenerator) that triggers the problem.

I think I have to add a stop condition to class hierary climbing at the place where it enters JDK-internal code.

@uschindler
Copy link
Member

Sorry there's no workaround at the moment except adding a Suppress directive or downgrading JDK.

The Lucene team haven't seen that up to now, as we are still on build 16.

@yrodiere
Copy link
Author

Hi!

No problem, thanks for the fast answer. I'll disable forbiddenapis when building on JDK 17+ for now; JDK 17 is experimental anyway :)

@uschindler
Copy link
Member

uschindler commented Apr 12, 2021 •
edited
Loading

I will do my best to fix this problem. The class hierarchy climbing was always some problematic part as it may lead to "false positives" (although correct detections), because the user does not expect the problem.

I will also update to latest ASM and add signatures for JDK-16 soon. So a fix is close, because I had no time for a specific JDK-16 release yet, so it's a good time to include that fix.

@uschindler uschindler self-assigned this Apr 12, 2021
@uschindler uschindler added the bug label Apr 12, 2021
@uschindler uschindler added this to the 3.2 milestone Apr 12, 2021
@uschindler
Copy link
Member

uschindler commented Apr 13, 2021 •
edited
Loading

Hi,
I did some investigations yesterday. There are some problems and I am no sure how to solve that:

You are using the static file "jdk-internal-1.8". This file contains an explicit signature jdk.internal.** (package name prefix). As this is a standard signatures file, there is no way to prevent the issue from happening. Because java.util.Random extends a class from this package. We have to check class extensions, because otheriwse we won't catch forbidden API usages of subclasses (e.g. sombody instantiates an implementation of a forbidden interface). The big problem here: forbiddenapis does not know anything about modules and it only sees a public class which is forbidden and is superclass. Javadocs do not list that superclass, because the javadocs tool is "intelligent" and can understand the module system. The approach taken by the OpenJDK developers is similar to good old StringBuilder and StringBuffer in JDK5 - both extend a package-private class, which is invisible to the end user and also not shown in javadocs. The difference here is the we don't extend a package private class, but another class from a different module just privately imported.

I can for sure fix the problem with jdk-non-portable, but the static jdk-internal-XX signatures files are statically and there is no way to add logic here that understands module system.

A quick workaround for you would be to use jdk-internal-9, as Oracle only put the jdk.internal prefix on the blacklist in JDK 8, but OpenJDK removed it later (JDK 9).

So there's a dilemma.

@uschindler
Copy link
Member

uschindler commented Apr 13, 2021 •
edited
Loading

FYI, there's a small problem in your POM file: Since forbiddenapis 3.1, there's a new setting releaseVersion (https://jenkins.thetaphi.de/job/Forbidden-APIs/javadoc/check-mojo.html#releaseVersion), which takes precedence over targetVersion (if given). As you define release version as global property, the plugin setting targetVersion is ignored (and default taken from project property).

To fix the POM file, i'd suggest to set release/target/source version in the properties to 8 and remove the explicit declaration in the plugin config.

@yrodiere
Copy link
Author

yrodiere commented Apr 13, 2021 •
edited
Loading

Thanks for the tip.

Regarding the signature files... I imagine jdk.internal.** classes and other internal classes are considered non-portable and are forbidden by jdk-non-portable anyway? That's what the documentation seems to suggest.

In my actual project (Hibernate Search) I actually use jdk-non-portable too, so maybe I simply don't need jdk-internal-XX at all? If so I could simply remove it.

@uschindler
Copy link
Member

Regarding the signature files... I imagine jdk.internal.** classes and other internal classes are considered non-portable and are forbidden by jdk-non-portable anyway? That's what the documentation seems to suggest.

Yes that's true. Unfortunately, the current problem also happens with jdk-non-portable (for same reasons). I have a fix at the moment, but this is so tricky that I need some time to test is thouroughly. The biggest proble is that not all special cases can be tested (as it lacks of examples in JDK). Mocking is impossible.

In my actual project (Hibernate Search) I actually use jdk-non-portable too, so maybe I simply don't need jdk-internal-XX at all? If so I could simply remove it.

Yes its a duplicate, but won't fix your issue :-)

@uschindler
Copy link
Member

FYI, I also found a problem in JDK 17 class library and opened a bug report. This is not dircetly causing this issue, but I found it by accident when writing test cases: https://bugs.openjdk.java.net/browse/JDK-8265137

@uschindler uschindler changed the title False positives when using JDK 17-ea17 to run the maven plugin False positives when using java.util.Random with JDK 17-ea17 Apr 13, 2021
@uschindler uschindler mentioned this issue Apr 16, 2021
Merged
@uschindler
Copy link
Member

There is now a PR: #178

I need to add a test. It consists of 2 commits: first is to rewrite the whole logic behind the class hierarchy to use a "visitor" pattern. In a second commit just a few checks are added: accce12

Basically: If some code from inside the runtime extends/implements some non-portable class/interface, this is ignored and neither method calls nor field accesses are recognized. This is still a hack, but looks useable.

@uschindler
Copy link
Member

uschindler commented Apr 21, 2021 •
edited
Loading

Hi,
I have not yet pushed the PR, but will do soon, as this special case may happen more often in the future.

In the meantime, the APIs of Random were revised, as the JDK developers figured out that having some hidden base class in the class hierarchy is a bad idea, as it suddenly changes pblic API of Random, ThreadLocalRandom and others.

Therefor they moved the internal implementations to a private class with static utility methods:

Once this is part of JDK, the problem should automatically disappear.

Nevertheless, I'd like to push the changes in the PR, but it is hard to find a good test case (you cannot mock or synthesize one, as it relies on classes be inside the JDK runtime - which may change all the time (like in the fixes coming to OpenJDK 17 soon).

I will think about it a few more days and then decide how to handle that.

@xiaowing xiaowing mentioned this issue Apr 22, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@uschindler uschindler

Labels
bug
Milestone
3.2
Development

Successfully merging a pull request may close this issue.

Fix false positives in JDK-17 with RandomSupport policeman-tools/forbidden-apis
2 participants
@yrodiere @uschindler