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

Linkage Checker to verify method references to JRE classes #1607

Merged
merged 10 commits into from
Aug 26, 2020
Merged

Linkage Checker to verify method references to JRE classes #1607

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ab53f2e
Validate system class in target
suztomo Aug 19, 2020
a8a1571
GraalVM references in exclusion rule
suztomo Aug 19, 2020
a36b452
Applied review
suztomo Aug 19, 2020
f0f81dc
Removing irrelevant comment
suztomo Aug 19, 2020
8199078
Merge remote-tracking branch 'origin/master' into protobuf-3.12.4
suztomo Aug 19, 2020
8a9dca3
Merge branch 'master' into protobuf-3.12.4
suztomo Aug 24, 2020
baeaa93
Merge branch 'master' into protobuf-3.12.4
suztomo Aug 24, 2020
82a0837
Merge branch 'master' into protobuf-3.12.4
suztomo Aug 24, 2020
b494064
Merge branch 'master' into protobuf-3.12.4
suztomo Aug 24, 2020
12c0fe1
Merge branch 'master' into protobuf-3.12.4
suztomo Aug 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions dependencies/src/main/java/com/google/cloud/tools/opensource/classpath/ClassDumper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,7 @@ JavaClass loadJavaClass(String className) throws ClassNotFoundException {
/** Returns true if {@code className} is available in the system class loader. */
boolean isSystemClass(String className) {
try {
if (className.startsWith("[")) {
// Array class
if (isArrayClass(className)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

will we catch a reference to an array of a missing class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but not from array class reference. When there's a reference to array of a class, there should be a reference to the class in the same class file.

return true;
}
extensionClassLoader.loadClass(className);
Expand All @@ -137,6 +136,11 @@ boolean isSystemClass(String className) {
}
}

/** Returns true if {@code className} is a binary name for array types. */
static boolean isArrayClass(String className) {
return className.startsWith("[");
}

/**
* Returns a map from classes to the symbol references they contain.
*/
Expand Down
7 changes: 5 additions & 2 deletions dependencies/src/main/java/com/google/cloud/tools/opensource/classpath/LinkageChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,8 +239,11 @@ Optional<LinkageProblem> findLinkageProblem(
String targetClassName = symbol.getClassBinaryName();
String methodName = symbol.getName();

// Skip references to Java runtime class. For example, java.lang.String.
if (classDumper.isSystemClass(targetClassName)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was hiding the protobuf's linkage error against Java 8 runtime protocolbuffers/protobuf#7827

if (ClassDumper.isArrayClass(targetClassName)) {
// Skip references to array class. However, we want to check other JDK-provided classes,
Copy link
Contributor

Choose a reason for hiding this comment

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

delete this comment. It describes what the code is not doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

// because it is possible for wrongly configured Java compiler to generate bad byte code that
// references methods that does not exist in Java 8. For example case, see
// https://github.com/protocolbuffers/protobuf/issues/7827.
return Optional.empty();
}

Expand Down
57 changes: 56 additions & 1 deletion dependencies/src/main/resources/linkage-checker-exclusion-default.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,62 @@
<Package name="org.graalvm" />
</Source>
</LinkageError>

<LinkageError>
<Target>
<Class name="java.lang.invoke.MethodHandle" />
</Target>
<Source>
<Package name="com.oracle.svm" />
</Source>
<Reason>
GraalVM-related libraries depend on Java Compiler Interface (JVMCI) that
only exists in special JDK. These missing classes are false positives, because
the code is only invoked when running in a GraalVM.
https://github.com/GoogleCloudPlatform/cloud-opensource-java/issues/929
</Reason>
</LinkageError>
<LinkageError>
<Target>
<Class name="java.lang.invoke.MethodHandle" />
</Target>
<Source>
<Package name="com.oracle.graal" />
</Source>
<Reason>
GraalVM-related libraries depend on Java Compiler Interface (JVMCI) that
only exists in special JDK. These missing classes are false positives, because
the code is only invoked when running in a GraalVM.
https://github.com/GoogleCloudPlatform/cloud-opensource-java/issues/929
</Reason>
</LinkageError>
<LinkageError>
<Target>
<Class name="java.lang.invoke.MethodHandle" />
</Target>
<Source>
<Package name="org.graalvm" />
</Source>
<Reason>
GraalVM-related libraries depend on Java Compiler Interface (JVMCI) that
only exists in special JDK. These missing classes are false positives, because
the code is only invoked when running in a GraalVM.
https://github.com/GoogleCloudPlatform/cloud-opensource-java/issues/929
</Reason>
</LinkageError>
<LinkageError>
<Target>
<Class name="java.lang.invoke.MethodHandle" />
</Target>
<Source>
<Package name="com.oracle.truffle" />
</Source>
<Reason>
GraalVM-related libraries depend on Java Compiler Interface (JVMCI) that
only exists in special JDK. These missing classes are false positives, because
the code is only invoked when running in a GraalVM.
https://github.com/GoogleCloudPlatform/cloud-opensource-java/issues/929
</Reason>
</LinkageError>
Comment on lines +44 to +99
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because now we check JDK classes, GraalVM-related exclusion is needed.

<LinkageError>
<Target>
<Class name="org.mockito.internal.creation.bytebuddy.MockMethodDispatcher" />
Expand Down
20 changes: 20 additions & 0 deletions ...dencies/src/test/java/com/google/cloud/tools/opensource/classpath/LinkageCheckerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1120,4 +1120,24 @@ public void testFindLinkageProblems_grpcAndGuava() throws IOException {
"has class binary name"))
.contains("io.grpc.internal.DnsNameResolver");
}

@Test
public void testFindLinkageProblems_referenceToJava11Method() throws IOException {
// protobuf-java 3.12.4 references a Java 11 method that does not exist in Java 8
// https://github.com/protocolbuffers/protobuf/issues/7827
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is Kristen's finding yesterday.

ImmutableList<ClassPathEntry> jars =
TestHelper.resolve("com.google.protobuf:protobuf-java:3.12.4");

LinkageChecker linkageChecker = LinkageChecker.create(jars);
ImmutableSet<LinkageProblem> linkageProblems = linkageChecker.findLinkageProblems();

MethodSymbol methodSymbol =
new MethodSymbol("java.nio.CharBuffer", "flip", "()Ljava/nio/CharBuffer;", false);

SymbolNotFoundProblem expectedProblem =
new SymbolNotFoundProblem(
new ClassFile(jars.get(0), "com.google.protobuf.TextFormat"), null, methodSymbol);

Truth.assertThat(linkageProblems).contains(expectedProblem);
}
}