find-sec-bugs / find-sec-bugs

The SpotBugs plugin for security audits of Java web applications and Android applications. (Also work with Kotlin, Groovy and Scala projects)
https://find-sec-bugs.github.io/
GNU Lesser General Public License v3.0
2.29k stars 474 forks source link

findsecbugs threads hang in java.util.WeakHashMap.get(WeakHashMap.java:403) #595

Open mhnot opened 4 years ago

mhnot commented 4 years ago

Environment

Component Version
Java 1.8.0.72
SpotBugs 4.1.2
FindSecBugs 1.10.1
Eclipse 4.16.0.20200615-1200

Problem

The find-security-bugs plugin for eclipse runs sometimes into a stucked thread problem with my project. Every now and then some threads are hanging at full CPU usage.

sample stacktrace:

"Worker-1300: Finding bugs in axcng-service-integrationtests..." #3795 prio=5 os_prio=0 tid=0x000000002b04f000 nid=0x804 runnable [0x000000007530d000]
   java.lang.Thread.State: RUNNABLE
    at java.util.WeakHashMap.get(WeakHashMap.java:403)
    at com.h3xstream.findsecbugs.BCELUtil.getParentClassNames(BCELUtil.java:65)
    at com.h3xstream.findsecbugs.taintanalysis.TaintConfig.getSuperMethodConfig(TaintConfig.java:243)
    at com.h3xstream.findsecbugs.taintanalysis.TaintConfig.getMethodConfig(TaintConfig.java:227)
    at com.h3xstream.findsecbugs.taintanalysis.TaintFrameModelingVisitor.getMethodConfig(TaintFrameModelingVisitor.java:609)
    at com.h3xstream.findsecbugs.taintanalysis.TaintFrameModelingVisitor.visitInvoke(TaintFrameModelingVisitor.java:560)
    at com.h3xstream.findsecbugs.taintanalysis.TaintFrameModelingVisitor.visitINVOKEVIRTUAL(TaintFrameModelingVisitor.java:390)
    at org.apache.bcel.generic.INVOKEVIRTUAL.accept(INVOKEVIRTUAL.java:88)
    at edu.umd.cs.findbugs.ba.AbstractFrameModelingVisitor.analyzeInstruction(AbstractFrameModelingVisitor.java:84)
    at com.h3xstream.findsecbugs.taintanalysis.TaintFrameModelingVisitor.analyzeInstruction(TaintFrameModelingVisitor.java:129)
    at com.h3xstream.findsecbugs.taintanalysis.TaintAnalysis.transferInstruction(TaintAnalysis.java:90)
    at com.h3xstream.findsecbugs.taintanalysis.TaintAnalysis.transferInstruction(TaintAnalysis.java:51)
    at edu.umd.cs.findbugs.ba.AbstractDataflowAnalysis.transfer(AbstractDataflowAnalysis.java:136)
    at edu.umd.cs.findbugs.ba.Dataflow.execute(Dataflow.java:378)
    at com.h3xstream.findsecbugs.taintanalysis.TaintDataflowEngine.analyze(TaintDataflowEngine.java:183)
    at com.h3xstream.findsecbugs.taintanalysis.TaintDataflowEngine.analyze(TaintDataflowEngine.java:56)
    at edu.umd.cs.findbugs.classfile.impl.AnalysisCache.analyzeMethod(AnalysisCache.java:368)
    at edu.umd.cs.findbugs.classfile.impl.AnalysisCache.getMethodAnalysis(AnalysisCache.java:321)
    at com.h3xstream.findsecbugs.injection.AbstractTaintDetector.getTaintDataFlow(AbstractTaintDetector.java:142)
    at com.h3xstream.findsecbugs.injection.AbstractTaintDetector.analyzeMethod(AbstractTaintDetector.java:109)
    at com.h3xstream.findsecbugs.injection.AbstractTaintDetector.visitClassContext(AbstractTaintDetector.java:94)
    at edu.umd.cs.findbugs.DetectorToDetector2Adapter.visitClass(DetectorToDetector2Adapter.java:76)
    at edu.umd.cs.findbugs.FindBugs2.lambda$null$1(FindBugs2.java:1108)
    at edu.umd.cs.findbugs.FindBugs2$$Lambda$865/805974058.call(Unknown Source)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at edu.umd.cs.findbugs.CurrentThreadExecutorService.execute(CurrentThreadExecutorService.java:86)
    at java.util.concurrent.AbstractExecutorService.invokeAll(AbstractExecutorService.java:238)
    at edu.umd.cs.findbugs.FindBugs2.analyzeApplication(FindBugs2.java:1118)
    at edu.umd.cs.findbugs.FindBugs2.execute(FindBugs2.java:309)
    at de.tobject.findbugs.builder.FindBugsWorker.runFindBugs(FindBugsWorker.java:313)
    at de.tobject.findbugs.builder.FindBugsWorker.work(FindBugsWorker.java:219)
    at de.tobject.findbugs.actions.FindBugsAction$StartedFromViewJob.runWithProgress(FindBugsAction.java:275)
    at de.tobject.findbugs.FindBugsJob.run(FindBugsJob.java:142)
    at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63)

