changeset 442:5cdf4352ee0b

8020463: Input argument array wrapping in loadWithNewGlobal is wrong Reviewed-by: attila, jlaskey
author sundar
date Fri, 12 Jul 2013 20:06:41 +0530
parents e27ebcfed6fa
children cbfeffbcd3f2 973d78ee0728
files src/jdk/nashorn/api/scripting/ScriptObjectMirror.java src/jdk/nashorn/internal/runtime/Context.java test/script/basic/JDK-8020463.js test/src/jdk/nashorn/api/scripting/ScriptEngineSecurityTest.java test/src/jdk/nashorn/api/scripting/ScriptEngineTest.java
diffstat 5 files changed, 168 insertions(+), 96 deletions(-) [+]
line wrap: on
line diff
--- a/src/jdk/nashorn/api/scripting/ScriptObjectMirror.java	Fri Jul 12 11:58:42 2013 +0200
+++ b/src/jdk/nashorn/api/scripting/ScriptObjectMirror.java	Fri Jul 12 20:06:41 2013 +0530
@@ -52,11 +52,6 @@
     private final ScriptObject sobj;
     private final ScriptObject global;
 
-    ScriptObjectMirror(final ScriptObject sobj, final ScriptObject global) {
-        this.sobj = sobj;
-        this.global = global;
-    }
-
     @Override
     public boolean equals(final Object other) {
         if (other instanceof ScriptObjectMirror) {
@@ -81,25 +76,6 @@
         });
     }
 
-    private <V> V inGlobal(final Callable<V> callable) {
-        final ScriptObject oldGlobal = NashornScriptEngine.getNashornGlobal();
-        final boolean globalChanged = (oldGlobal != global);
-        if (globalChanged) {
-            NashornScriptEngine.setNashornGlobal(global);
-        }
-        try {
-            return callable.call();
-        } catch (final RuntimeException e) {
-            throw e;
-        } catch (final Exception e) {
-            throw new AssertionError("Cannot happen", e);
-        } finally {
-            if (globalChanged) {
-                NashornScriptEngine.setNashornGlobal(oldGlobal);
-            }
-        }
-    }
-
     // JSObject methods
     @Override
     public Object call(final String functionName, final Object... args) {
@@ -212,6 +188,8 @@
         });
     }
 
