ConcurrentModificationException in SootClass.getMethodUnsafe #1116
Conversation
| @@ -548,7 +548,7 @@ public SootMethod getMethodUnsafe(String name, List<Type> parameterTypes, Type r | |||
| return null; | |||
| } | |||
|
|
|||
| for (SootMethod method : methodList) { | |||
| for (SootMethod method : new ArrayList<>(methodList)) { | |||
There was a problem hiding this comment.
This is not a real fix, because the copy constructor of the ArrayList also iterates over the contents of the original list and copies over all elements. Therefore, if the list is concurrently modified by another thread, this issue might as well strike while the copy constructor is running. The much better question would be why the method is concurrently modified in the first place
There was a problem hiding this comment.
Have you ever encountered a ConcurrentModificationException in the new ArrayList(Collection) constructor? Or are you referring to that the ´methodList` variable is not synchronized?
I have not investigated why the method is concurrently modified, however I assume that this happens because of necessary phantom method references being added as they are used by the current SootClass instance or any other SootClass that is processed at that time in a different thread. I don't see a chance to synchronize the whole process.
There was a problem hiding this comment.
Apparently, the copy constructor in newer JDKs doesn't iterate, but uses System.arrayCopy deep down. So you're technically right that you shouldn't get a ConcurrentModificationException anymore, but you might end up with all sorts of funny effects if a resizing happens while you are copying, because ArrayLists have a distinct field for their size, which they use when accessing the internal array. I think it's safe to say that your fix greatly reduces the risk of concurrency issues, but it's still a bit shady. I'm still not sure why aother thread manipulates methods concurrently here. Do you have a stack trace?
There was a problem hiding this comment.
This is one example stack trace I found:
java.util.ConcurrentModificationException: null
at java.base/java.util.ArrayList$Itr.checkForComodification(ArrayList.java:1042) ~[na:na]
at java.base/java.util.ArrayList$Itr.next(ArrayList.java:996) ~[na:na]
at soot.SootClass.getMethodUnsafe(SootClass.java:553)
at soot.SootMethodRefImpl.tryResolve(SootMethodRefImpl.java:210)
at soot.SootMethodRefImpl.tryResolve(SootMethodRefImpl.java:188)
at soot.toolkits.exceptions.UnitThrowAnalysis.mightThrow(UnitThrowAnalysis.java:297)
at soot.toolkits.exceptions.UnitThrowAnalysis$ValueSwitch.caseInstanceInvokeExpr(UnitThrowAnalysis.java:1120)
at soot.toolkits.exceptions.UnitThrowAnalysis$ValueSwitch.caseVirtualInvokeExpr(UnitThrowAnalysis.java:970)
at soot.jimple.internal.AbstractVirtualInvokeExpr.apply(AbstractVirtualInvokeExpr.java:79)
at soot.toolkits.exceptions.UnitThrowAnalysis.mightThrow(UnitThrowAnalysis.java:289)
at soot.toolkits.exceptions.UnitThrowAnalysis$UnitSwitch.caseAssignStmt(UnitThrowAnalysis.java:751)
at soot.jimple.internal.JAssignStmt.apply(JAssignStmt.java:242)
at soot.toolkits.exceptions.UnitThrowAnalysis.mightThrow(UnitThrowAnalysis.java:275)
at soot.toolkits.graph.ExceptionalUnitGraph.buildExceptionDests(ExceptionalUnitGraph.java:287)
at soot.toolkits.graph.ExceptionalUnitGraph.initialize(ExceptionalUnitGraph.java:229)
at soot.toolkits.graph.ExceptionalUnitGraph.<init>(ExceptionalUnitGraph.java:135)
at soot.toolkits.graph.ExceptionalUnitGraph.<init>(ExceptionalUnitGraph.java:163)
at soot.toolkits.scalar.LocalDefs$Factory.newLocalDefs(LocalDefs.java:65)
at soot.toolkits.scalar.LocalDefs$Factory.newLocalDefs(LocalDefs.java:51)
at soot.jimple.toolkits.scalar.DeadAssignmentEliminator.internalTransform(DeadAssignmentEliminator.java:239)
at soot.BodyTransformer.transform(BodyTransformer.java:55)
at soot.BodyTransformer.transform(BodyTransformer.java:59)
at soot.dexpler.DexBody.jimplify(DexBody.java:644)
at soot.dexpler.DexMethod$1.getBody(DexMethod.java:119)
at soot.SootMethod.retrieveActiveBody(SootMethod.java:402)
It occurred when processing <cjh: void <init>(cja)> of com.starfinanz.smob.android.bwmobilbanking (one of several apps that caused this error).
I think more interesting would be where the call that modified the methodList came from, but that is not that simple to get.
There was a problem hiding this comment.
Where do you call retrieveActiveBody()? Does that call stem from user code or from Soot code? Is that call issued in threaded code? I'm still trying to figure out where the multiple threads come from, i.e., why the method list gets changed while someone iterates over it.
There was a problem hiding this comment.
@jpstotz @StevenArzt Can we come to a conclusion how to (not) handle this issue for now?
What do you think about my proposal?
There was a problem hiding this comment.
The question is whether we want to give any concurrency guarantees or not. I'd rather not, because there are tons of other places that are likely to fail as well. As I said, we created a concurrent version of Soot, but had to change the semantics of many operations. The only alternative would be to synchronize all accesses to internal collections as suggested by @mbenz89 , but that would pretty much lead to a single-threaded version again.
There was a problem hiding this comment.
I just fixed another issue that is caused by running the jimple body creation in parallel. There, the fast-hierarchy was not proper synchronized during jimple body creation.
@jpstotz is right here, PackManager does the exact parallel execution as he proposes.
I agree that synchronizing every field will ultimately lead to a kind-of single-threaded version again. However, the above-mentioned error seems to rise rather frequently, as it was reported by others as well.
I guess we have to options here:
- Disable the parallel jimplification at all, which will lead to huge performance decreases.
- At least synchronize the method list and see how this plays out, i.e. see which other race conditions we are witnessing.
There was a problem hiding this comment.
Then let's do it like that, i.e., synchronize the method. It seems that we have no better option at the moment.
There was a problem hiding this comment.
Like using this copy-iteration approach proposed by Jan? And synchronizing the method list during the copy operation? Or are you saying we should synchronize the whole methodList?
BTW one of my students just found another synchronization issue, where multiple instances of the same class could be added to the Scene and have different resolving levels (causing #1078 and more). This can be omitted if we synchronize all operations that add/remove/check classes in Scene -.- . It's still far from single-threaded then, but it will definitely worsen performance quite a bit during jimplification, i guess.
|
Might be related to #1083 |
|
Might be related to #1067 |
|
Might be related to #1162 |
|
I found a similar error when processing the apk with soot the call stack is: I fixed the bug by adding new ArrayList<> in soot.FastHierarchy.getSignaturePolymorphicMethod like: |
Using the latest Soot version I often encountered with certain apps a ConcurrentModificationException in
public SootMethod getMethodUnsafe(String name, List<Type> parameterTypes, Type returnType)This pull request fixes that problem.
Additionally it includes a small non-related fix for a missing exception message in
soot.jimple.toolkits.typing.integer.InternalTypingExceptionwhich was always null instead of the actual message.