Update forbiddenapis to v2.6

[uschindler]

Changelog: Version 2.6 (released 2018-09-17):

New features:

• Add Java 11 support by upgrading to ASM 6.2.1. This includes updated signatures files
  and build system changes (shipped documentation is currently missing reference to
  Java 11, sorry).
• The plugin was published on the Gradle Plugin portal.
  Due to a bug in plugin initialization, older versions may not fully work.
  Thanks to Sebastian Davids and the Gradle team for help and pushing this forward!
• When signatures are using classes that are not found on classpath, the option to ignore
  those warnings is no longer so noisy: It only lists all failed signatures separately
  where methods/fields do not exit, but the missing classes are reported only with a
  single line, thanks ghost,
  Tim Allison, Dawid Weiss.
• Report more details about class that failed to load while scanning the classes
  to be checked, thanks Trejkaz.
• Only print warning about Gradle daemon once.

Bug fixes:

• Gradle: Fix SourceSets added after plugin loading don't get tasks.
• Add a hack for the broken JavaVersion enum in Gradle. It returns version "1.9" and "1.10"
  (which are invalid targetCompatibility versions) and this problem is only fixed after Java 11.
  Before Java 11 we switch to plain "majorVersion" property, thanks Sebastian Davids.
• Gradle: Always apply JavaBasePlugin, otherwise startup may fail with new plugin DSL,
  as initialization order is undefined, thanks Sebastian
  Davids.

Internals:

• Refactor signatures file parsing and lookup of signatures (remove from Checker).
• Workaround for Java 1.6 (minimum version) builds with Maven Central TLS settings.
• Cleanup Gradle plugin initialization exception handling, check minimal Gradle version.

Important: Normally I would supply a PR, but due to the recent changes in Elasticsearch (use forbiddenapis CLI instead of plugin - why the hell???), I can't support it anymore (the CLI is also not actively maintained and stays for "simple use cases", it was never made for automated running from build scripts). Therefore, please do the update on your own. Just a few things:

• Forbiddenapis can handle target "11" (no version cap needed, or at least raise cap to 11)
• If you update (you should!) release branches that use the real Gradle plugin, please remove the commenting about the server/java9 sourceset. The release fixed the bug in the Gradle plugin (later added sourcesets are not initialized with forbiddenApisJava9 task). Due to many updates in the Gradle plugin, an update of release/maintenance branches should really be investigated (especially to check Java 9 code and compiled Groovy code).

Uwe

[elasticmachine]

Pinging @elastic/es-core-infra

[alpar-t]

@uschindler thanks for letting us know! We are running with the CLI so that we can do it with a different JVM than the one Gradle is running with to be able to better handle multi release jars, and was planning to open an issue about and discuss to see if we can incorporate this into the plugin, but it did not seem straight forward. We want to move Gradle to run on Java 10 but still run forbiddenapi checks with Java 8 without calling this out specifically in CI.

[uschindler]

Hi Alpar,
I have seen the issue, but still the idea to run forbiddenapis is unclear and not needed. The arguments are all not really explain it:

