8016235: Use in catch block that may not have been executed in try block caused illegal byte code to be generated
Reviewed-by: jlaskey, hannesw
--- a/nashorn/src/jdk/nashorn/internal/codegen/Attr.java Thu Jun 13 20:50:24 2013 +0200
+++ b/nashorn/src/jdk/nashorn/internal/codegen/Attr.java Fri Jun 14 13:53:40 2013 +0200
@@ -35,6 +35,7 @@
import static jdk.nashorn.internal.codegen.CompilerConstants.SWITCH_TAG_PREFIX;
import static jdk.nashorn.internal.codegen.CompilerConstants.THIS;
import static jdk.nashorn.internal.codegen.CompilerConstants.VARARGS;
+import static jdk.nashorn.internal.ir.Symbol.IS_ALWAYS_DEFINED;
import static jdk.nashorn.internal.ir.Symbol.IS_CONSTANT;
import static jdk.nashorn.internal.ir.Symbol.IS_FUNCTION_SELF;
import static jdk.nashorn.internal.ir.Symbol.IS_GLOBAL;
@@ -128,6 +129,8 @@
private final Deque<Type> returnTypes;
+ private int catchNestingLevel;
+
private static final DebugLogger LOG = new DebugLogger("attr");
private static final boolean DEBUG = LOG.isEnabled();
@@ -169,14 +172,14 @@
if (functionNode.isVarArg()) {
initCompileConstant(VARARGS, body, IS_PARAM | IS_INTERNAL, Type.OBJECT_ARRAY);
if (functionNode.needsArguments()) {
- initCompileConstant(ARGUMENTS, body, IS_VAR | IS_INTERNAL, Type.typeFor(ScriptObject.class));
+ initCompileConstant(ARGUMENTS, body, IS_VAR | IS_INTERNAL | IS_ALWAYS_DEFINED, Type.typeFor(ScriptObject.class));
addLocalDef(ARGUMENTS.symbolName());
}
}
initParameters(functionNode, body);
- initCompileConstant(SCOPE, body, IS_VAR | IS_INTERNAL, Type.typeFor(ScriptObject.class));
- initCompileConstant(RETURN, body, IS_VAR | IS_INTERNAL, Type.OBJECT);
+ initCompileConstant(SCOPE, body, IS_VAR | IS_INTERNAL | IS_ALWAYS_DEFINED, Type.typeFor(ScriptObject.class));
+ initCompileConstant(RETURN, body, IS_VAR | IS_INTERNAL | IS_ALWAYS_DEFINED, Type.OBJECT);
}
@@ -320,10 +323,12 @@
final Block block = lc.getCurrentBlock();
start(catchNode);
+ catchNestingLevel++;
// define block-local exception variable
- final Symbol def = defineSymbol(block, exception.getName(), IS_VAR | IS_LET);
- newType(def, Type.OBJECT);
+ final Symbol def = defineSymbol(block, exception.getName(), IS_VAR | IS_LET | IS_ALWAYS_DEFINED);
+ newType(def, Type.OBJECT); //we can catch anything, not just ecma exceptions
+
addLocalDef(exception.getName());
return true;
@@ -334,6 +339,9 @@
final IdentNode exception = catchNode.getException();
final Block block = lc.getCurrentBlock();
final Symbol symbol = findSymbol(block, exception.getName());
+
+ catchNestingLevel--;
+
assert symbol != null;
return end(catchNode.setException((IdentNode)exception.setSymbol(lc, symbol)));
}
@@ -543,9 +551,19 @@
assert lc.getFunctionBody(functionNode).getExistingSymbol(CALLEE.symbolName()) != null;
lc.setFlag(functionNode.getBody(), Block.NEEDS_SELF_SYMBOL);
newType(symbol, FunctionNode.FUNCTION_TYPE);
- } else if (!identNode.isInitializedHere()) { // NASHORN-448
- // here is a use outside the local def scope
- if (!isLocalDef(name)) {
+ } else if (!identNode.isInitializedHere()) {
+ /*
+ * See NASHORN-448, JDK-8016235
+ *
+ * Here is a use outside the local def scope
+ * the inCatch check is a conservative approach to handle things that might have only been
+ * defined in the try block, but with variable declarations, which due to JavaScript rules
+ * have to be lifted up into the function scope outside the try block anyway, but as the
+ * flow can fault at almost any place in the try block and get us to the catch block, all we
+ * know is that we have a declaration, not a definition. This can be made better and less
+ * conservative once we superimpose a CFG onto the AST.
+ */
+ if (!isLocalDef(name) || inCatch()) {
newType(symbol, Type.OBJECT);
symbol.setCanBeUndefined();
}
@@ -572,6 +590,10 @@
return end(identNode.setSymbol(lc, symbol));
}
+ private boolean inCatch() {
+ return catchNestingLevel > 0;
+ }
+
/**
* If the symbol isn't already a scope symbol, and it is either not local to the current function, or it is being
* referenced from within a with block, we force it to be a scope symbol.
@@ -584,26 +606,26 @@
}
private boolean symbolNeedsToBeScope(Symbol symbol) {
- if(symbol.isThis() || symbol.isInternal()) {
+ if (symbol.isThis() || symbol.isInternal()) {
return false;
}
boolean previousWasBlock = false;
- for(final Iterator<LexicalContextNode> it = lc.getAllNodes(); it.hasNext();) {
+ for (final Iterator<LexicalContextNode> it = lc.getAllNodes(); it.hasNext();) {
final LexicalContextNode node = it.next();
- if(node instanceof FunctionNode) {
+ if (node instanceof FunctionNode) {
// We reached the function boundary without seeing a definition for the symbol - it needs to be in
// scope.
return true;
- } else if(node instanceof WithNode) {
- if(previousWasBlock) {
+ } else if (node instanceof WithNode) {
+ if (previousWasBlock) {
// We reached a WithNode; the symbol must be scoped. Note that if the WithNode was not immediately
// preceded by a block, this means we're currently processing its expression, not its body,
// therefore it doesn't count.
return true;
}
previousWasBlock = false;
- } else if(node instanceof Block) {
- if(((Block)node).getExistingSymbol(symbol.getName()) == symbol) {
+ } else if (node instanceof Block) {
+ if (((Block)node).getExistingSymbol(symbol.getName()) == symbol) {
// We reached the block that defines the symbol without reaching either the function boundary, or a
// WithNode. The symbol need not be scoped.
return false;
@@ -1700,8 +1722,8 @@
}
private void pushLocalsBlock() {
- localDefs.push(localDefs.isEmpty() ? new HashSet<String>() : new HashSet<>(localDefs.peek()));
- localUses.push(localUses.isEmpty() ? new HashSet<String>() : new HashSet<>(localUses.peek()));
+ localDefs.push(new HashSet<>(localDefs.peek()));
+ localUses.push(new HashSet<>(localUses.peek()));
}
private void popLocals() {
--- a/nashorn/src/jdk/nashorn/internal/ir/Symbol.java Thu Jun 13 20:50:24 2013 +0200
+++ b/nashorn/src/jdk/nashorn/internal/ir/Symbol.java Fri Jun 14 13:53:40 2013 +0200
@@ -55,23 +55,25 @@
public static final int KINDMASK = (1 << 3) - 1; // Kinds are represented by lower three bits
/** Is this scope */
- public static final int IS_SCOPE = 1 << 4;
+ public static final int IS_SCOPE = 1 << 4;
/** Is this a this symbol */
- public static final int IS_THIS = 1 << 5;
+ public static final int IS_THIS = 1 << 5;
/** Can this symbol ever be undefined */
- public static final int CAN_BE_UNDEFINED = 1 << 6;
+ public static final int CAN_BE_UNDEFINED = 1 << 6;
+ /** Is this symbol always defined? */
+ public static final int IS_ALWAYS_DEFINED = 1 << 8;
/** Can this symbol ever have primitive types */
- public static final int CAN_BE_PRIMITIVE = 1 << 7;
+ public static final int CAN_BE_PRIMITIVE = 1 << 9;
/** Is this a let */
- public static final int IS_LET = 1 << 8;
+ public static final int IS_LET = 1 << 10;
/** Is this an internal symbol, never represented explicitly in source code */
- public static final int IS_INTERNAL = 1 << 9;
+ public static final int IS_INTERNAL = 1 << 11;
/** Is this a function self-reference symbol */
- public static final int IS_FUNCTION_SELF = 1 << 10;
+ public static final int IS_FUNCTION_SELF = 1 << 12;
/** Is this a specialized param? */
- public static final int IS_SPECIALIZED_PARAM = 1 << 11;
+ public static final int IS_SPECIALIZED_PARAM = 1 << 13;
/** Is this symbol a shared temporary? */
- public static final int IS_SHARED = 1 << 12;
+ public static final int IS_SHARED = 1 << 14;
/** Null or name identifying symbol. */
private final String name;
@@ -384,7 +386,7 @@
* Mark this symbol as one being shared by multiple expressions. The symbol must be a temporary.
*/
public void setIsShared() {
- if(!isShared()) {
+ if (!isShared()) {
assert isTemp();
trace("SET IS SHARED");
flags |= IS_SHARED;
@@ -417,6 +419,14 @@
}
/**
+ * Check if this symbol is always defined, which overrides all canBeUndefined tags
+ * @return true if always defined
+ */
+ public boolean isAlwaysDefined() {
+ return isParam() || (flags & IS_ALWAYS_DEFINED) == IS_ALWAYS_DEFINED;
+ }
+
+ /**
* Get the range for this symbol
* @return range for symbol
*/
@@ -462,7 +472,9 @@
*/
public void setCanBeUndefined() {
assert type.isObject() : type;
- if (!isParam() && !canBeUndefined()) {//parameters are never undefined
+ if (isAlwaysDefined()) {
+ return;
+ } else if (!canBeUndefined()) {
assert !isShared();
flags |= CAN_BE_UNDEFINED;
}
--- a/nashorn/src/jdk/nashorn/internal/parser/JSONParser.java Thu Jun 13 20:50:24 2013 +0200
+++ b/nashorn/src/jdk/nashorn/internal/parser/JSONParser.java Fri Jun 14 13:53:40 2013 +0200
@@ -149,9 +149,9 @@
@Override
protected void scanNumber() {
// Record beginning of number.
- final int start = position;
+ final int startPosition = position;
// Assume value is a decimal.
- TokenType type = TokenType.DECIMAL;
+ TokenType valueType = TokenType.DECIMAL;
// floating point can't start with a "." with no leading digit before
if (ch0 == '.') {
@@ -211,11 +211,11 @@
}
}
- type = TokenType.FLOATING;
+ valueType = TokenType.FLOATING;
}
// Add number token.
- add(type, start);
+ add(valueType, startPosition);
}
// ECMA 15.12.1.1 The JSON Lexical Grammar - JSONEscapeCharacter
--- a/nashorn/src/jdk/nashorn/internal/parser/Lexer.java Thu Jun 13 20:50:24 2013 +0200
+++ b/nashorn/src/jdk/nashorn/internal/parser/Lexer.java Fri Jun 14 13:53:40 2013 +0200
@@ -919,6 +919,7 @@
/**
* Scan over a string literal.
+ * @param add true if we nare not just scanning but should actually modify the token stream
*/
protected void scanString(final boolean add) {
// Type of string.
@@ -1614,6 +1615,12 @@
return null;
}
+ /**
+ * Get the correctly localized error message for a given message id format arguments
+ * @param msgId message id
+ * @param args format arguments
+ * @return message
+ */
protected static String message(final String msgId, final String... args) {
return ECMAErrors.getMessage("lexer.error." + msgId, args);
}
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/nashorn/test/script/basic/JDK-8016235.js Fri Jun 14 13:53:40 2013 +0200
@@ -0,0 +1,51 @@
+/*
+ * Copyright (c) 2010, 2013, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/**
+ * JDK-8016235 : use before definition in catch block generated erroneous bytecode
+ * as there is no guarantee anything in the try block has executed.
+ *
+ * @test
+ * @run
+ */
+
+function f() {
+ try {
+ var parser = {};
+ } catch (e) {
+ parser = parser.context();
+ }
+}
+
+function g() {
+ try {
+ return "apa";
+ } catch (tmp) {
+ //for now, too conservative as var ex declaration exists on the function
+ //level, but at least the code does not break, and the analysis is driven
+ //from the catch block (the rare case), not the try block (the common case)
+ var ex = new Error("DOM Exception 5");
+ ex.code = ex.number = 5;
+ return ex;
+ }
+}