Fix for Bugzilla #39479, #40109

based on patch contributed by Andy Clement

Generalizes the patch with a method org.aspectj.weaver.bcel.Utility.copyInstruction
that works-around the bug in Select.copy().  Changed all calls to
Instruction.copy() to use this new method, would be nice to add the
rule:
   	 * declare error:
   	 *     call(* Instruction.copy()) && within(org.aspectj.weaver)
   	 *       && !withincode(* Utility.copyInstruction(Instruction)):
   	 *     "use Utility.copyInstruction to work-around bug in Select.copy()";

5 files changed, 42 insertions, 5 deletions

diff --git a/weaver/src/org/aspectj/weaver/bcel/BcelClassWeaver.java b/weaver/src/org/aspectj/weaver/bcel/BcelClassWeaver.java
index 0a4a9d318..97c8fbdae 100644
--- a/weaver/src/org/aspectj/weaver/bcel/BcelClassWeaver.java
+++ b/weaver/src/org/aspectj/weaver/bcel/BcelClassWeaver.java
@@ -443,7 +443,7 @@ class BcelClassWeaver implements IClassWeaver {
 			src != null;
 			src = src.getNext()) 
 		{
-			Instruction fresh = src.getInstruction().copy();
+			Instruction fresh = Utility.copyInstruction(src.getInstruction());
 			InstructionHandle dest;
 			if (fresh instanceof CPInstruction) {
 				// need to reset index to go to new constant pool.  This is totally
diff --git a/weaver/src/org/aspectj/weaver/bcel/BcelShadow.java b/weaver/src/org/aspectj/weaver/bcel/BcelShadow.java
index 058231bab..00b2d2678 100644
--- a/weaver/src/org/aspectj/weaver/bcel/BcelShadow.java
+++ b/weaver/src/org/aspectj/weaver/bcel/BcelShadow.java
@@ -953,7 +953,7 @@ public class BcelShadow extends Shadow {
         for (InstructionHandle ih = range.getStart(); ih != range.getEnd(); ih = ih.getNext()) {
             if (ih.getInstruction() instanceof ReturnInstruction) {
                 returns.add(ih);
-                ret = ih.getInstruction().copy();
+                ret = Utility.copyInstruction(ih.getInstruction());
             }
         }
         InstructionList retList;
diff --git a/weaver/src/org/aspectj/weaver/bcel/LazyMethodGen.java b/weaver/src/org/aspectj/weaver/bcel/LazyMethodGen.java
index f5d87a3e7..569ac9793 100644
--- a/weaver/src/org/aspectj/weaver/bcel/LazyMethodGen.java
+++ b/weaver/src/org/aspectj/weaver/bcel/LazyMethodGen.java
@@ -728,7 +728,7 @@ public final class LazyMethodGen {
                 continue;
             }
             Instruction i = ih.getInstruction();
-            Instruction c = i.copy(); // Use clone for shallow copy
+            Instruction c = Utility.copyInstruction(i);
 
             if (c instanceof BranchInstruction)
                 map.put(ih, fresh.append((BranchInstruction) c));
diff --git a/weaver/src/org/aspectj/weaver/bcel/ShadowRange.java b/weaver/src/org/aspectj/weaver/bcel/ShadowRange.java
index 3bbc1cd8a..cf69bea9c 100644
--- a/weaver/src/org/aspectj/weaver/bcel/ShadowRange.java
+++ b/weaver/src/org/aspectj/weaver/bcel/ShadowRange.java
@@ -71,7 +71,7 @@ final class ShadowRange extends Range {
         for (InstructionHandle oldIh = start.getNext(); oldIh != end; oldIh = oldIh.getNext()) {
 		    // first we copy the instruction itself.  
             Instruction oldI = oldIh.getInstruction();
-            Instruction freshI = (oldI == RANGEINSTRUCTION) ? oldI : oldI.copy();
+            Instruction freshI = (oldI == RANGEINSTRUCTION) ? oldI : Utility.copyInstruction(oldI);
 
 			// Now we add it to the new instruction list.
             InstructionHandle freshIh;
diff --git a/weaver/src/org/aspectj/weaver/bcel/Utility.java b/weaver/src/org/aspectj/weaver/bcel/Utility.java
index d2179010b..93c2347bc 100644
--- a/weaver/src/org/aspectj/weaver/bcel/Utility.java
+++ b/weaver/src/org/aspectj/weaver/bcel/Utility.java
@@ -21,6 +21,7 @@ import org.apache.bcel.Constants;
 import org.apache.bcel.classfile.ClassParser;
 import org.apache.bcel.classfile.JavaClass;
 import org.apache.bcel.classfile.Method;
+import org.apache.bcel.generic.ArrayType;
 import org.apache.bcel.generic.BIPUSH;
 import org.apache.bcel.generic.BasicType;
 import org.apache.bcel.generic.BranchInstruction;
@@ -34,9 +35,10 @@ import org.apache.bcel.generic.InstructionList;
 import org.apache.bcel.generic.InstructionTargeter;
 import org.apache.bcel.generic.LDC;
 import org.apache.bcel.generic.ObjectType;
-import org.apache.bcel.generic.ArrayType;
 import org.apache.bcel.generic.ReferenceType;
 import org.apache.bcel.generic.SIPUSH;
+import org.apache.bcel.generic.SWITCH;
+import org.apache.bcel.generic.Select;
 import org.apache.bcel.generic.TargetLostException;
 import org.apache.bcel.generic.Type;
 import org.aspectj.weaver.BCException;
@@ -419,6 +421,41 @@ public class Utility {
             throw new BCException("this really can't happen");
         }
    	}
+   	
+   	/**
+   	 * Fix for Bugzilla #39479, #40109 patch contributed by Andy Clement
+   	 * 
+   	 * Need to manually copy Select instructions - if we rely on the the 'fresh' object
+   	 * created by copy(), the InstructionHandle array 'targets' inside the Select
+   	 * object will not have been deep copied, so modifying targets in fresh will modify
+   	 * the original Select - not what we want !  (It is a bug in BCEL to do with cloning
+   	 * Select objects).
+   	 * 
+   	 * <pre>
+   	 * declare error:
+   	 *     call(* Instruction.copy()) && within(org.aspectj.weaver)
+   	 *       && !withincode(* Utility.copyInstruction(Instruction)):
+   	 *     "use Utility.copyInstruction to work-around bug in Select.copy()";
+   	 * </pre>
+   	 */
+	public static Instruction copyInstruction(Instruction i) {
+		if (i instanceof Select) {
+			Select freshSelect = (Select)i;
+				  
+			// Create a new targets array that looks just like the existing one
+			InstructionHandle[] targets = new InstructionHandle[freshSelect.getTargets().length];
+			for (int ii = 0; ii < targets.length; ii++) {
+			  targets[ii] = freshSelect.getTargets()[ii];
+			}
+				  
+			// Create a new select statement with the new targets array
+			SWITCH switchStatement = new SWITCH(freshSelect.getMatchs(),targets,freshSelect.getTarget());
+			return (Select)switchStatement.getInstruction();	
+		} else {
+			return i.copy(); // Use clone for shallow copy...
+		}
+	}
+   	
 
 	/** returns -1 if no source line attribute */
 	// this naive version overruns the JVM stack size, if only Java understood tail recursion...