• To better release Multi-Release JARs: Fobiddenapis in the current version perfectly supports multirelease JARs, as it can scan newer code also with older VMs and vice versa (the class file format has nothing to do with the running VM, as forbiddenapis parses it on its own). If you have a multi-release JAR, you hjave 2 sourcesets (one for base java and one for java 9). Since the last update, forbiddenapis automatically runs on both (there was a bug that sourcesets added after JavaPlugin load were not evaluated, see Gradle: SourceSets added after plugin loading don't get tasks policeman-tools/forbidden-apis#138 - the same you had in your own plugin: sourcesets.all{} should be used instead of sourcesets.foreach{}). The base one works with targetVersion=8, the java9 one with targetVersion=9 - plain simple
• The problem of deprecated APIs completely removed in later Java versions (Thread.stop): You should not add those into your own signatures files, as there is already a jdk-deprecated. E.g. you add jdk-deprecated(targetVersion8) to your base sourceset (that's also compiled against java 8). If you run this in Java 8, forbiddenapis will complain about the deprecated API and fail build (no extra signature needed in your own sigantures, it's obsolete). If you run this in Java 10, the compile will fail anyways. But even if it it would pass, forbiddenapis has a special marker in jdk-deprecated (as its machine-generated signature file) and will not complain about missing signatures.

To correctly use forbiddenapis in any JVM, you just need to:

• set the target version correctly for the source set both on compiler (forked or not, doesn't matter) and on forbiddenapis. In your case, set target to 8 for main sourceset; for the MR JAR use Java 9 or 10 or 11. For build reprocuibility, explicitey setting the targetVersion is important, as this makes it independent from runtime. Forbiddenapis will parse the signatures and class files according to that version. It is by the way planned to use Java 9+'s feature to compile run against older runtime version (javac -release) in the same way like javac (it resolves signatures against this older version)
• run forbiddenapis for every sourceset (Java, Groovy, Kotlin,...). The updated version fixes the bug with lazy evaluation of sourcesets.

BTW, neither Checkstyle nor PMD are able to fork (for the same reason), they use the Ant task internally to execute.

Unless there are any other reasons (please explain details), I see no reason to add an option to the upstream forbiddenapis task to support forking. The only thing would be to use the -release 8 feature of later javac also with forbiddenapis (still under investigation), but that's not needed for jdk-deprecated.

[uschindler]

Maybe bring @rjernst in here...

[uschindler]

Here is the magic in jdk-deprecated for targetVersion 1.8: https://github.com/policeman-tools/forbidden-apis/blob/2.6/src/main/resources/de/thetaphi/forbiddenapis/signatures/jdk-deprecated-1.8.txt#L7

Here you see the "bad" thread methods, as declared "deprecated in Java 8": https://github.com/policeman-tools/forbidden-apis/blob/2.6/src/main/resources/de/thetaphi/forbiddenapis/signatures/jdk-deprecated-1.8.txt#L176-L180

On Java 11, this still allows forbiddenapis to work correctly, as those files are not hand-maintained, they were generated by machine, so ignoring signatures not available at runtime is safe, as a typos / human errors cannot happen.

[alpar-t]

Multi release jars were a problem with 3rd party multi release jars for sure in the third party audit for sure, as different classes would be loaded and scanned depending on the running jvm, and would thus produce different results. We should have produced and example with @rjernst when we discussed this instead of relying on memory. We might also have been confused that during the third party audit task some classes meant for java 9 were being scanned when running on 8 as well due to how the jars were being extracted.

[uschindler]

The third party audit was indeed a problem in the previous setup: It just extracted all JAR files and therefor it also extracted the META-INF/versions folder. So Thirdparty-Audit should have applied an exclusion rule on META-INF/**

Uwe

[rjernst]

Thanks @uschindler. This all makes more sense now.

@atorok Let's switch back to using the forbiddenapis plugin and change third party audit to exclude META-INF as suggested.

[uschindler]

Hi @rjernst ,

I think the main forbiddenapis scan (of Elasticsearch Source code) is perfectly fine with the official plugin, there are no changes needed. I'd only remove the duplicate signatures (jdk-deprecated and elasttic's own have both Thread deprecated method, that's unneeded). Of course with the new plugin version, the Elasticsearch Java 9 sourceSet can now also be scanned (see the code I added, but commented out in my PR of March 2018).

The third party audit has some special cases, which are also not solvable with running forked (the problem stays):

The JAR files are extracted and then the ANT task is started to scan the extracted class files. When doing this, there is no classloader involved, so it's reproducible with different JVMs. The problem is another one: META-INF/versions contains duplicate class files, which are also scanned. Due to same class name, this leads to non-reproducibility, because it depends on scanning order which class file is then looked up internally in forbiddenapis, when other classes are scanned that refer to those duplicated ones. The only way to solve this is to only extract class files from the main folder and exclude META-INF/ folder while extracting the JAR file. Of course this would not scan the java9/10/11 versions in third party audits. But due to the module system, it's very unlikely that a 3rd party JAR uses internal APIs in the Java 9+ versions of class files. If you still want to scan those classes, it needs more work in 3rd party audit (it needs to extract and scan the 3rd party jar multiple times and for each run, extract java9/java10/... class files separately and overwrite the main class files (so replace original files with versioned ones, and then scan again with targetVersion=...). I'd not do this!

[alpar-t]

@uschindler This is what we do for third party audit now:

getProject().copy(spec -> {
                spec.from(getProject().zipTree(jar));
                spec.into(jarExpandDir);
                // Exclude classes for multi release jars above target
                for (int i = Integer.parseInt(targetCompatibility.getMajorVersion()) + 1;
                     i <= Integer.parseInt(JavaVersion.VERSION_HIGHER.getMajorVersion());
                     i++
                ) {
                  spec.exclude("META-INF/versions/" + i + "/**");
                }
            })

So when we are scanning with an older JVM, we don't run into the class files meant for the newer versions and unknown class format or missing class errors.
For example, when we are running on java 8, we will exclude files mean for java 9.
I assumed this was fine as the jars are on the classpath and we only scan the extracted jars to get a list of all the classes, so when loading them we will load the right one. Can you confirm that ?

There are different jobs in CI that set the target compatibility from 8 to 11 so that assures us that we do eventually scan all the classes, even those meant for java 9.

@rjernst I reverted to using the plugin in #33809

[uschindler]

Hi,
it looks fine to just exclude the higher versions. But nevertheless there is the problem that forbiddenapis internally keeps a map of class files keyed on class name for looking up purposes (it's own "classloader"-like mechanism just looks for references between classes that are part of the scanned set by looking up via the classname key). This approach would add the same class multiple times to the map.
The correct way to implement this would to unzip the JAR file multiple times, each overwriting files that are already there). It would start with the base classes. And then it would iterate up to target versions and extracting the META-INFO/versions/MAJOR over the existing tree (without the versions prefix). This wuld overwrite the older versions with newer ones.
The current CLI based approach was also wrong in that regard.