I pulled the stacktraces many times and they are always hanging at "java.util.WeakHashMap.get(WeakHashMap.java:403)"

The problem seems to be caused by the usage of WeakHashMap which is not thread safe and some concurrent access.

topolik commented 4 years ago

Hi @mhnot,

The whole thing runs using one thread only so we optimize for CPU, the process can eat lot of memory. How much memory do you give it? My guess is that it's GC pauses, especially if it eats your whole CPU (compared to one thread).

mhnot commented 4 years ago

I use spotbugs in a rather large eclipse workspace consisting of around 140 java projects with a few hundred jars. Eclipse is started with 4GB max heap.

My machine has 4 cores with 8 logical processors. All of them are busy after starting spotbugs within eclipse. I captured multiple thread dumps and every one contains 8 "RUNNABLE" findbugs threads. And multiple of these 8 threads are running findsecbugs while others are busy with stadard spotbugs analysis.

This is the progress view from eclipse showing also multiple parallel operations: EclipseProgressBar

And this is the spotbugs eclipse config: EclipseSpotbugsConfig

I can also provide a threaddump but I have to remove all class names and project names first.

topolik commented 4 years ago

Thanks, it might be possible Eclipse run it with several threads (I don't use Eclipse). That's interesting to know.

Can you please connect jconsole or similar (I use https://www.yourkit.com/) to look at the allocated memory, GC pauses etc?

Also, you have SpotBugs configured to run as an extra job, I'm sorry I don't know what it means. Does it spawn a new thread, JVM? What's the heap there?

GC can remove items from WeakHashMap but it should not create a deadlock, my guess is still full heap and GC trying to free a bit of memory.

For reference I'm testing a webapp with 2000 modules with 1500 jar files (600MB in class files) and the JVM consumes around 10GB of heap.

mhnot commented 4 years ago

It is not a memory problem. Eclipse is started with 4GB max heap. Currently used are roughly 1.7GB. 4 Threads are hanging again in WeakHashMap.get(WeakHashMap.java:403). I triggered several GCs via Eclipse and jconsole but the threads are still hanging even after 1 hour.

I assume that the option "Run SpotBugs analysis as extra job (independent from build job)" leads to faster compile results. The build should be available and Spotbugs can run afterwards.

topolik commented 4 years ago

Thanks @mhnot, then the next step is to come up with a test to reproduce it /cc @h3xstream if he has any idea :)

topolik commented 4 years ago

I'm looking into SpotBugs code and they are not very well prepared for multi-threading as well. The solution here can be 1, to move the cache into a ThreadLocal if the separate threads are analyzing separate parts of application 2, synchronize access to the cache (or CopyOnWrite...) if the threads are sharing parts of application

jerrinot commented 3 years ago

fwiw: concurrent access to j.u.(Weak)HashMap often manifests as an infinite loop in map.get().

jerrinot commented 3 years ago

I think the static superMap cache is fundamentally problematic: Even if you make it thread-safe the problem is that it's still static.

This probably(?) means its lifecycle is bounded to the lifecycle of Eclipse itself. So when you change a class hierarchy - for example by adding or removing a super class - then this change will NOT be reflected in the superMap cache. There is no cache invalidation after all.

In other words: The cache is assuming a class hierarchy cannot change during the lifetime of the cache. This is true e.g. when being executed from Maven. But it's probably not true when being executed from IDE.

There is a good chance it effects also other IDEs. Unless they create a new classloader for each analysis.

Moving the cache to thread-local could help, but you would have to clear it up before each analysis. Otherwise a thread re-use would also lead to a cache re-use and hence to the same issue with class hierarchy changes.

topolik commented 1 month ago

Just stumbled upon https://dowbecki.com/WeakHashMap-is-not-thread-safe/ where they identified the same problem, referencing also https://issues.jenkins.io/browse/JENKINS-6528

The recommendation is to avoid WeakHashMap in a multi-threaded environment completely. I'm working on the fix.