From cd3bd162cb686da73d1e86ae288e8ea93fd2228e Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Sun, 5 Jul 2026 06:01:43 +0300 Subject: [PATCH] Reject synchronization on primitive wrappers --- .../maven/BytecodeComplianceMojo.java | 89 +++++++++++--- .../maven/BytecodeComplianceMojoTest.java | 111 ++++++++++++++++++ 2 files changed, 183 insertions(+), 17 deletions(-) diff --git a/maven/codenameone-maven-plugin/src/main/java/com/codename1/maven/BytecodeComplianceMojo.java b/maven/codenameone-maven-plugin/src/main/java/com/codename1/maven/BytecodeComplianceMojo.java index cae5ba15b6d..10d89236d69 100644 --- a/maven/codenameone-maven-plugin/src/main/java/com/codename1/maven/BytecodeComplianceMojo.java +++ b/maven/codenameone-maven-plugin/src/main/java/com/codename1/maven/BytecodeComplianceMojo.java @@ -63,6 +63,7 @@ public class BytecodeComplianceMojo extends AbstractCN1Mojo { private static final Map SUGGESTED_REPLACEMENTS; private static final Set SIMD_OWNER_NAMES; + private static final Set PRIMITIVE_WRAPPER_INTERNAL_NAMES; static { Map m = new HashMap(); @@ -76,6 +77,16 @@ public class BytecodeComplianceMojo extends AbstractCN1Mojo { simdOwners.add("com/codename1/impl/ios/IOSSimd"); simdOwners.add("com/codename1/impl/javase/JavaSESimd"); SIMD_OWNER_NAMES = Collections.unmodifiableSet(simdOwners); + Set primitiveWrappers = new HashSet(); + primitiveWrappers.add("java/lang/Boolean"); + primitiveWrappers.add("java/lang/Byte"); + primitiveWrappers.add("java/lang/Character"); + primitiveWrappers.add("java/lang/Double"); + primitiveWrappers.add("java/lang/Float"); + primitiveWrappers.add("java/lang/Integer"); + primitiveWrappers.add("java/lang/Long"); + primitiveWrappers.add("java/lang/Short"); + PRIMITIVE_WRAPPER_INTERNAL_NAMES = Collections.unmodifiableSet(primitiveWrappers); } private static final int MAX_CLASS_MAJOR_VERSION = Opcodes.V17; @@ -503,7 +514,7 @@ private List scanProjectClasses(File outputDir, final Map scanProjectClasses(File outputDir, final Map violations) throws IOException, MojoExecutionException { + private void addSemanticStackViolations(File classFile, File outputDir, ClassReader reader, List violations) throws IOException, MojoExecutionException { ClassNode classNode = new ClassNode(); reader.accept(classNode, ClassReader.EXPAND_FRAMES); for (MethodNode method : classNode.methods) { @@ -526,7 +537,7 @@ private void addSimdAllocaViolations(File classFile, File outputDir, ClassReader Analyzer analyzer = new Analyzer(new SimdAllocaInterpreter()); frames = analyzer.analyze(classNode.name, method); } catch (AnalyzerException ex) { - throw new MojoExecutionException("Failed to analyze SIMD alloca usage for " + classFile + " in " + classNode.name + "#" + method.name + method.desc, ex); + throw new MojoExecutionException("Failed to analyze bytecode semantics for " + classFile + " in " + classNode.name + "#" + method.name + method.desc, ex); } for (AbstractInsnNode instruction = method.instructions.getFirst(); instruction != null; instruction = instruction.getNext()) { int index = method.instructions.indexOf(instruction); @@ -535,27 +546,44 @@ private void addSimdAllocaViolations(File classFile, File outputDir, ClassReader continue; } int opcode = instruction.getOpcode(); + if (opcode == Opcodes.MONITORENTER) { + BasicValue lockValue = frame.getStack(frame.getStackSize() - 1); + if (isPrimitiveWrapperValue(lockValue)) { + addSemanticViolation(violations, classFile, outputDir, classNode.name, method, + "Synchronization on primitive wrapper " + lockValue.getType().getInternalName(), + "Use a dedicated Object lock instead of synchronizing on primitive wrapper values."); + } + continue; + } if (opcode == Opcodes.ARETURN) { if (isAllocaValue(frame.getStack(frame.getStackSize() - 1))) { - addViolation(violations, classFile, outputDir, classNode.name, method, "SIMD alloca value returned from method"); + addSemanticViolation(violations, classFile, outputDir, classNode.name, method, + "SIMD alloca value returned from method", + "Keep SIMD alloca scratch arrays method-local and only pass them to Simd methods."); } continue; } if (opcode == Opcodes.PUTSTATIC) { if (isAllocaValue(frame.getStack(frame.getStackSize() - 1))) { - addViolation(violations, classFile, outputDir, classNode.name, method, "SIMD alloca value stored into static field"); + addSemanticViolation(violations, classFile, outputDir, classNode.name, method, + "SIMD alloca value stored into static field", + "Keep SIMD alloca scratch arrays method-local and only pass them to Simd methods."); } continue; } if (opcode == Opcodes.PUTFIELD) { if (isAllocaValue(frame.getStack(frame.getStackSize() - 1))) { - addViolation(violations, classFile, outputDir, classNode.name, method, "SIMD alloca value stored into instance field"); + addSemanticViolation(violations, classFile, outputDir, classNode.name, method, + "SIMD alloca value stored into instance field", + "Keep SIMD alloca scratch arrays method-local and only pass them to Simd methods."); } continue; } if (opcode == Opcodes.AASTORE) { if (isAllocaValue(frame.getStack(frame.getStackSize() - 1))) { - addViolation(violations, classFile, outputDir, classNode.name, method, "SIMD alloca value stored into object array"); + addSemanticViolation(violations, classFile, outputDir, classNode.name, method, + "SIMD alloca value stored into object array", + "Keep SIMD alloca scratch arrays method-local and only pass them to Simd methods."); } continue; } @@ -575,8 +603,9 @@ && isAllocaValue(frame.getStack(frame.getStackSize() - 1 - argumentCount))) { usesAlloca = true; } if (usesAlloca && !isSimdOwner(methodInsn.owner)) { - addViolation(violations, classFile, outputDir, classNode.name, method, - "SIMD alloca value passed to non-Simd method " + methodInsn.owner + "#" + methodInsn.name + methodInsn.desc); + addSemanticViolation(violations, classFile, outputDir, classNode.name, method, + "SIMD alloca value passed to non-Simd method " + methodInsn.owner + "#" + methodInsn.name + methodInsn.desc, + "Keep SIMD alloca scratch arrays method-local and only pass them to Simd methods."); } continue; } @@ -584,7 +613,9 @@ && isAllocaValue(frame.getStack(frame.getStackSize() - 1 - argumentCount))) { Type[] args = Type.getArgumentTypes(((InvokeDynamicInsnNode) instruction).desc); for (int i = 0; i < args.length; i++) { if (isAllocaValue(frame.getStack(frame.getStackSize() - 1 - i))) { - addViolation(violations, classFile, outputDir, classNode.name, method, "SIMD alloca value passed to invokedynamic"); + addSemanticViolation(violations, classFile, outputDir, classNode.name, method, + "SIMD alloca value passed to invokedynamic", + "Keep SIMD alloca scratch arrays method-local and only pass them to Simd methods."); break; } } @@ -593,19 +624,26 @@ && isAllocaValue(frame.getStack(frame.getStackSize() - 1 - argumentCount))) { } } - private void addViolation(List violations, File classFile, File outputDir, String sourceClass, MethodNode method, String referencedMember) { + private void addSemanticViolation(List violations, File classFile, File outputDir, String sourceClass, MethodNode method, String referencedMember, String suggestion) { String relativePath = classFile.getAbsolutePath().replace(outputDir.getAbsolutePath(), ""); if (relativePath.startsWith(File.separator)) { relativePath = relativePath.substring(1); } violations.add(new Violation(sourceClass, method.name + method.desc, referencedMember, - "Keep SIMD alloca scratch arrays method-local and only pass them to Simd methods.", relativePath)); + suggestion, relativePath)); } private static boolean isAllocaValue(BasicValue value) { return value instanceof SimdAllocaValue && ((SimdAllocaValue) value).alloca; } + private static boolean isPrimitiveWrapperValue(BasicValue value) { + if (value == null || value.getType() == null || value.getType().getSort() != Type.OBJECT) { + return false; + } + return PRIMITIVE_WRAPPER_INTERNAL_NAMES.contains(value.getType().getInternalName()); + } + private static final class SimdAllocaValue extends BasicValue { private final boolean alloca; @@ -645,11 +683,13 @@ private SimdAllocaInterpreter() { @Override public BasicValue newValue(Type type) { - BasicValue base = super.newValue(type); - if (base == null || base == BasicValue.UNINITIALIZED_VALUE) { - return base; + if (type == null) { + return BasicValue.UNINITIALIZED_VALUE; + } + if (type == Type.VOID_TYPE) { + return null; } - return new SimdAllocaValue(base.getType(), false); + return new SimdAllocaValue(type, false); } @Override @@ -708,12 +748,27 @@ public BasicValue naryOperation(AbstractInsnNode insn, List out) { diff --git a/maven/codenameone-maven-plugin/src/test/java/com/codename1/maven/BytecodeComplianceMojoTest.java b/maven/codenameone-maven-plugin/src/test/java/com/codename1/maven/BytecodeComplianceMojoTest.java index 28c4c87a642..fcd99e26208 100644 --- a/maven/codenameone-maven-plugin/src/test/java/com/codename1/maven/BytecodeComplianceMojoTest.java +++ b/maven/codenameone-maven-plugin/src/test/java/com/codename1/maven/BytecodeComplianceMojoTest.java @@ -321,6 +321,43 @@ void rejectsSimdAllocaValueReturnedFromMethod(@TempDir Path tempDir) throws Exce "Expected SIMD alloca verifier to reject returning scratch arrays"); } + @Test + void rejectsSynchronizationOnPrimitiveWrapper(@TempDir Path tempDir) throws Exception { + Path outputDir = tempDir.resolve("classes"); + Path allowedDir = tempDir.resolve("allowed"); + Files.createDirectories(outputDir); + Files.createDirectories(allowedDir); + + writeJavaLangObject(allowedDir); + writePrimitiveWrapperApi(allowedDir, "java/lang/Integer", "valueOf", "(I)Ljava/lang/Integer;"); + writePrimitiveWrapperSynchronizedClass(outputDir, "app/IntegerLockUser", "java/lang/Integer", "valueOf", "(I)Ljava/lang/Integer;"); + + BytecodeComplianceMojo mojo = new BytecodeComplianceMojo(); + Map allowedIndex = buildClassIndex(mojo, Collections.singletonList(allowedDir.toFile())); + List violations = scanProjectClasses(mojo, outputDir, allowedIndex, Collections.emptyMap()); + + assertTrue(hasViolationForReferencePrefix(violations, "Synchronization on primitive wrapper java/lang/Integer"), + "Expected primitive wrapper synchronization to be rejected"); + } + + @Test + void allowsSynchronizationOnDedicatedObjectLock(@TempDir Path tempDir) throws Exception { + Path outputDir = tempDir.resolve("classes"); + Path allowedDir = tempDir.resolve("allowed"); + Files.createDirectories(outputDir); + Files.createDirectories(allowedDir); + + writeJavaLangObject(allowedDir); + writeObjectSynchronizedClass(outputDir, "app/ObjectLockUser"); + + BytecodeComplianceMojo mojo = new BytecodeComplianceMojo(); + Map allowedIndex = buildClassIndex(mojo, Collections.singletonList(allowedDir.toFile())); + List violations = scanProjectClasses(mojo, outputDir, allowedIndex, Collections.emptyMap()); + + assertFalse(hasViolationForReferencePrefix(violations, "Synchronization on primitive wrapper"), + "Expected synchronization on a normal Object lock to remain allowed"); + } + @Test void recognizesAllSimdAllocaHelperNames() throws Exception { Method method = BytecodeComplianceMojo.class.getDeclaredMethod("isSimdAllocaMethod", String.class, String.class, String.class); @@ -673,6 +710,65 @@ private void writeAllocaCaller(Path root, String className, AllocaUsage usage) t writeBytes(root, className, writer.toByteArray()); } + private void writePrimitiveWrapperSynchronizedClass(Path root, String className, String owner, String methodName, String descriptor) throws Exception { + ClassWriter writer = new ClassWriter(0); + writer.visit(Opcodes.V1_8, Opcodes.ACC_PUBLIC, className, null, "java/lang/Object", null); + + MethodVisitor init = writer.visitMethod(Opcodes.ACC_PUBLIC, "", "()V", null, null); + init.visitCode(); + init.visitVarInsn(Opcodes.ALOAD, 0); + init.visitMethodInsn(Opcodes.INVOKESPECIAL, "java/lang/Object", "", "()V", false); + init.visitInsn(Opcodes.RETURN); + init.visitMaxs(1, 1); + init.visitEnd(); + + MethodVisitor run = writer.visitMethod(Opcodes.ACC_PUBLIC | Opcodes.ACC_STATIC, "run", "()V", null, null); + run.visitCode(); + run.visitInsn(Opcodes.ICONST_1); + run.visitMethodInsn(Opcodes.INVOKESTATIC, owner, methodName, descriptor, false); + run.visitInsn(Opcodes.DUP); + run.visitVarInsn(Opcodes.ASTORE, 0); + run.visitInsn(Opcodes.MONITORENTER); + run.visitVarInsn(Opcodes.ALOAD, 0); + run.visitInsn(Opcodes.MONITOREXIT); + run.visitInsn(Opcodes.RETURN); + run.visitMaxs(2, 1); + run.visitEnd(); + + writer.visitEnd(); + writeBytes(root, className, writer.toByteArray()); + } + + private void writeObjectSynchronizedClass(Path root, String className) throws Exception { + ClassWriter writer = new ClassWriter(0); + writer.visit(Opcodes.V1_8, Opcodes.ACC_PUBLIC, className, null, "java/lang/Object", null); + + MethodVisitor init = writer.visitMethod(Opcodes.ACC_PUBLIC, "", "()V", null, null); + init.visitCode(); + init.visitVarInsn(Opcodes.ALOAD, 0); + init.visitMethodInsn(Opcodes.INVOKESPECIAL, "java/lang/Object", "", "()V", false); + init.visitInsn(Opcodes.RETURN); + init.visitMaxs(1, 1); + init.visitEnd(); + + MethodVisitor run = writer.visitMethod(Opcodes.ACC_PUBLIC | Opcodes.ACC_STATIC, "run", "()V", null, null); + run.visitCode(); + run.visitTypeInsn(Opcodes.NEW, "java/lang/Object"); + run.visitInsn(Opcodes.DUP); + run.visitMethodInsn(Opcodes.INVOKESPECIAL, "java/lang/Object", "", "()V", false); + run.visitInsn(Opcodes.DUP); + run.visitVarInsn(Opcodes.ASTORE, 0); + run.visitInsn(Opcodes.MONITORENTER); + run.visitVarInsn(Opcodes.ALOAD, 0); + run.visitInsn(Opcodes.MONITOREXIT); + run.visitInsn(Opcodes.RETURN); + run.visitMaxs(2, 1); + run.visitEnd(); + + writer.visitEnd(); + writeBytes(root, className, writer.toByteArray()); + } + private void writeSubclass(Path root, String className, String superName) throws Exception { ClassWriter writer = new ClassWriter(0); @@ -690,6 +786,21 @@ private void writeSubclass(Path root, String className, String superName) throws writeBytes(root, className, writer.toByteArray()); } + private void writePrimitiveWrapperApi(Path root, String className, String methodName, String descriptor) throws Exception { + ClassWriter writer = new ClassWriter(0); + writer.visit(Opcodes.V1_8, Opcodes.ACC_PUBLIC | Opcodes.ACC_FINAL, className, null, "java/lang/Object", null); + + MethodVisitor valueOf = writer.visitMethod(Opcodes.ACC_PUBLIC | Opcodes.ACC_STATIC, methodName, descriptor, null, null); + valueOf.visitCode(); + valueOf.visitInsn(Opcodes.ACONST_NULL); + valueOf.visitInsn(Opcodes.ARETURN); + valueOf.visitMaxs(1, 1); + valueOf.visitEnd(); + + writer.visitEnd(); + writeBytes(root, className, writer.toByteArray()); + } + private void writeApiClass(Path root, String className, String methodName, String descriptor) throws Exception { ClassWriter writer = new ClassWriter(0); writer.visit(Opcodes.V1_8, Opcodes.ACC_PUBLIC, className, null, "java/lang/Object", null);