From: Alexander Kriegisch <Alexander@Kriegisch.name>
Date: Thu, 29 Dec 2022 14:24:27 +0000 (+0100)
Subject: Improve BcelTypeMunger.mungeMethodDelegate to avoid race condition
X-Git-Tag: V1_9_20~65
X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=0ea66aedba0ab3c275255a64f1f5d76c2f39ce1f;p=aspectj.git

Improve BcelTypeMunger.mungeMethodDelegate to avoid race condition

Fixes #198, i.e. test DeclareMixinTests.testCaseEConcurrent from the
previous commit now passes.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
---

diff --git a/weaver/src/main/java/org/aspectj/weaver/bcel/BcelTypeMunger.java b/weaver/src/main/java/org/aspectj/weaver/bcel/BcelTypeMunger.java
index 9e56d9e88..0ecad3606 100644
--- a/weaver/src/main/java/org/aspectj/weaver/bcel/BcelTypeMunger.java
+++ b/weaver/src/main/java/org/aspectj/weaver/bcel/BcelTypeMunger.java
@@ -1436,8 +1436,13 @@ public class BcelTypeMunger extends ConcreteTypeMunger {
 			InstructionList body = new InstructionList();
 			InstructionFactory fact = gen.getFactory();
 
-			// getfield
+			// Wrap delegate field initialisation in 'synchronized(this)' block - MONITORENTER (beginning of 'try')
 			body.append(InstructionConstants.ALOAD_0);
+			body.append(InstructionConstants.MONITORENTER);
+
+			// The JVM spec requires us add an exception handler ensuring MONITOREXIT in case of an exception inside the
+			// synchronized block, see https://docs.oracle.com/javase/specs/jvms/se17/html/jvms-3.html#jvms-3.14.
+			InstructionHandle tryStart = body.append(InstructionConstants.ALOAD_0);
 			body.append(Utility.createGet(fact, munger.getDelegate(weaver.getLazyClassGen().getType())));
 			InstructionBranch ifNonNull = InstructionFactory.createBranchInstruction(Constants.IFNONNULL, null);
 			body.append(ifNonNull);
@@ -1487,9 +1492,33 @@ public class BcelTypeMunger extends ConcreteTypeMunger {
 				body.append(Utility.createSet(fact, munger.getDelegate(weaver.getLazyClassGen().getType())));
 			}
 
-			// if not null use the instance we've got
+			// Wrap delegate field initialisation in 'synchronized(this)' block - MONITOREXIT (end of 'try')
 			InstructionHandle ifNonNullElse = body.append(InstructionConstants.ALOAD_0);
 			ifNonNull.setTarget(ifNonNullElse);
+			body.append(InstructionConstants.MONITOREXIT);
+
+			// There was no error in the 'synchronized(this)' block -> jump to first instruction after exception handler
+			InstructionBranch gotoAfterTryCatch = new InstructionBranch(Constants.GOTO, null);
+			InstructionHandle tryEnd = body.append(gotoAfterTryCatch);
+
+			// Exception handler (logical 'catch') for the 'synchronized(this)' block ensures that MONITOREXIT is also called
+			// in case of an error
+			InstructionHandle catchStart = body.append(InstructionConstants.ALOAD_0);
+			body.append(InstructionConstants.MONITOREXIT);
+			InstructionHandle catchEnd = body.append(InstructionConstants.ATHROW);
+
+			// Add exception handler for 'synchronized(this)' block
+			mg.addExceptionHandler(tryStart, tryEnd.getPrev(), catchStart, null, false);
+
+			// CAVEAT: Add an extra, self-referential exception handler entry. I.e., the handler handles its own exceptions.
+			// According to https://docs.oracle.com/javase/specs/jvms/se17/html/jvms-3.html#jvms-3.14, this is required, and
+			// Javac also creates code like this, which is why we are mimicking it.
+			mg.addExceptionHandler(catchStart, catchEnd.getPrev(), catchStart, null, false);
+
+			// if not null use the instance we've got
+			InstructionHandle afterTryCatch = body.append(InstructionConstants.ALOAD_0);
+			// Tell 'gotoAfterTryCatch' where to find the first statement after the exception handler
+			gotoAfterTryCatch.setTarget(afterTryCatch);
 			body.append(Utility.createGet(fact, munger.getDelegate(weaver.getLazyClassGen().getType())));
 
 			// args