From 463d19277ee648250a5fe662a96fdcfd5580b469 Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Tue, 19 May 2026 16:26:14 +0300 Subject: [PATCH] Fix #3108: don't rely on C arg eval order in PUTFIELD/MULTIANEWARRAY ParparVM emitted C of the form set_field_X(POP_DOUBLE(), POP_OBJ()) for primitive instance-field writes when the value/target operands could not be folded into literals, and alloc3DArray/alloc4DArray(td, POP_INT(), POP_INT(), ...) for MULTIANEWARRAY. C does not specify the order of function-argument evaluation, so clang on iOS evaluated the POPs right-to-left and: - PUTFIELD: popped the double's bits as an object reference, dereferenced garbage, and crashed with the NPE reported in #3108. Reproduced via chained assignment with widening conversion (a.x = a.y = (int)expr), which forces a dup2_x1 sequence that defeats the existing operand folding in Field.tryReduce. - MULTIANEWARRAY: silently swapped the array dimensions, so new int[a][b][c] could be allocated as if it were new int[c][b][a]. Both sites now use PEEK semantics with an explicit SP adjustment, mirror- ing the pattern already used by the object PUTFIELD path and the 2D MULTIANEWARRAY case. The MultiArray fix also corrects the (currently dead, but defensive) dims --- .../tools/translator/bytecodes/Field.java | 30 ++-- .../translator/bytecodes/MultiArray.java | 44 ++++-- .../BytecodeInstructionIntegrationTest.java | 135 ++++++++++++++++++ 3 files changed, 178 insertions(+), 31 deletions(-) diff --git a/vm/ByteCodeTranslator/src/com/codename1/tools/translator/bytecodes/Field.java b/vm/ByteCodeTranslator/src/com/codename1/tools/translator/bytecodes/Field.java index e490fb1e6f..f535414024 100644 --- a/vm/ByteCodeTranslator/src/com/codename1/tools/translator/bytecodes/Field.java +++ b/vm/ByteCodeTranslator/src/com/codename1/tools/translator/bytecodes/Field.java @@ -344,35 +344,33 @@ public void appendInstruction(StringBuilder sbOut) { valueOpAppended = true; targetOpAppended = true; } else { + // Pop semantics here matter: C does not specify the order of + // function argument evaluation, so emitting + // "set_field_X(POP_DOUBLE(), POP_OBJ())" can pop the operands + // in the wrong order on iOS (clang). This manifests when the + // value and target both come from the stack (e.g. after a + // chained assignment "a.x = a.y = expr" that produces a + // dup2_x1 + two putfields). Always read with PEEK and pop + // separately. See issue #3108. switch(desc.charAt(0)) { case 'L': case '[': - b.append("PEEK_OBJ"); - //if(useThis) { - // b.append("(1), __cn1ThisObject);\n SP--;\n"); - //} else { - b.append("(1), PEEK_OBJ(2));\n POP_MANY(2);\n"); - //} - sbOut.append(b); + b.append("PEEK_OBJ(1), PEEK_OBJ(2));\n POP_MANY(2);\n"); + sbOut.append(b); return; case 'D': - b.append("POP_DOUBLE"); + b.append("PEEK_DOUBLE(1), PEEK_OBJ(2));\n SP -= 2;\n"); break; case 'F': - b.append("POP_FLOAT"); + b.append("PEEK_FLOAT(1), PEEK_OBJ(2));\n SP -= 2;\n"); break; case 'J': - b.append("POP_LONG"); + b.append("PEEK_LONG(1), PEEK_OBJ(2));\n SP -= 2;\n"); break; default: - b.append("POP_INT"); + b.append("PEEK_INT(1), PEEK_OBJ(2));\n SP -= 2;\n"); break; } - //if(useThis) { - // b.append("(), __cn1ThisObject);\n"); - //} else { - b.append("(), POP_OBJ());\n"); - //} } break; } diff --git a/vm/ByteCodeTranslator/src/com/codename1/tools/translator/bytecodes/MultiArray.java b/vm/ByteCodeTranslator/src/com/codename1/tools/translator/bytecodes/MultiArray.java index ee56ad1874..80e81631e5 100644 --- a/vm/ByteCodeTranslator/src/com/codename1/tools/translator/bytecodes/MultiArray.java +++ b/vm/ByteCodeTranslator/src/com/codename1/tools/translator/bytecodes/MultiArray.java @@ -124,18 +124,30 @@ public void appendInstruction(StringBuilder b, List l) { break; case 3: - b.append(" PUSH_OBJ(alloc3DArray(threadStateData, POP_INT(), "); + // alloc3DArray params are (innermost, middle, outermost). + // JVM stack: outermost at the bottom, innermost-of-specified at + // the top, so after "SP -= dims" SP[0]..SP[dims-1] hold the + // values outermost..innermost. Any unspecified inner dims are + // passed as -1. + // + // Emitting "alloc3DArray(td, POP_INT(), POP_INT(), POP_INT())" + // is unsafe -- C does not specify function argument evaluation + // order, so clang on iOS can pop them in reverse and silently + // swap the dimensions. Use PEEK semantics with explicit SP + // adjustment instead (same pattern as the 2D case above and + // the PUTFIELD fix from issue #3108). + b.append(" SP -= ").append(dims).append("; PUSH_OBJ(alloc3DArray(threadStateData, "); switch(dims) { case 1: - b.append("-1, -1"); + b.append("-1, -1, (*SP).data.i"); break; - + case 2: - b.append("POP_INT(), -1"); + b.append("-1, (*(SP+1)).data.i, (*SP).data.i"); break; - + case 3: - b.append("POP_INT(), POP_INT()"); + b.append("(*(SP+2)).data.i, (*(SP+1)).data.i, (*SP).data.i"); break; } b.append(", &class_array3__"); @@ -152,24 +164,26 @@ public void appendInstruction(StringBuilder b, List l) { } b.append("))); /* MULTIANEWARRAY */\n"); break; - + case 4: - b.append(" PUSH_OBJ(alloc4DArray(threadStateData, POP_INT(), "); + // See comment on case 3 -- same rationale. alloc4DArray params + // are (innermost, depth-2, depth-3, outermost). + b.append(" SP -= ").append(dims).append("; PUSH_OBJ(alloc4DArray(threadStateData, "); switch(dims) { case 1: - b.append("-1, -1, -1"); + b.append("-1, -1, -1, (*SP).data.i"); break; - + case 2: - b.append("POP_INT(), -1, -1"); + b.append("-1, -1, (*(SP+1)).data.i, (*SP).data.i"); break; - + case 3: - b.append("POP_INT(), POP_INT(), -1"); + b.append("-1, (*(SP+2)).data.i, (*(SP+1)).data.i, (*SP).data.i"); break; - + case 4: - b.append("POP_INT(), POP_INT(), POP_INT()"); + b.append("(*(SP+3)).data.i, (*(SP+2)).data.i, (*(SP+1)).data.i, (*SP).data.i"); break; } b.append(", &class_array4__"); diff --git a/vm/tests/src/test/java/com/codename1/tools/translator/BytecodeInstructionIntegrationTest.java b/vm/tests/src/test/java/com/codename1/tools/translator/BytecodeInstructionIntegrationTest.java index 90035a1547..a5d468651a 100644 --- a/vm/tests/src/test/java/com/codename1/tools/translator/BytecodeInstructionIntegrationTest.java +++ b/vm/tests/src/test/java/com/codename1/tools/translator/BytecodeInstructionIntegrationTest.java @@ -564,6 +564,69 @@ void multiArrayAddsDependenciesAndRegistersArrayTypes() throws Exception { } } + /** + * Regression test for the same class of bug as issue #3108, applied to + * MultiArray. The translator used to emit + * alloc3DArray(td, POP_INT(), POP_INT(), POP_INT()) + * and the 4D equivalent. C does not specify function-argument evaluation + * order, so clang on iOS would evaluate the POPs in reverse and silently + * swap the array dimensions -- "new int[a][b][c]" could be allocated as if + * it were "new int[c][b][a]". + * + * Asserts the emitted C reads each dimension with PEEK semantics + * ((*SP).data.i, (*(SP+1)).data.i, ...) after a single SP decrement, so + * the dimensions land in the right alloc?DArray slots regardless of how + * the C compiler reorders the function-argument evaluation. + */ + @Test + void multiArrayEmissionIsArgumentOrderSafe() throws Exception { + Set snapshot = snapshotArrayTypes(); + try { + // 3D, fully-specified ("new int[a][b][c]"). + MultiArray ma3 = new MultiArray("[[[I", 3); + List deps3 = new ArrayList<>(); + ma3.addDependencies(deps3); + StringBuilder out3 = new StringBuilder(); + ma3.appendInstruction(out3, new ArrayList<>()); + String c3 = out3.toString(); + assertTrue(c3.contains("SP -= 3"), + "3D MULTIANEWARRAY should decrement SP once for all dims:\n" + c3); + assertTrue(c3.contains("alloc3DArray(threadStateData, (*(SP+2)).data.i, (*(SP+1)).data.i, (*SP).data.i"), + "3D dims=3 must read dims with PEEK in (innermost, middle, outermost) order:\n" + c3); + assertFalse(c3.contains("POP_INT(), POP_INT()"), + "3D MULTIANEWARRAY must not chain multiple POP_INT calls:\n" + c3); + + // 4D, fully-specified ("new int[a][b][c][d]"). + MultiArray ma4 = new MultiArray("[[[[I", 4); + List deps4 = new ArrayList<>(); + ma4.addDependencies(deps4); + StringBuilder out4 = new StringBuilder(); + ma4.appendInstruction(out4, new ArrayList<>()); + String c4 = out4.toString(); + assertTrue(c4.contains("SP -= 4"), + "4D MULTIANEWARRAY should decrement SP once for all dims:\n" + c4); + assertTrue(c4.contains("alloc4DArray(threadStateData, (*(SP+3)).data.i, (*(SP+2)).data.i, (*(SP+1)).data.i, (*SP).data.i"), + "4D dims=4 must read dims with PEEK in innermost-to-outermost order:\n" + c4); + assertFalse(c4.contains("POP_INT(), POP_INT()"), + "4D MULTIANEWARRAY must not chain multiple POP_INT calls:\n" + c4); + + // 2D was already safe; verify the existing pattern is intact. + MultiArray ma2 = new MultiArray("[[Ljava/lang/String;", 2); + List deps2 = new ArrayList<>(); + ma2.addDependencies(deps2); + StringBuilder out2 = new StringBuilder(); + ma2.appendInstruction(out2, new ArrayList<>()); + String c2 = out2.toString(); + assertTrue(c2.contains("SP -= 2") && + c2.contains("(*(SP+1)).data.i, (*SP).data.i"), + "2D MULTIANEWARRAY must keep the existing PEEK pattern:\n" + c2); + assertFalse(c2.contains("POP_INT(), POP_INT()"), + "2D MULTIANEWARRAY must not chain multiple POP_INT calls:\n" + c2); + } finally { + restoreArrayTypes(snapshot); + } + } + @Test void arrayLengthExpressionReducesAndAssigns() throws Exception { List instructions = new ArrayList<>(); @@ -1081,6 +1144,78 @@ void testBasicInstructionCoverage() { } } + /** + * Regression test for issue #3108: a chained assignment with widening + * conversion ("this.a = this.b = doubleExpr") emits dup2_x1 + two + * putfield's. The translator's unfolded PUTFIELD path used to emit + * "set_field_X(POP_DOUBLE(), POP_OBJ())" -- but C does not specify the + * order of evaluation of function arguments, so clang on iOS was popping + * the operands in the wrong order, causing the double's bits to be + * interpreted as an object reference and crashing with NPE. + * + * This test asserts that the unfolded primitive PUTFIELD path uses PEEK + * (which has no side effect on SP) followed by an explicit pop, mirroring + * the fix that was already in place for the object PUTFIELD path. + */ + @Test + void putfieldUnfoldedPathUsesPeekNotPop() { + // The unfolded path is taken when valueOp and targetOp are null, + // i.e. when tryReduce could not fold the operands into literals. + // Build a Field with PUTFIELD opcode for each primitive desc and + // check the emitted C. + String[][] cases = { + {"D", "PEEK_DOUBLE(1)"}, + {"F", "PEEK_FLOAT(1)"}, + {"J", "PEEK_LONG(1)"}, + {"I", "PEEK_INT(1)"}, + {"Z", "PEEK_INT(1)"}, + {"B", "PEEK_INT(1)"}, + {"C", "PEEK_INT(1)"}, + {"S", "PEEK_INT(1)"}, + }; + for (String[] c : cases) { + String desc = c[0]; + String expectedPeek = c[1]; + com.codename1.tools.translator.bytecodes.Field f = + new com.codename1.tools.translator.bytecodes.Field( + Opcodes.PUTFIELD, "MyClass", "myField", desc); + StringBuilder out = new StringBuilder(); + f.appendInstruction(out); + String c1 = out.toString(); + assertTrue(c1.contains(expectedPeek), + "PUTFIELD desc=" + desc + " should use " + expectedPeek + + " but emitted:\n" + c1); + assertTrue(c1.contains("PEEK_OBJ(2)"), + "PUTFIELD desc=" + desc + " should read target with PEEK_OBJ(2):\n" + c1); + // Explicit pop must follow (either SP -= 2 or POP_MANY). + assertTrue(c1.contains("SP -= 2") || c1.contains("POP_MANY"), + "PUTFIELD desc=" + desc + " should pop the consumed slots explicitly:\n" + c1); + // Most importantly: must NOT use POP_X() inside the call where + // argument evaluation order would matter. + assertFalse(c1.contains("POP_DOUBLE(), POP_OBJ()"), + "PUTFIELD double must not rely on C argument evaluation order:\n" + c1); + assertFalse(c1.contains("POP_FLOAT(), POP_OBJ()"), + "PUTFIELD float must not rely on C argument evaluation order:\n" + c1); + assertFalse(c1.contains("POP_LONG(), POP_OBJ()"), + "PUTFIELD long must not rely on C argument evaluation order:\n" + c1); + assertFalse(c1.contains("POP_INT(), POP_OBJ()"), + "PUTFIELD int must not rely on C argument evaluation order:\n" + c1); + } + + // Object PUTFIELD already used PEEK + POP_MANY; cover it too so a + // future change can't silently regress it back to POP. + com.codename1.tools.translator.bytecodes.Field objField = + new com.codename1.tools.translator.bytecodes.Field( + Opcodes.PUTFIELD, "MyClass", "myField", "Ljava/lang/Object;"); + StringBuilder objOut = new StringBuilder(); + objField.appendInstruction(objOut); + String objC = objOut.toString(); + assertTrue(objC.contains("PEEK_OBJ(1)") && objC.contains("PEEK_OBJ(2)"), + "Object PUTFIELD must read both operands with PEEK:\n" + objC); + assertFalse(objC.contains("POP_OBJ(), POP_OBJ()"), + "Object PUTFIELD must not rely on C argument evaluation order:\n" + objC); + } + @Test void testArithmeticExpressionCoverage() { // Use tryReduce to construct an ArithmeticExpression since constructor is private