Community
Participate
Working Groups
Every once in a long while I observed an NPE with a stack trace like this one: java.lang.NullPointerException at org.apache.bcel.generic.InstructionList.getInstructionHandles(InstructionList.java:1021) at org.apache.bcel.generic.InstructionList.findHandle(InstructionList.java:141) at org.apache.bcel.generic.MethodGen.<init>(MethodGen.java:194) at org.eclipse.objectteams.otre.ObjectTeamsTransformation.newMethodGen(ObjectTeamsTransformation.java:1369) at org.eclipse.objectteams.otre.BaseMethodTransformation.checkReplaceWickedSuper(BaseMethodTransformation.java:196) at org.eclipse.objectteams.otre.BaseMethodTransformation.doTransformCode(BaseMethodTransformation.java:181) at org.eclipse.objectteams.otre.jplis.ObjectTeamsTransformer.internalTransform(ObjectTeamsTransformer.java:201) at org.eclipse.objectteams.otre.jplis.ObjectTeamsTransformer.transform(ObjectTeamsTransformer.java:96) at org.eclipse.objectteams.otequinox.internal.hook.TransformerHook.transformClass(TransformerHook.java:397) at org.eclipse.objectteams.otequinox.internal.hook.TransformerHook.processClass(TransformerHook.java:337) Today I succeeded in reproducing the NPE in a small example: Method org.apache.bcel.generic.InstructionHandle.getInstructionHandle(Instruction) is not thread-safe: two threads could share the same InstructionHandle, and as these are links in a linked list, this can mess up the list structure. As an effect stored list length and actual number of linked cells don't match and method InstructionList.getInstructionHandles() may run over the end. => NPE.
Created attachment 194414 [details] proposed fix This patch intercepts the byte codes of the affected bcel class and if all figures match (including CRC) add the required "synchronized" flag to the buggy method. Debugging a runtime workbench I could indeed see that the debugger saw the method as synchronized.
Released as r1466,7
Verified for 0.8M7 using build 201105010849
Even in 2.0 RC2 the NPE still occurred, the difference between stack traces seems to be irrelevant: java.lang.NullPointerException at org.apache.bcel.generic.InstructionList.getInstructionHandles(InstructionList.java:1021) at org.apache.bcel.generic.InstructionList.findHandle(InstructionList.java:141) at org.apache.bcel.generic.MethodGen.<init>(MethodGen.java:208) at org.eclipse.objectteams.otre.ObjectTeamsTransformation.newMethodGen(ObjectTeamsTransformation.java:1369)
Created attachment 196842 [details] additional fixes This patch includes three more methods in the original strategy: InstructionHandle.addHandle() BranchHandle.getBranchHandle() BranchHandle.addHandle() All these methods access a static field (ih_list resp. bh_list) and must be synchronized.
Resolved: - log shows that both classes are now hot patched - debugger shows that all these methods are indeed synchronized now, (with the exception of BranchHandle.addHandle() which is never called when running the OTDT).
Verified using build 201105311237
I've once again seen the exception in the debugger after the logs said that all hot-patches had been successfully applied. From re-reading the comments and inspecting the sources I suspect that synchronization is still broken because two of the methods are non-static while synchronization against the class is required.
A new solution has been pushed as commit 3b1be84439870630980a6b057ddcb99a827dfd79 Now we don't just patch individual bytes but replace the entire two class files. This enables us to add blocks like this synchronized(InstructionHandle.class) { ... }
While previous patches made the issue less frequent it could still be observed once in a while. As I could inspect a corrupted InstructionList in the debugger it looked exactly as caused by the lack of synchronization, although the patch had been applied. This leaves me with only one explanation: synchronization on the non-volatile field ih_list isn't sufficient to force propagating the change between threads. Since making the field volatile defies its purpose of optimization I'm removing those static lists altogether. This has been pushed via commit 39cd15fb302def84ef176414e6c517fe0a037738
Issue hasn't been observed since the latest fix. Thus marking as verified for 2.1.
