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

[MSHARED-1248] maven-dependency-analyzer should log instead of failing #89

Merged
merged 3 commits into from
Jun 25, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
2 changes: 2 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,8 @@
<artifactId>apache-rat-plugin</artifactId>
<configuration>
<excludes combine.children="append">
<!-- Corrupted class files -->
<exclude>**/*.clazz</exclude>
<!-- binary class for unit test -->
<exclude>**/*.classx</exclude>
</excludes>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@ public void visitClass(String className, InputStream in) {
// some bug inside ASM causes an IOB exception. Log it and move on?
// this happens when the class isn't valid.
logger.warn("Unable to process: " + className);
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be helpful to include the exception in the warning here as well

Copy link
Member Author

@garydgregory garydgregory Jun 19, 2023

Choose a reason for hiding this comment

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

@elharo
I updated the call.

} catch (IllegalArgumentException e) {
// [MSHARED-1248] should log instead of failing when analyzing a corrupted jar file
logger.warn("Unable to process: " + className, e);
Copy link
Member

Choose a reason for hiding this comment

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

I'm still not sure if it will be the best solution.
Eg. when analyzer will not support jdk in newer version - user will only have a warnings but nothing will be analyzed.
We know that warnings are many times ignored ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @slawekjaranowski
I'm not sure how else you propose to solve https://issues.apache.org/jira/browse/MSHARED-1248
Ideas?

Copy link
Member

Choose a reason for hiding this comment

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

As I see all project production and tests classes are analyzed.
Maybe allowing exclusion for some path, patterns will be what we need.

In specific project we know which classes are broken and which should be excluded.

So we not need to ignore errors when we will can exclude what we want.

@garydgregory - What do you think?

Copy link
Contributor

@elharo elharo Jun 19, 2023

Choose a reason for hiding this comment

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

Make this log message distinct. E.g. "Byte code of " + className + " is corrupt" and possibly include the name or path of the jar file in which the class appears.

Copy link
Member Author

Choose a reason for hiding this comment

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

@elharo
I updated the message. I do not see where to get a path since we are starting from an InputStream.

}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@
import static org.assertj.core.api.Assertions.assertThat;

class ResultCollectorTest {

private static String ROOT = "src/test/resources/org/apache/maven/shared/dependency/analyzer";

Set<String> getDependencies(Class<?> inspectClass) throws IOException {
String className = inspectClass.getName();
String path = '/' + className.replace('.', '/') + ".class";
Expand All @@ -50,7 +53,61 @@ void testJava11Invoke() throws IOException {
"src/test/resources/org/apache/maven/shared/dependency/analyzer/commons-bcel-issue362/Bcel362.classx");
DependencyClassFileVisitor visitor = new DependencyClassFileVisitor();
try (InputStream is = Files.newInputStream(path)) {
visitor.visitClass(className, is);
visitor.visitClass("issue362.Bcel362", is);
}
}

@Test
public void testOssFuzz51980() throws IOException {
// Add a non-"class" suffix so that surefire does not try to read the file and fail the build
visitClass(ROOT + "/ossfuzz/issue51980/Test.class.clazz");
}

@Test
public void testOssFuzz51989() throws IOException {
visitClass(ROOT + "/ossfuzz/issue51989/Test.class.clazz");
}

@Test
public void testOssFuzz52168() throws IOException {
visitClass(ROOT + "/ossfuzz/issue52168/Test.class.clazz");
}

@Test
public void testOssFuzz53543() throws IOException {
visitClass(ROOT + "/ossfuzz/issue53543/Test.class.clazz");
}

@Test
public void testOssFuzz53544a() throws IOException {
visitClass(ROOT + "/ossfuzz/issue53544a/Test.class.clazz");
}

@Test
public void testOssFuzz53620() throws IOException {
visitClass(ROOT + "/ossfuzz/issue53620/Test.class.clazz");
}

@Test
public void testOssFuzz53676() throws IOException {
visitClass(ROOT + "/ossfuzz/issue53676/Test.class.clazz");
}

@Test
public void testOssFuzz54199() throws IOException {
visitClass(ROOT + "/ossfuzz/issue54119/Test.class.clazz");
}

@Test
public void testOssFuzz54254() throws IOException {
visitClass(ROOT + "/ossfuzz/issue54254/Test.class.clazz");
}

private void visitClass(String location) throws IOException {
Path path = Paths.get(location);
DependencyClassFileVisitor visitor = new DependencyClassFileVisitor();
try (InputStream is = Files.newInputStream(path)) {
visitor.visitClass("Test", is);
}
}

Expand Down
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.