[rfc][icedtea-web] Fix for PR1198, JSObject passed incorrectly to Javascript
Jiri Vanek
jvanek at redhat.com
Fri Jan 4 01:43:20 PST 2013
On 12/03/2012 08:03 PM, Adam Domurad wrote:
> On 11/30/2012 04:08 PM, Adam Domurad wrote:
>> Hi all. Attached is a fix for PR1198.
>>
>> I still plan to write unit tests for at least the new method PluginAppletViewer.toObjectIDString, but I wanted to get this out before leaving for the weekend.
>>
>> One tricky thing with this patch was that it had to consolidate _a lot_ of duplicated functionality (actually I found some subtle differences in handling, this should be more consistent). Once that was done the actual patch was fairly straight forward.
>>
>> The basic issue was that JSObject was being passed as if it were a normal Java object, when the liveconnect spec specifies that it should be returned as the underlying javascript object.
>>
>> A method was added to JSObject to get the underlying reference. This was kept package-private to not pollute its public interface. As well, a new permission was added for accessing this method. To access this outside of the package, a utility was created in JSUtil.
>>
>> This patch adds a special case to Java->JS communication when sending objects, to indicate that a Javascript object is being passed.
>>
>> With patch applied, JSObjectFromEval should pass in all browsers.
>>
>> 2012-11-30 Adam Domurad <adomurad at redhat.com>
>>
>> Fix PR1198: JSObject passed incorrectly to Javascript
>> * plugin/icedteanp/IcedTeaJavaRequestProcessor.cc: Pass extra data for
>> 'jsobject' object result messages.
>> * plugin/icedteanp/IcedTeaPluginRequestProcessor.cc: Same.
>> * plugin/icedteanp/IcedTeaPluginUtils.cc: Add special casing of
>> javascript references passed from java.
>> * plugin/icedteanp/java/netscape/javascript/JSObjectUnboxPermission.java:
>> New permission for unboxing a JSObject's internal reference.
>> * plugin/icedteanp/java/netscape/javascript/JSObject.java
>> (getInternalReference): New, package-private, retrieves internal
>> reference (Must have proper permission).
>> * plugin/icedteanp/java/netscape/javascript/JSUtil.java
>> (getJSObjectInternalReference) New, utility for accessing
>> JSObject#getInternalReference from outside the package.
>> * plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java:
>> (toObjectIDString): New, creates a string that precisely identifies a
>> Java object.
>> (handleMessage): Replace a lot of duplicated functionality with
>> 'toObjectIDString'.
>> * plugin/icedteanp/java/sun/applet/PluginAppletViewer.java: Replace
>> duplicated functionality with 'toObjectIDString'.
>> * tests/reproducers/simple/JSObjectFromEval/srcs/JSObjectFromEval.java:
>> Don't print out type passed (differs from browser to browser).
>> * tests/reproducers/simple/JSObjectFromEval/testcases/JSObjectFromEvalTest.java:
>> Don't check type passed (differs from browser to browser). Remove
>> known-to-fail. Reformat.
>
> As promised attached is the unit-test for the newly introduced Java->JS function (which encapsulates logic that was duplicated in many areas)
>
> Note that this patch contains a Makefile patch for unit testing sun.applet package, however this will be committed with another now-approved patch so I have left it out of the ChangeLog.
>
> ChangeLog:
> 2012-12-XX Adam Domurad <adomurad at redhat.com>
>
> Unit test for PluginAppletSecurityContext#toObjectIDString. Make
> PluginAppletSecurityContext more unit-testable.
> * plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java:
> Don't initialize security manager in constructor. Fix a few Java->JS
> corner cases.
> * plugin/icedteanp/java/sun/applet/PluginMain.java: Initialize
> SecurityManager before creating PluginAppletSecurityContext.
> * tests/netx/unit/sun/applet/PluginAppletSecurityContextTest.java:
> Unit test for all the corner cases of converting a Java object to a
> string that can be precisely identified.
>
> Happy hacking,
> -Adam
>
>
Tests first. A lot of questions....
> jsfix-unittest.patch
>
>
> diff --git a/Makefile.am b/Makefile.am
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -969,7 +969,7 @@ stamps/netx-unit-tests-compile.stamp: st
> mkdir -p $(NETX_UNIT_TEST_DIR)&& \
> $(BOOT_DIR)/bin/javac $(IT_JAVACFLAGS) \
> -d $(NETX_UNIT_TEST_DIR) \
> - -classpath $(JUNIT_JAR):$(NETX_DIR)/lib/classes.jar:$(TEST_EXTENSIONS_DIR) \
> + -classpath $(JUNIT_JAR):$(DESTDIR)$(datadir)/$(PACKAGE_NAME)/plugin.jar:$(NETX_DIR)/lib/classes.jar:$(TEST_EXTENSIONS_DIR) \
> @netx-unit-tests-source-files.txt&& \
> mkdir -p stamps&& \
> touch $@
> @@ -999,7 +999,7 @@ stamps/run-netx-unit-tests.stamp: stamps
> done ; \
> cd $(NETX_UNIT_TEST_DIR) ; \
> class_names=`cat $(UNIT_CLASS_NAMES)` ; \
> - CLASSPATH=$(NETX_DIR)/lib/classes.jar:$(JUNIT_JAR):$(JUNIT_RUNNER_JAR):$(TEST_EXTENSIONS_DIR):. \
> + CLASSPATH=$(NETX_DIR)/lib/classes.jar:$(DESTDIR)$(datadir)/$(PACKAGE_NAME)/plugin.jar:$(JUNIT_JAR):$(JUNIT_RUNNER_JAR):$(TEST_EXTENSIONS_DIR):. \
Just for confirmation, I remember both those changes already in, little bit better (without installed path)
> $(BOOT_DIR)/bin/java -Xbootclasspath:$(RUNTIME) CommandLine $$class_names
> if WITH_XSLTPROC
> $(XSLTPROC) --stringparam logs logs_unit.html $(TESTS_SRCDIR)/$(REPORT_STYLES_DIRNAME)/jreport.xsl $(NETX_UNIT_TEST_DIR)/tests-output.xml> $(TESTS_DIR)/index_unit.html
> diff --git a/plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java b/plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java
> --- a/plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java
> +++ b/plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java
> @@ -53,7 +53,6 @@ import java.security.Permissions;
> import java.security.PrivilegedAction;
> import java.security.ProtectionDomain;
> import java.util.ArrayList;
> -import java.util.Arrays;
> import java.util.Hashtable;
> import java.util.List;
> import java.util.Map;
> @@ -241,16 +240,6 @@ public class PluginAppletSecurityContext
> public PluginAppletSecurityContext(int identifier) {
> this.identifier = identifier;
>
> - // We need a security manager.. and since there is a good chance that
> - // an applet will be loaded at some point, we should make it the SM
> - // that JNLPRuntime will try to install
> - if (System.getSecurityManager() == null) {
> - JNLPRuntime.initialize(/* isApplication */false);
> - JNLPRuntime.setDefaultLaunchHandler(new DefaultLaunchHandler(System.err));
> - }
> -
> - JNLPRuntime.disableExit();
> -
Can't the moving of this initiliazaton cause some hole?
What corner cases it fixed? Can't they be fixed by better way?
Anyway - it should be in different changeset.
> URL u = null;
> try {
> u = new URL("file://");
> @@ -348,16 +337,16 @@ public class PluginAppletSecurityContext
> */
> public String toObjectIDString(Object obj, Class<?> type, boolean unboxPrimitives) {
>
> + /* Void (can occur from declared return type), pass special "void" string: */
> + if (type == Void.TYPE) {
> + return "literalreturn void";
> + }
> +
> /* Null, pass special "null" string: */
> if (obj == null) {
> return "literalreturn null";
> }
>
> - /* Void (can occur from declared return type), pass special "void" string: */
> - if (type == Void.TYPE) {
> - return "literalreturn void";
> - }
> -
> /* Primitive, accurately represented by its toString() form: */
> boolean returnAsString = ( type == Boolean.TYPE
> || type == Byte.TYPE
> @@ -378,7 +367,7 @@ public class PluginAppletSecurityContext
>
> /* Floating point number, we ensure we give enough precision: */
> if ( type == Float.TYPE || type == Double.TYPE ||
> - ( unboxPrimitives&& (type == Double.class || type == Double.class) )) {
> + ( unboxPrimitives&& (type == Float.class || type == Double.class) )) {
Why this? Also Shouldn't it be in different changeset?
> return "literalreturn " + String.format("%308.308e", obj);
> }
>
> diff --git a/plugin/icedteanp/java/sun/applet/PluginMain.java b/plugin/icedteanp/java/sun/applet/PluginMain.java
> --- a/plugin/icedteanp/java/sun/applet/PluginMain.java
> +++ b/plugin/icedteanp/java/sun/applet/PluginMain.java
> @@ -73,6 +73,7 @@ import java.net.ProxySelector;
> import java.util.Enumeration;
> import java.util.Properties;
>
> +import net.sourceforge.jnlp.DefaultLaunchHandler;
> import net.sourceforge.jnlp.config.DeploymentConfiguration;
> import net.sourceforge.jnlp.runtime.JNLPRuntime;
> import net.sourceforge.jnlp.security.JNLPAuthenticator;
> @@ -106,6 +107,15 @@ public class PluginMain {
> // must be called before JNLPRuntime.initialize()
> JNLPRuntime.setRedirectStreams(redirectStreams);
>
> + // We need a security manager for PluginAppletSecurityContext,
> + // so we ensure it is initialized
> + if (System.getSecurityManager() == null) {
> + JNLPRuntime.initialize(/* isApplication */false);
> + JNLPRuntime.setDefaultLaunchHandler(new DefaultLaunchHandler(System.err));
> + }
> +
> + JNLPRuntime.disableExit();
> +
Concerns told above.
As mentioned - there is several changes I believe belongs to different changeset. If I'm wrong feel free to correct me.
I' have cced Jana to put her voice to js<->java communicaton,
> PluginAppletSecurityContext sc = new PluginAppletSecurityContext(0);
> sc.prePopulateLCClasses();
> PluginAppletSecurityContext.setStreamhandler(streamHandler);
> diff --git a/tests/netx/unit/sun/applet/PluginAppletSecurityContextTest.java b/tests/netx/unit/sun/applet/PluginAppletSecurityContextTest.java
> new file mode 100644
> --- /dev/null
> +++ b/tests/netx/unit/sun/applet/PluginAppletSecurityContextTest.java
> @@ -0,0 +1,181 @@
> +package sun.applet;
> +
> +import static org.junit.Assert.assertTrue;
> +import static org.junit.Assert.assertFalse;
> +import static org.junit.Assert.assertEquals;
> +
> +import netscape.javascript.JSObject;
> +
> +import org.junit.Test;
> +
> +public class PluginAppletSecurityContextTest {
> +
> + @Test
> + public void toIDStringNullTest() {
> + PluginAppletSecurityContext pasc = new PluginAppletSecurityContext(0);
> + assertEquals("literalreturn null",
> + pasc.toObjectIDString(null, Object.class, false));
> + }
> +
> + @Test
> + public void toIDStringVoidTest() {
> + PluginAppletSecurityContext pasc = new PluginAppletSecurityContext(0);
> + assertEquals("literalreturn void",
> + pasc.toObjectIDString(null, Void.TYPE, false));
> +
> + assertFalse("literalreturn void".equals(pasc.toObjectIDString(null,
> + Void.class, false)));
> + }
> +
> + @Test
> + public void toIDStringIntegralTest() {
> + // NB: the special .TYPE classes here represent primitives
> + PluginAppletSecurityContext pasc = new PluginAppletSecurityContext(0);
> +
> + // Test both unboxing allowed and not allowed to be sure it doesn't
> + // alter result
> + // although it really shouldn't
> + for (boolean unboxPrimitives : new Boolean[] { false, true }) {
> + assertEquals("literalreturn true", pasc.toObjectIDString(
> + new Boolean(true), Boolean.TYPE, unboxPrimitives));
> +
> + assertEquals("literalreturn 1", pasc.toObjectIDString(new Byte(
> + (byte) 1), Byte.TYPE, unboxPrimitives));
> +
> + assertEquals("literalreturn 1", pasc.toObjectIDString(
> + new Character((char) 1), Character.TYPE, unboxPrimitives));
> +
> + assertEquals("literalreturn 1", pasc.toObjectIDString(new Short(
> + (short) 1), Short.TYPE, unboxPrimitives));
> +
> + assertEquals("literalreturn 1", pasc.toObjectIDString(
> + new Integer(1), Integer.TYPE, unboxPrimitives));
> +
> + assertEquals("literalreturn 1", pasc.toObjectIDString(new Long(1),
> + Long.TYPE, unboxPrimitives));
> + }
> + }
> +
> + @Test
> + public void toIDStringBoxedIntegralNoUnboxingTest() {
> + PluginAppletSecurityContext pasc = new PluginAppletSecurityContext(0);
> +
> + assertFalse("literalreturn true".equals(pasc.toObjectIDString(
> + new Boolean(true), Boolean.class, false)));
> +
> + assertFalse("literalreturn 1".equals(pasc.toObjectIDString(new Byte(
> + (byte) 1), Byte.class, false)));
> +
> + assertFalse("literalreturn 1".equals(pasc.toObjectIDString(
> + new Character((char) 1), Character.class, false)));
> +
> + assertFalse("literalreturn 1".equals(pasc.toObjectIDString(new Short(
> + (short) 1), Short.class, false)));
> +
> + assertFalse("literalreturn 1".equals(pasc.toObjectIDString(new Integer(
> + 1), Integer.class, false)));
> +
> + assertFalse("literalreturn 1".equals(pasc.toObjectIDString(new Long(1),
> + Long.class, false)));
> + }
> +
> + @Test
> + public void toIDStringBoxedIntegralWithUnboxingTest() {
> + PluginAppletSecurityContext pasc = new PluginAppletSecurityContext(0);
> +
> + assertEquals("literalreturn true",
> + pasc.toObjectIDString(new Boolean(true), Boolean.class, true));
> +
> + assertEquals("literalreturn 1",
> + pasc.toObjectIDString(new Byte((byte) 1), Byte.class, true));
> +
> + assertEquals("literalreturn 1", pasc.toObjectIDString(new Character(
> + (char) 1), Character.class, true));
> +
> + assertEquals("literalreturn 1",
> + pasc.toObjectIDString(new Short((short) 1), Short.class, true));
> +
> + assertEquals("literalreturn 1",
> + pasc.toObjectIDString(new Integer(1), Integer.class, true));
> +
> + assertEquals("literalreturn 1",
> + pasc.toObjectIDString(new Long(1), Long.class, true));
> + }
> +
> + @Test
> + public void toIDStringFloatingPoint() {
> + final int prefixLength = "literalreturn ".length();
> +
> + // NB: the special .TYPE classes here represent primitives
> + PluginAppletSecurityContext pasc = new PluginAppletSecurityContext(0);
> +
> + // Test both unboxing allowed and not allowed to be sure it doesn't
> + // alter result
> + // although it really shouldn't
> + for (boolean unboxPrimitives : new Boolean[] { false, true }) {
> + {
> + final float testFloat = 3.141592f;
> + String idString = pasc.toObjectIDString(new Float(testFloat),
> + Float.TYPE, unboxPrimitives);
> + String floatRepr = idString.substring(prefixLength);
> + assertTrue(testFloat == Float.parseFloat(floatRepr));
> + }
> + {
> + final double testDouble = 3.141592;
> + String idString = pasc.toObjectIDString(new Double(testDouble),
> + Double.TYPE, unboxPrimitives);
> + String doubleRepr = idString.substring(prefixLength);
> + assertTrue(testDouble == Double.parseDouble(doubleRepr));
> + }
> +
> + }
> + {
> + final float testFloat = 3.141592f;
> + String idString = pasc.toObjectIDString(new Float(testFloat),
> + Float.class, true);
> + String floatRepr = idString.substring(prefixLength);
> + assertTrue(testFloat == Float.parseFloat(floatRepr));
> + }
> + {
> + final double testDouble = 3.141592;
> + String idString = pasc.toObjectIDString(new Double(testDouble),
> + Double.class, true);
> + String doubleRepr = idString.substring(prefixLength);
> + assertTrue(testDouble == Double.parseDouble(doubleRepr));
> + }
> + {
> + final float testFloat = 3.141592f;
> + String idString = pasc.toObjectIDString(new Float(testFloat),
> + Float.class, false);
> + assertFalse(idString.startsWith("literalreturn "));
> + }
> + {
> + final double testDouble = 3.141592;
> + String idString = pasc.toObjectIDString(new Double(testDouble),
> + Double.class, false);
> + assertFalse(idString.startsWith("literalreturn "));
> + }
> + }
> +
> +// FIXME: How can we get the permissions to do this?
no idea :(
Otherwise tests looks oook. Thank you!
> +// @Test
> +// public void toIDStringJSObject() {
> +// PluginAppletSecurityContext pasc = new PluginAppletSecurityContext(0);
> +//
> +// long testReference = 1;
> +// assertEquals("literalreturn 1", pasc.toObjectIDString(new JSObject(
> +// testReference), JSObject.class, false));
> +// }
> +
> + @Test
> + public void toIDStringArbitraryObject() {
> + PluginAppletSecurityContext pasc = new PluginAppletSecurityContext(0);
> +
> + final Object testObject = new Object();
> + String idString = pasc.toObjectIDString(testObject,
> + testObject.getClass(), false);
> +
> + assertFalse(idString.startsWith("literalreturn"));
> + assertFalse(idString.startsWith("jsobject"));
> + }
> +}
More information about the distro-pkg-dev
mailing list