Browse Source

Script: Always dup new objects (#70479)

A script that creates a new object but doesn't use it,
such as by assigning it to a variable, would fail to compile
with an `ArrayIndexOutOfBoundsException`.

`new ArrayList(); return 1;` is an example that
currently fails.

This is because we do not `dup` the result of
`new` if the new object if the object is unread.

This is a problem because we always `pop` the 
operand stack at the end of a statement (see ASM 
phase's `visitStatement`).  The number of `pop`s 
depend on the type of expression in the statement.

A new object needs to be `pop`ed once.
However, the operand stack is empty if the
new object is not read in the statement.

This changes always `dup`s the result of `new` 
so `visitStatement` has something to `pop`.

Fixes: #70478
Stuart Tettemer 4 years ago
parent
commit
d599adc8a8

+ 2 - 4
modules/lang-painless/src/main/java/org/elasticsearch/painless/phase/DefaultIRTreeToASMBytesPhase.java

@@ -98,7 +98,6 @@ import org.elasticsearch.painless.symbol.FunctionTable.LocalFunction;
 import org.elasticsearch.painless.symbol.IRDecorations.IRCAllEscape;
 import org.elasticsearch.painless.symbol.IRDecorations.IRCContinuous;
 import org.elasticsearch.painless.symbol.IRDecorations.IRCInitialize;
-import org.elasticsearch.painless.symbol.IRDecorations.IRCRead;
 import org.elasticsearch.painless.symbol.IRDecorations.IRCStatic;
 import org.elasticsearch.painless.symbol.IRDecorations.IRCSynthetic;
 import org.elasticsearch.painless.symbol.IRDecorations.IRCVarArgs;
@@ -1181,9 +1180,8 @@ public class DefaultIRTreeToASMBytesPhase implements IRTreeVisitor<WriteScope> {
 
         methodWriter.newInstance(MethodWriter.getType(irNewObjectNode.getDecorationValue(IRDExpressionType.class)));
 
-        if (irNewObjectNode.hasCondition(IRCRead.class)) {
-            methodWriter.dup();
-        }
+        // Always dup so that visitStatementExpression's always has something to pop
+        methodWriter.dup();
 
         for (ExpressionNode irArgumentNode : irNewObjectNode.getArgumentNodes()) {
             visit(irArgumentNode, writeScope);

+ 18 - 0
modules/lang-painless/src/test/java/org/elasticsearch/painless/StatementTests.java

@@ -0,0 +1,18 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+package org.elasticsearch.painless;
+
+public class StatementTests extends ScriptTestCase {
+    // Ensure that new object creation without a read does not fail.
+    public void testMethodDup() {
+        assertEquals(1, exec("int i = 1; new ArrayList(new HashSet()); return i;"));
+        assertEquals(1, exec("new HashSet(); return 1;"));
+        assertEquals(1, exec("void foo() { new HashMap(); new ArrayList(); } return 1;"));
+    }
+}