From 91024728dc6033889507f6c5d7510931761fb1b8 Mon Sep 17 00:00:00 2001
From: Dmitry Mikhaylov <dmitry@smallgiantgames.com>
Date: Tue, 29 Jun 2021 09:53:18 +0300
Subject: [PATCH] minor improvements

---
 .../bcel/classfile/LocalVariableTable.java    | 13 +++++--
 .../LocalVariableTableConcurrencyTest.java    | 37 ++++++++++---------
 2 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/bcel-builder/src/main/java/org/aspectj/apache/bcel/classfile/LocalVariableTable.java b/bcel-builder/src/main/java/org/aspectj/apache/bcel/classfile/LocalVariableTable.java
index 545227050..555e6bb2b 100644
--- a/bcel-builder/src/main/java/org/aspectj/apache/bcel/classfile/LocalVariableTable.java
+++ b/bcel-builder/src/main/java/org/aspectj/apache/bcel/classfile/LocalVariableTable.java
@@ -189,9 +189,16 @@ public class LocalVariableTable extends Attribute {
 		return buf.toString();
 	}
 
-	@Override
-	public Object clone() throws CloneNotSupportedException {
-		return super.clone();
+	/**
+	 * Returns copy of this attribute using same packed state. Used in unit tests.
+	 */
+	public synchronized LocalVariableTable copyFromPackedState() {
+		if (!isInPackedState) throw new IllegalStateException("No in packed state");
+		try {
+			return new LocalVariableTable(nameIndex, length, new DataInputStream(new ByteArrayInputStream(data)), getConstantPool());
+		} catch (IOException e) {
+			throw new RuntimeException("Failed to unpack clone", e);
+		}
 	}
 
 	/**
diff --git a/bcel-builder/src/test/java/org/aspectj/apache/bcel/classfile/tests/LocalVariableTableConcurrencyTest.java b/bcel-builder/src/test/java/org/aspectj/apache/bcel/classfile/tests/LocalVariableTableConcurrencyTest.java
index da9b8c296..b62ffe380 100644
--- a/bcel-builder/src/test/java/org/aspectj/apache/bcel/classfile/tests/LocalVariableTableConcurrencyTest.java
+++ b/bcel-builder/src/test/java/org/aspectj/apache/bcel/classfile/tests/LocalVariableTableConcurrencyTest.java
@@ -7,7 +7,7 @@
  * http://www.eclipse.org/legal/epl-v10.html
  *
  * Contributors:
- *     Andy Clement -     initial implementation
+ *     Dmitry Mikhaylov - initial implementation
  * ******************************************************************/
 
 package org.aspectj.apache.bcel.classfile.tests;
@@ -16,7 +16,6 @@ import org.aspectj.apache.bcel.classfile.Code;
 import org.aspectj.apache.bcel.classfile.JavaClass;
 import org.aspectj.apache.bcel.classfile.LocalVariableTable;
 import org.aspectj.apache.bcel.classfile.Method;
-import org.aspectj.apache.bcel.classfile.tests.BcelTestCase;
 
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutorService;
@@ -35,29 +34,31 @@ public class LocalVariableTableConcurrencyTest extends BcelTestCase {
 
 	protected void setUp() throws Exception {
 		super.setUp();
+
 		for (int i = 0; i < nThreads; i++) workers[i] = Executors.newSingleThreadExecutor();
 
 		JavaClass clazz = getClassFromJar("SimpleGenericsProgram");
-
 		Method mainMethod = getMethod(clazz,"main");
 		Code codeAttr = (Code) findAttribute("Code",mainMethod.getAttributes());
-
-		reference =
-				(LocalVariableTable) findAttribute("LocalVariableTable",codeAttr.getAttributes());
-	}
-
-	private LocalVariableTable createReferenceCopy() {
-		try {
-			return (LocalVariableTable) reference.clone();
-		} catch (CloneNotSupportedException e) {
-			throw new RuntimeException("Faileed to clone LocalVariableTable", e);
-		}
+		reference = (LocalVariableTable) findAttribute("LocalVariableTable",codeAttr.getAttributes());
 	}
 
+	/**
+	 * Try to hit concurrency bug in org.aspectj.apache.bcel.classfile.LocalVariableTable.unpack().
+	 * We do so by running unpack() on same instance with multiple threads, and artificially
+	 * delaying all threads but first so that they enter unpack() the moment first thread is about to leave it.
+	 *
+	 * Since this test relies on empirically obtained access pattern and number of iterations,
+	 * it never has 100% probability of hitting the bug. If it fails - there is certainly a bug.
+	 * If it passes, it could mean anything - fully correct code or slightly changed execution order preventing
+	 * threads to collide at problematic location.
+	 *
+	 * As such, it is not really good for unit testing.
+	 */
 	public void testLocalVariableTableAttributeConcurrency() throws RuntimeException, InterruptedException {
 		final AtomicReference<RuntimeException> error = new AtomicReference<>();
-		for (int i = 0; i < 100000; i++) {
-			LocalVariableTable sharedInstance = createReferenceCopy();
+		for (int i = 0; i < 10000; i++) {
+			LocalVariableTable sharedInstance = reference.copyFromPackedState();
 			CountDownLatch preStart = new CountDownLatch(nThreads);
 			Semaphore start = new Semaphore(0);
 			CountDownLatch finish = new CountDownLatch(nThreads);
@@ -67,8 +68,8 @@ public class LocalVariableTableConcurrencyTest extends BcelTestCase {
 				workers[j].execute(() -> {
 					preStart.countDown();
 					start.acquireUninterruptibly();
-					// trying to trigger concurrent unpacking - one tread should enter unpack() when other is about to leave it
-					if (needsDelay) createReferenceCopy().getTableLength();
+					// trying to trigger concurrent unpacking bug - one tread should enter unpack() when other is about to leave it
+					if (needsDelay) reference.copyFromPackedState().getTableLength();
 					try {
 						sharedInstance.getTableLength();
 					}
-- 
2.39.5