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

Allow forbidden APIs for JDK 8-only signatures for a JDK 7+ project #63

Closed
dakrone opened this issue Aug 14, 2015 · 7 comments
Closed
Assignees

Comments

@dakrone
Copy link

dakrone commented Aug 14, 2015

If I try to add:

java.nio.files.Files#readAllLines(java.nio.files.Path) @ etc etc etc

It breaks when run under Java 7 because this is a Java-8 only method

dakrone referenced this issue in elastic/elasticsearch Aug 14, 2015
@uschindler
Copy link
Member

Hi Lee,

first: it makes no sense to add this method to a Java 7 project, because it would not even compile with Java 7, so it would not generate the bytecode (Javac fails) and the bytecode cannot be analyzed with forbidden-apis because of that. So it's the same like adding a signature with @OverRide to a Java 7 project that is not available in Java 7 (but works with Java 8). The compiler complains because it does not override anything. So sorry, this is a non-issue. If you just want to add it to your signatures file for reference when you later upgrade to Java 8, use comment syntax. forbidden-apis internally has a separate signatures file for every supported java version.

Anyways, if you really want to add signatures that are not existent, you can do this: http://jenkins.thetaphi.de/job/Forbidden-APIs/javadoc/check-mojo.html#failOnUnresolvableSignatures

But you should not do this for reasons above, it also floods you with warnings in the maven log. To keep track of those methods, just add them as comments.

Now the technical details: We cannot forbid non-existent methods, because the forbidden-apis compiler needs the full method signature (including modifiers, returns type,...) to detect it in byte code. So it really needs to read the signature. We only have one "relaxed" option: wildcards on class names (you can add signature 'java.io.File*').

Lastly (unrelated), the above mentioned method is not "unsafe", so no need to forbid it. It does not use default charset, its defined to use UTF-8 (see javadocs): "Read all lines from a file. Bytes from the file are decoded into characters using the UTF-8 charset." Because of that its not listed in forbidden-apis "official" unsafe signatures (where we would list it otherwise...)
In Java 7/8 there are many other methods without charsets in classes around the new Path API, but none of them are forbidden, because happily Oracle/OpenJDK decided to NOT use default charset. They are all defined to use UTF-8!

Finally: If you want to forbid all methods that are not part of the Java version used to compile, use the animal-sniffer maven plugin. This is similar to forbidden-apis, but inverse. It only allows to call methods from the defined JDK (1.7 in your case) and fails on all methods not in the official Java JDK signature: http://www.mojohaus.org/animal-sniffer/

@uschindler
Copy link
Member

This is no issue, you can already allow unresolveable sigantures: #14

See http://jenkins.thetaphi.de/job/Forbidden-APIs/javadoc/check-mojo.html#failOnUnresolvableSignatures for corresponding maven opts, same for ANT: http://jenkins.thetaphi.de/job/Forbidden-APIs/javadoc/ant-task.html

@dakrone
Copy link
Author

dakrone commented Aug 14, 2015

first: it makes no sense to add this method to a Java 7 project,
because it would not even compile with Java 7, so it would not
generate the bytecode (Javac fails) and the bytecode cannot be
analyzed with forbidden-apis because of that. So it's the same like
adding a signature with @OverRide to a Java 7 project that is not
available in Java 7 (but works with Java 8). The compiler complains
because it does not override anything. So sorry, this is a non-issue.
If you just want to add it to your signatures file for reference when
you later upgrade to Java 8, use comment syntax. forbidden-apis
internally has a separate signatures file for every supported java
version.

Ahh okay, nice to know there are separate signatures. I do still think
it'd be nice to have support for it for projects that don't have strict
requirements on JDK versions for compilation.

Now the technical details: We cannot forbid non-existent methods,
because the forbidden-apis compiler needs the full method signature
(including modifiers, returns type,...) to detect it in byte code. So
it really needs to read the signature. We only have one "relaxed"
option: wildcards on class names (you can add signature
'java.io.File*').

Can you expound on the technical reasoning? I don't know how
forbidden-apis transforms the string representation for a forbidden API
into an Object, so I don't know why it requires the method signature
actually exist (instead of transforming the byte-code representation to
a textual format, which would work regardless of version/existence).

Lastly (unrelated), the above mentioned method is not "unsafe", so no
need to forbid it.

I understand; the above is just an example of a method, reading the
documentation I see that it uses UTF-8, however, just because it uses
UTF-8 does not mean you can assume it's safe! What if a project wanted
to enforce a different Charset was used everywhere (and thus should
always be specified)?

@uschindler
Copy link
Member

Can you expound on the technical reasoning? I don't know how
forbidden-apis transforms the string representation for a forbidden API
into an Object, so I don't know why it requires the method signature
actually exist (instead of transforming the byte-code representation to
a textual format, which would work regardless of version/existence).

The signatures file format does not contain the return type, so it has to be determined. Actually, one signature expands to several signatures internally (sonmetimes), because some methods in the JDK have multiple covariant return types (e.g. a clone() override with different return type is automatically added by Java compiler also with Object return type)

Actually, you can already ignore non-existent signatures, but it MUST print a warning! The problem with static analysis tools is: If we would not check the signatures for validity from the beginning, a simple typo in the signatures file would cause no error, but also detect no violation. So better to be strict and require the signature to be correct from the beginning. We just added the possibility to not fails the build on unresolveable signatures because of issues like this (see issue history). But the warning is mandatory...!!!!!!!!!!!

Now some technical details: By resolving the signature from the beginning to a bytecode type descriptor (MethodHandle/MethodType in Java 7+: http://docs.oracle.com/javase/7/docs/api/java/lang/invoke/MethodType.html#fromMethodDescriptorString(java.lang.String,%20java.lang.ClassLoader)), the lookup at runtime is much easier. Its just a string lookup in a map (the type descriptor as key). For that to work we have to get the type descriptor initially. Otherwise forbiddenapis would need to do much more stuff on every invoke instruction in bytecode to determine if its forbidden. Think of superclasses, interfaces,... This is just a design decision, changing this would need a rewrite of whole detection logic - for no reason :-)

@uschindler
Copy link
Member

I understand; the above is just an example of a method, reading the
documentation I see that it uses UTF-8, however, just because it uses
UTF-8 does not mean you can assume it's safe! What if a project wanted
to enforce a different Charset was used everywhere (and thus should
always be specified)?

That's your turn to decide, but it won't get included in official method signatures of forbiddenapis.

@dakrone
Copy link
Author

dakrone commented Aug 14, 2015

Okay, makes sense, thanks for the technical explanation!

@uschindler
Copy link
Member

@dakrone No problem!

If you really want to add the signatures back that were reoved from elastic/elasticsearch@178dc7e, just use the above mentioned Maven config (failOnUnresolvableSignatures=false). This is why I closed this as "duplicate".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants