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

Some detectors broken since 4.5.3.0 #1919

Closed
fproulx-boostsecurity opened this issue Jan 12, 2022 · 4 comments
Closed

Some detectors broken since 4.5.3.0 #1919

fproulx-boostsecurity opened this issue Jan 12, 2022 · 4 comments

Comments

@fproulx-boostsecurity
Copy link

With the same pom.xml, I get normal execution with 4.5.2.0, but switching to 4.5.3.0, I get some errors with some FindSecBugs detectors:

pom.xml plugins section

<!-- SpotBugs Static Analysis -->
<plugin>
<groupId>com.github.spotbugs</groupId>
<artifactId>spotbugs-maven-plugin</artifactId>
<version>4.5.3.0</version>    <!-- Works fine with 4.5.2.0 -->
<configuration>
	<effort>Max</effort>
	<threshold>Normal</threshold>
	<failOnError>false</failOnError>
	<includeFilterFile>${session.executionRootDirectory}/.github/findsecbugs/spotbugs-security-include.xml</includeFilterFile>
	<plugins>
		<plugin>
			<groupId>com.h3xstream.findsecbugs</groupId>
			<artifactId>findsecbugs-plugin</artifactId>
			<version>1.11.0</version>
		</plugin>
	</plugins>
</configuration>
</plugin>
 mvn clean compile com.github.spotbugs:spotbugs-maven-plugin:spotbugs                                                                                                                                                                                               master  ✭
[INFO] Scanning for projects...
[INFO]
[INFO] --------------------< io.shiftleft:hello-shiftleft >--------------------
[INFO] Building hello-shiftleft 0.0.1
[INFO] --------------------------------[ jar ]---------------------------------
[INFO]
[INFO] --- maven-clean-plugin:2.6.1:clean (default-clean) @ hello-shiftleft ---
[INFO] Deleting /Users/fproulx/sandbox/java/shiftleft-java-demo/target
[INFO]
[INFO] --- maven-resources-plugin:2.6:resources (default-resources) @ hello-shiftleft ---
[INFO] Using 'UTF-8' encoding to copy filtered resources.
[INFO] Copying 4 resources
[INFO] Copying 4 resources
[INFO]
[INFO] --- maven-compiler-plugin:3.6.1:compile (default-compile) @ hello-shiftleft ---
[INFO] Changes detected - recompiling the module!
[INFO] Compiling 21 source files to /Users/fproulx/sandbox/java/shiftleft-java-demo/target/classes
[INFO]
[INFO] --- spotbugs-maven-plugin:4.5.3.0:spotbugs (default-cli) @ hello-shiftleft ---
[INFO] Fork Value is true
     [java] The following errors occurred during analysis:
     [java]   Exception analyzing io.shiftleft.controller.AppErrorController using detector com.h3xstream.findsecbugs.spring.SpringEntityLeakDetector
     [java]     java.lang.IllegalArgumentException: Invalid class name java/util/Map<Ljava/lang/String;Ljava/lang/Object
     [java]       At edu.umd.cs.findbugs.classfile.ClassDescriptor.<init>(ClassDescriptor.java:59)
     [java]       At edu.umd.cs.findbugs.classfile.DescriptorFactory.getClassDescriptor(DescriptorFactory.java:128)
     [java]       At edu.umd.cs.findbugs.AnalysisCacheToRepositoryAdapter.loadClass(AnalysisCacheToRepositoryAdapter.java:90)
     [java]       At org.apache.bcel.Repository.lookupClass(Repository.java:65)
     [java]       At com.h3xstream.findsecbugs.spring.SignatureParserWithGeneric.typeToJavaClass(SignatureParserWithGeneric.java:73)
     [java]       At com.h3xstream.findsecbugs.spring.SignatureParserWithGeneric.getReturnClasses(SignatureParserWithGeneric.java:58)
     [java]       At com.h3xstream.findsecbugs.spring.SpringEntityLeakDetector.analyzeMethod(SpringEntityLeakDetector.java:112)
     [java]       At com.h3xstream.findsecbugs.spring.SpringEntityLeakDetector.visitClassContext(SpringEntityLeakDetector.java:69)
     [java]       At edu.umd.cs.findbugs.DetectorToDetector2Adapter.visitClass(DetectorToDetector2Adapter.java:76)
     [java]       At edu.umd.cs.findbugs.FindBugs2.lambda$analyzeApplication$1(FindBugs2.java:1108)
     [java]       At java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
     [java]       At edu.umd.cs.findbugs.CurrentThreadExecutorService.execute(CurrentThreadExecutorService.java:86)
     [java]       At java.base/java.util.concurrent.AbstractExecutorService.invokeAll(AbstractExecutorService.java:247)
     [java]       At edu.umd.cs.findbugs.FindBugs2.analyzeApplication(FindBugs2.java:1118)
     [java]       At edu.umd.cs.findbugs.FindBugs2.execute(FindBugs2.java:309)
     [java]       At edu.umd.cs.findbugs.FindBugs.runMain(FindBugs.java:395)
     [java]       At edu.umd.cs.findbugs.FindBugs2.main(FindBugs2.java:1231)
@gtoison
Copy link
Contributor

gtoison commented Jan 18, 2022

@studro wouldn't this be a regression since #1884 ?

@studro
Copy link
Contributor

studro commented Jan 18, 2022

@gtoison Interesting. I had a suspicion that things downstream could break due to tightening up the class name check, but hoped we would have been saved by test coverage.

I'd like to point out that I'm not a spotbugs maintainer or long-term contributor - this was just a single patch to fix invalid rejection of edge-case Kotlin classes (with an attempt to actually get class name validation correct while I was there instead of just doing away with it altogether). Therefore, the views of the maintainers and long-term contributors may be different from mine on this topic, and will ultimately should have more weight. My views are my own as a one-off contributor and do not represent the project.

Upon looking at the stack trace, java/util/Map<Ljava/lang/String;Ljava/lang/Object seems like an invalid class name to me, even though it would have been supported in the past (the previous check was simply checking that the class name did not contain ( or ), which was plainly wrong). Is there a reason the code in com.h3xstream.findsecbugs.spring is trying to load a class with this name? It seems like it may have just silently failed in the past. It would probably be better to fix this in the downstream code, if possible, rather than accepting names that do not comply with JLS 13.1 and JVMS 4.2.

I might be happy to analyse this further and either contribute a PR to either here or find-sec-bugs, but unfortunately, it will likely be many weeks before I will have time to do it.

@scottsteen
Copy link

I raised the same issue over on find-sec-bugs - find-sec-bugs/find-sec-bugs#668.

I also raised a PR over there to resolve the issue - find-sec-bugs/find-sec-bugs#669

@studro
Copy link
Contributor

studro commented Jan 24, 2022

@scottsteen 🚀 Great work! Thanks for fixing this downstream.

@KengoTODA - this issue can possibly be closed, as this will be fixed in find-sec-bugs/find-sec-bugs#669.

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

No branches or pull requests

5 participants