+    // javax.script.Bindings methods
+
     @Override
     public void clear() {
         inGlobal(new Callable<Object>() {
@@ -379,7 +357,7 @@
     public Object getProto() {
         return inGlobal(new Callable<Object>() {
             @Override public Object call() {
-                return wrap(getScriptObject().getProto(), global);
+                return wrap(sobj.getProto(), global);
             }
         });
     }
@@ -395,7 +373,7 @@
     public Object getOwnPropertyDescriptor(final String key) {
         return inGlobal(new Callable<Object>() {
             @Override public Object call() {
-                return wrap(getScriptObject().getOwnPropertyDescriptor(key), global);
+                return wrap(sobj.getOwnPropertyDescriptor(key), global);
             }
         });
     }
@@ -409,7 +387,7 @@
     public String[] getOwnKeys(final boolean all) {
         return inGlobal(new Callable<String[]>() {
             @Override public String[] call() {
-                return getScriptObject().getOwnKeys(all);
+                return sobj.getOwnKeys(all);
             }
         });
     }
@@ -422,7 +400,7 @@
     public ScriptObjectMirror preventExtensions() {
         return inGlobal(new Callable<ScriptObjectMirror>() {
             @Override public ScriptObjectMirror call() {
-                getScriptObject().preventExtensions();
+                sobj.preventExtensions();
                 return ScriptObjectMirror.this;
             }
         });
@@ -435,7 +413,7 @@
     public boolean isExtensible() {
         return inGlobal(new Callable<Boolean>() {
             @Override public Boolean call() {
-                return getScriptObject().isExtensible();
+                return sobj.isExtensible();
             }
         });
     }
@@ -447,7 +425,7 @@
     public ScriptObjectMirror seal() {
         return inGlobal(new Callable<ScriptObjectMirror>() {
             @Override public ScriptObjectMirror call() {
-                getScriptObject().seal();
+                sobj.seal();
                 return ScriptObjectMirror.this;
             }
         });
@@ -460,7 +438,7 @@
     public boolean isSealed() {
         return inGlobal(new Callable<Boolean>() {
             @Override public Boolean call() {
-                return getScriptObject().isSealed();
+                return sobj.isSealed();
             }
         });
     }
@@ -472,7 +450,7 @@
     public ScriptObjectMirror freeze() {
         return inGlobal(new Callable<ScriptObjectMirror>() {
             @Override public ScriptObjectMirror call() {
-                getScriptObject().freeze();
+                sobj.freeze();
                 return ScriptObjectMirror.this;
             }
         });
@@ -485,7 +463,7 @@
     public boolean isFrozen() {
         return inGlobal(new Callable<Boolean>() {
             @Override public Boolean call() {
-                return getScriptObject().isFrozen();
+                return sobj.isFrozen();
             }
         });
     }
@@ -507,12 +485,39 @@
 
         return inGlobal(new Callable<Boolean>() {
             @Override public Boolean call() {
-                return getScriptObject().isInstance(instance.getScriptObject());
+                return sobj.isInstance(instance.sobj);
             }
         });
     }
 
     /**
+     * is this a function object?
+     *
+     * @return if this mirror wraps a ECMAScript function instance
+     */
+    public boolean isFunction() {
+        return sobj instanceof ScriptFunction;
+    }
+
+    /**
+     * is this a 'use strict' function object?
+     *
+     * @return true if this mirror represents a ECMAScript 'use strict' function
+     */
+    public boolean isStrictFunction() {
+        return isFunction() && ((ScriptFunction)sobj).isStrict();
+    }
+
+    /**
+     * is this an array object?
+     *
+     * @return if this mirror wraps a ECMAScript array object
+     */
+    public boolean isArray() {
+        return sobj.isArray();
+    }
+
+    /**
      * Utility to check if given object is ECMAScript undefined value
      *
      * @param obj object to check
@@ -523,35 +528,6 @@
     }
 
     /**
-     * is this a function object?
-     *
-     * @return if this mirror wraps a ECMAScript function instance
-     */
-    public boolean isFunction() {
-        return getScriptObject() instanceof ScriptFunction;
-    }
-
-    /**
-     * is this a 'use strict' function object?
-     *
-     * @return true if this mirror represents a ECMAScript 'use strict' function
-     */
-    public boolean isStrictFunction() {
-        return isFunction() && ((ScriptFunction)getScriptObject()).isStrict();
-    }
-
-    /**
-     * is this an array object?
-     *
-     * @return if this mirror wraps a ECMAScript array object
-     */
-    public boolean isArray() {
-        return getScriptObject().isArray();
-    }
-
-    // These are public only so that Context can access these.
-
-    /**
      * Make a script object mirror on given object if needed.
      *
      * @param obj object to be wrapped
@@ -621,6 +597,12 @@
     }
 
     // package-privates below this.
+
+    ScriptObjectMirror(final ScriptObject sobj, final ScriptObject global) {
+        this.sobj = sobj;
+        this.global = global;
+    }
+
     ScriptObject getScriptObject() {
         return sobj;
     }
@@ -628,4 +610,25 @@
     static Object translateUndefined(Object obj) {
         return (obj == ScriptRuntime.UNDEFINED)? null : obj;
     }
+
+    // internals only below this.
+    private <V> V inGlobal(final Callable<V> callable) {
+        final ScriptObject oldGlobal = NashornScriptEngine.getNashornGlobal();
+        final boolean globalChanged = (oldGlobal != global);
+        if (globalChanged) {
+            NashornScriptEngine.setNashornGlobal(global);
+        }
+        try {
+            return callable.call();
+        } catch (final RuntimeException e) {
+            throw e;
+        } catch (final Exception e) {
+            throw new AssertionError("Cannot happen", e);
+        } finally {
+            if (globalChanged) {
+                NashornScriptEngine.setNashornGlobal(oldGlobal);
+            }
+        }
+    }
+
 }
--- a/src/jdk/nashorn/internal/runtime/Context.java	Fri Jul 12 11:58:42 2013 +0200
+++ b/src/jdk/nashorn/internal/runtime/Context.java	Fri Jul 12 20:06:41 2013 +0530
@@ -526,11 +526,13 @@
         });
         setGlobalTrusted(newGlobal);
 
-        final Object[] wrapped = args == null? ScriptRuntime.EMPTY_ARRAY :  ScriptObjectMirror.wrapArray(args, newGlobal);
+        final Object[] wrapped = args == null? ScriptRuntime.EMPTY_ARRAY :  ScriptObjectMirror.wrapArray(args, oldGlobal);
         newGlobal.put("arguments", ((GlobalObject)newGlobal).wrapAsObject(wrapped));
 
         try {
-            return ScriptObjectMirror.wrap(load(newGlobal, from), newGlobal);
+            // wrap objects from newGlobal's world as mirrors - but if result
+            // is from oldGlobal's world, unwrap it!
+            return ScriptObjectMirror.unwrap(ScriptObjectMirror.wrap(load(newGlobal, from), newGlobal), oldGlobal);
         } finally {
             setGlobalTrusted(oldGlobal);
         }
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/script/basic/JDK-8020463.js	Fri Jul 12 20:06:41 2013 +0530
@@ -0,0 +1,54 @@
+/*
+ * 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-8020463: Input argument array wrapping in loadWithNewGlobal is wrong
+ *
+ * @test
+ * @run
+ */
+
+loadWithNewGlobal({
+    name: "test",
+    script: "arguments[0]();"
+}, func);
+
+function func() {
+    try {
+        foo;
+    } catch (e) {
+        if (! (e instanceof ReferenceError)) {
+            fail("FAILED: expected ReferenceError!");
+        }
+    }
+}
+
+// identity
+var result = loadWithNewGlobal({
+    name: "test2",
+    script: "arguments[0]"
+}, this);
+
+if (result !== this) {
+    fail("FAILED: expected global to be returned 'as is'");
+}
--- a/test/src/jdk/nashorn/api/scripting/ScriptEngineSecurityTest.java	Fri Jul 12 11:58:42 2013 +0200
+++ b/test/src/jdk/nashorn/api/scripting/ScriptEngineSecurityTest.java	Fri Jul 12 20:06:41 2013 +0530
@@ -27,7 +27,10 @@
 
 import static org.testng.Assert.fail;
 
+import java.util.Objects;
+import javax.script.Invocable;
 import javax.script.ScriptEngine;
+import javax.script.ScriptException;
 import javax.script.ScriptEngineManager;
 import org.testng.annotations.Test;
 
@@ -44,6 +47,7 @@
     public void securityPackagesTest() {
         if (System.getSecurityManager() == null) {
             // pass vacuously
+            return;
         }
 
         final ScriptEngineManager m = new ScriptEngineManager();
@@ -64,6 +68,7 @@
     public void securityJavaTypeTest() {
         if (System.getSecurityManager() == null) {
             // pass vacuously
+            return;
         }
 
         final ScriptEngineManager m = new ScriptEngineManager();
@@ -84,6 +89,7 @@
     public void securityClassForNameTest() {
         if (System.getSecurityManager() == null) {
             // pass vacuously
+            return;
         }
 
         final ScriptEngineManager m = new ScriptEngineManager();
@@ -104,6 +110,7 @@
     public void securitySystemExit() {
         if (System.getSecurityManager() == null) {
             // pass vacuously
+            return;
         }
 
         final ScriptEngineManager m = new ScriptEngineManager();
@@ -124,6 +131,7 @@
     public void securitySystemLoadLibrary() {
         if (System.getSecurityManager() == null) {
             // pass vacuously
+            return;
         }
 
         final ScriptEngineManager m = new ScriptEngineManager();
@@ -139,4 +147,40 @@
             }
         }
     }
+
+    @Test
+    /**
+     * Check that script can't implement sensitive package interfaces.
+     */
+    public void checkSensitiveInterfaceImplTest() throws ScriptException {
+        if (System.getSecurityManager() == null) {
+            // pass vacuously
+            return;
+        }
+
+        final ScriptEngineManager m = new ScriptEngineManager();
+        final ScriptEngine e = m.getEngineByName("nashorn");
+        final Object[] holder = new Object[1];
+        e.put("holder", holder);
+        // put an empty script object into array
+        e.eval("holder[0] = {}");
+        // holder[0] is an object of some subclass of ScriptObject
+        Class ScriptObjectClass = holder[0].getClass().getSuperclass();
+        Class PropertyAccessClass = ScriptObjectClass.getInterfaces()[0];
+        // implementation methods for PropertyAccess class
+        e.eval("function set() {}; function get() {}; function getInt(){} " +
+               "function getDouble(){}; function getLong() {}; " +
+               "this.delete = function () {}; function has() {}; " +
+               "function hasOwnProperty() {}");
+
+        // get implementation of a restricted package interface
+        try {
+            log(Objects.toString(((Invocable)e).getInterface((Class<?>)PropertyAccessClass)));
+            fail("should have thrown SecurityException");
+        } catch (final Exception exp) {
+            if (! (exp instanceof SecurityException)) {
+                fail("SecurityException expected, got " + exp);
+            }
+        }
+    }
 }
--- a/test/src/jdk/nashorn/api/scripting/ScriptEngineTest.java	Fri Jul 12 11:58:42 2013 +0200
+++ b/test/src/jdk/nashorn/api/scripting/ScriptEngineTest.java	Fri Jul 12 20:06:41 2013 +0530
@@ -945,35 +945,4 @@
         Assert.assertEquals(itf.test1(42, "a", "b"), "i == 42, strings instanceof java.lang.String[] == true, strings == [a, b]");
         Assert.assertEquals(itf.test2(44, "c", "d", "e"), "arguments[0] == 44, arguments[1] instanceof java.lang.String[] == true, arguments[1] == [c, d, e]");
     }
-
-    @Test
-    /**
-     * Check that script can't implement sensitive package interfaces.
-     */
-    public void checkSensitiveInterfaceImplTest() throws ScriptException {
-        final ScriptEngineManager m = new ScriptEngineManager();
-        final ScriptEngine e = m.getEngineByName("nashorn");
-        final Object[] holder = new Object[1];
-        e.put("holder", holder);
-        // put an empty script object into array
-        e.eval("holder[0] = {}");
-        // holder[0] is an object of some subclass of ScriptObject
-        Class ScriptObjectClass = holder[0].getClass().getSuperclass();
-        Class PropertyAccessClass = ScriptObjectClass.getInterfaces()[0];
-        // implementation methods for PropertyAccess class
-        e.eval("function set() {}; function get() {}; function getInt(){} " +
-               "function getDouble(){}; function getLong() {}; " +
-               "this.delete = function () {}; function has() {}; " +
-               "function hasOwnProperty() {}");
-
-        // get implementation of a restricted package interface
-        try {
-            log(Objects.toString(((Invocable)e).getInterface((Class<?>)PropertyAccessClass)));
-            fail("should have thrown SecurityException");
-        } catch (final Exception exp) {
-            if (! (exp instanceof SecurityException)) {
-                fail("SecurityException expected, got " + exp);
-            }
-        }
-    }
 }