200016 - Crash accessing Java package from JS

Status: RESOLVED FIXED

(Core Graveyard :: Java: OJI, defect)

Description

[Mitchell Stoltz (not reading bugmail)]

From Bugtraq:

Hi,

executing

<scr1pt language="Javascript">
t = new Packages.sun.plugin.javascript.navig5.JSObject(1,1);
</scr1pt>

crashes Netscape 7.02 and Opera 7 on Windows XP.
The active JVM in both tested browsers is Java 1.4.1_02 from Sun.

This liveconnect (javascript-2-java-communication) stuff seems
to be still very dangerous.

Sincerely
Marc Schoenefeld

Comment 1

[Joshua Xia]

In jsj_JSObject.c

	/* JNI returns a NULL handle for a Java 'null' */
    if (!handle)
        return NULL;

    return handle->js_obj;

get a invalid handle (handle != 0) from JPI side, just like #183092

Comment 2

[Brendan Eich [:brendan]]

I'm sorry, no -- if an exception was thrown, the return value should be null. 
Anything else is a bug in the JNI, JPI, JVM interface glue, or whatever it's
called these days.

/be

Comment 3

[Joshua Xia]

I remember that another bug is also happen on the reason. (crash because of
handle->js_obj;)

Yes, I will push them (Sun 's JPI team member) to fix this on JPI side.
It is not elegant to check exception anywhere and reduce the performance.

I inserted checking exception code in this case to test, can't get exception, I
will check inside JPI.

Brendan,
Are you on IRC mozilla channel?
should we talk about this kind of bugs?

Comment 4

[Joshua Xia]

I can't get exception from JPI and the handle is also invalid,
Xiaobin:
Please check this.
Thanks!

Comment 5

[Joshua Xia]

->All OS

Comment 6

[Joshua Xia]

filed a P1 bug in Sun's bugtraq #4850496

Comment 7

[Will Levine]

this is a dupe of bug 199694  ?

Comment 8

[Joshua Xia]

*** Bug 199694 has been marked as a duplicate of this bug. ***

Comment 9

[Marc Bejarano]

that was backwards

this is the dupe.  bug 199694 is the original.

Comment 10

[Jeremias Reith]

IMHO this bug should block 1.5 since people at bugtraq start in believing that
we are not intressted in fixing criticial bugs.

Comment 11

[Brendan Eich [:brendan]]

Joshua, any hope of a fix for 1.5final?  We need it this week, if possible.

/be

Comment 12

[Joshua Xia]

This bug can't be fixed on mozilla side. It must be fixed on Java Plugin side.
I will push JPI member to fix it.

Comment 13

[Xiaobin Lu]

The problem is due to invalid cast between jint and JSObjectHandle. 

jsj_JSObject.c Ln 313:
  handle = (JSObjectHandle*)JSJCallbacks->unwrap....

It calls to lcglue.cpp Ln 400. That function returned a jint. JPI side returns 
the right value "1". 

The fix should be either JPI side or browser side correctly malloc 
JSObjectHandle and assign the returned value from callback function to handle-
>js_obj.

Comment 14

[Xiaobin Lu]

A typo in my last comment, 
The fix should be either OJI side or liveconnect side to correctly allocate 
JSObjectHandle and assign the right value to the struct.

Comment 15

[Brendan Eich [:brendan]]

Xiaobin, I am not an expert on this code, and I agree the cast is ugly, but that
doesn't seem to be the cause of the crash.  Is it not the case that every handle
returned by unwrap_java_wrapper must have previously been passed into the OJI
implementation via a get_java_wrapper call?  The only call to get_java_wrapper
that I can see is in jsj_WrapJSObject, and jsj_WrapJSObject does indeed allocate
a JSObjectHandle for the OJI to associate with a Java object.

Is the problem that some other Java object than one created as a wrapper for a
JS object is being "unwrapped"?

/be

Comment 16

[Jack Lu]

Ok, let me try to clarify.

When the user uses t = new Packages.sun.plugin.javascript.navig5.JSObject(1, 1),
what the browser really does is, it invokes invoke_java_constructor (see
jsj_method.c) and calls to Java to create a java object, in this case,
sun.plugin.javascript.navig5.JSObject. After that, it calls
jsj_ConvertJavaObjectToJSValue (jsj_convert.c, ln 820) to convert the java
object to a js value, the logic of that function is if the passed in java object
is a jsobject (java type), it will just call jsj_
UnwrapJSObjectWrapper(jsj_JSObject.c, ln 293) to unwrap it to get the contained
native jsobject. 

From the whole call sequence, there is no JSObjectHandle gets created. So it is
 wrong to assume unwrap_java_wrapper will return a JSObjectHandle. It actually
just retrieves the private field "nativeJSObject" which is 1 in this case.

So I guess the communication protocol of the JVM glue interface is breaking here.
 

Comment 17

[Joshua Xia]

Yes, I also checked this and found that:

Before "unwrap_java_wrapper_impl" (in lcglue.cpp) is invoked, the
"get_java_wrapper_impl" (in lcglue.cpp) should be invoked to set field
"nativeJSObject" in JSObject.class. In general, mozilla set JSObjectHandle 's
pointer as a int into JSObject.class 's "nativeJSObject" field when
"get_java_wrapper_impl", and get back JSObjectHandle 's pointer when
"unwrap_java_wrapper_impl".

But in this case,  "get_java_wrapper_impl" is not invoked before
"unwrap_java_wrapper_impl", so the JSObjectHandle that get from Java Plugin is
invalid (which is 0x1). it should be JSObjectHandle 's pointer address, and
JSObjectHandle->js_obj = 1.

Xiaobin,
In this testcase: t = new Packages.sun.plugin.javascript.navig5.JSObject(1, 1);
did this "JSObject" conflict with JPI 's "working" JSObject?

or why did JSObject in JPI 's JSObjectHandle be initialized to be 0x1 ?

Comment 18

[Joshua Xia]

I modified testcase to be:

<scr1pt language="Javascript">
t = new Packages.sun.plugin.javascript.navig5.JSObject(1, 0);
</scr1pt>

the crash gone, so that the "JSObjectHandle" return from JPI is really the
second parameter.

I think this is not right. checking java_wrapper_obj.
I think this testcase is too special. it conflict JPI 's working "JSObject" with
client "JSObject"

Comment 19

[Joshua Xia]

Hi Xiaobin,

Is "netscape/javascript/JSObject" 's class (njJSObject) same to
"sun.plugin.javascript.navig5.JSObject" 's class?

Why "sun.plugin.javascript.navig5.JSObject" 's java_object isInstance of
"netscape/javascript/JSObject"?

Comment 20

[Joshua Xia]

Because of "sun.plugin.javascript.navig5.JSObject" 's java_object isInstance of
"netscape/javascript/JSObject". so that it unwrap without wrap.

Comment 21

[Asa Dotzler [:asa]]

Too late for 1.4.1

Comment 22

[Asa Dotzler [:asa]]

Probably too late for 1.5 but let us know if you get a fix into 1.6 soon and
we'll consider it for 1.5.

Comment 23

[Jack Lu]

I looked into the code and I don't think there is an easy fix for this bug. If 
we can fix it, we need to understand why there is a need to use JSObjectHandle 
and find an alternative way to do it.

For most cases, JSObject is wrapped inside JSObjectHandle, however, for the 
testcase mentioned, it directly invokes the constructor of s.p.j.n.JSObject 
without first converting the second param to JSObjectHandle. That is why the 
crashes from.

Comment 24

[Brendan Eich [:brendan]]

Did this regress during 1.5?  Did the testcase work in 1.4?

/be

Comment 25

[Joshua Xia]

This bug is duplicate of #200016

Comment 26

[Joshua Xia]

*** Bug 221982 has been marked as a duplicate of this bug. ***

Comment 27

[Joshua Xia]

sorry,  #221982 is mark as dup of this bug.

Comment 28

[Joshua Xia]

->kyle

Comment 29

[Kyle Yuan]

Brendan, no, the test case does not work in 1.4 too. I'm looking into this 
right now, is there any chance to get it into 1.6?

Comment 30

[Daniel Veditz [:dveditz]]

We'd have to see a patch and evaluate its risk before granting actual approval
for 1.6, but this is definitely something that we'd like to have fixed. Some of
the comments imply that the fix isn't going to be easy, though, so no
guarantees. If it looks like something that needs quite a bit of real-world
testing it may get bumped to 1.7a since it won't even have the 1.6beta shakedown
period.

Comment 31

[chris hofmann]

please nominate this bug as soon as a patch appears to help in getting it
evaluated for the next release

Comment 32

[Kyle Yuan]

I'm discussing with the java team about preventing users from creating
sun.plugin.javascript.navig5.JSObject in the javascript side (that was a mistake
because JSObject is only used for communication from java to javascript, it's
not supposed to be created in the javascript side). But before the possible fix
in the JRE side could happen (not earlier than JRE 1.5.1, I guess), we have to
hack in the OJI side to block this call. That's ugly but low-risky. 
I think Xiaobin's comment 23 did not exactly cover the real problem.

Comment 33

[Jack Lu]

I don't think blocking sun.plugin.javascript.navig5.JSObject is a good idea 
since that is not the only one public class the user can access. It is 
impractical to block all such calls to construct an object or whatever.

Comment 34

[Kyle Yuan]

When the creation of sun.plugin.javascript.navig5.JSObject is done, javascript
wants to wrap the java object to a js object, but unfortunately,
sun.plugin.javascript.navig5.JSObject is derived from
netscape.javascript.JSObject, i.e. it's not an ordinary java object, so js turns
to unwrap the java object, that's the real problem. See:
http://lxr.mozilla.org/seamonkey/source/js/src/liveconnect/jsj_convert.c#854

Actually, I'm not just thinking about blocking
sun.plugin.javascript.navig5.JSObject, I'm thinking about blocking all the
access to sun.* packages from js.

Comment 35

[Kyle Yuan]

Re: comment 33
Though there are thousands of other packages user can access, but only the
classes derived from netscape.javascript.JSObject have this kind of problem.

Comment 36

[Jack Lu]

Kyle,

The easy fix will be to insert some logic in invoke_java_constructor before
calling jsj_ConvertJavaObjectToJSValue.

if (the returned java object instanceof netscape.javascript.jsobject)
   don't try to grap the js_obj from JSObjectHandle

You can add another flag to jsj_UnwrapJSObjectWrapper to indicate that the 
passed in java_wrapper_obj is a raw jsobject instead of being a JSObjectHandle.

Comment 37

[Kyle Yuan]

Attachment: prevent JSObject and its derived classes from being created in js

Comment 38

[Kyle Yuan]

Comment on attachment 136917 [details] [diff] [review]
prevent JSObject and its derived classes from being created in js

Xiaobin, I don't understand the 2nd change you said in comment 36. Is that
necessary for this bug?

Comment 39

[Jack Lu]

Kyle,

You change looks good to me assuming remove the support of JSObject 
instantiation through sun package would be agreed by most of people here.

Actually if I understand correctly, the whold problem is the current 
liveconnect code assumes all the JSObject passed from Java side is a 
JSObjectHandle wrapper around the real underlying jsobject. That assumption 
won't be correct if the creation of the JSObject is implicilty created by 
liveconnect engine through OJI liveconnect glue interface.

Coming back, if we still want to support something like  new Packages.sun.
plugin.javascript.navig5.JSObject(1,1), liveconnect code should be able to 
differentiate the normal creation of jsobject and any jsobject passed to 
javaside. This is what I proposed in comment 36. We need a flag to indicate that 
this is a normal creation of jsobject, not anything originally passed from 
Javascript side.

Comment 40

[Kyle Yuan]

>Actually if I understand correctly, the whold problem is the current 
>liveconnect code assumes all the JSObject passed from Java side is a 
>JSObjectHandle wrapper around the real underlying jsobject.

I'd like to agree to this assumption. If we allow the JSObject to be created in
js, don't you think the following code is weird?

jsfunction () {
  jsobj = new Packages.sun.plugin.javascript.navig5.JSObject(1,this);
  jsobj.call("jsfunction", null);
}

Comment 41

[Kyle Yuan]

Attachment: Approach #2, address Xiaobin's comment, add a flag to indicate whether the java object is created in js

Frankly to say, I don't like this patch which takes such a big effort to
support an useless scenario. But in the interest of fixing this asap, I'd like
to listen to anybody else's option on those two patches.

Comment 42

[Marc Schoenefeld]

I was just wondering if the bug I submitted 9 months ago is fixed already, 
but unfortunately not, next time i will check is the 31st of March, wenn this 
bug is getting one year old ..... 

Comment 43

[chris hofmann]

lets try to make some progress for 1.7beta

Comment 44

[Kyle Yuan]

Attachment: workaround fix - block accessing to all classes in sun.plugin package

Please see the comments in my patch for detail.

I discussed this issue with Xiaobin via emails. We all agree to use this
workaround fix in mozilla side first, then do a better job in the next jre
release.

Comment 45

[Kyle Yuan]

Comment on attachment 141712 [details] [diff] [review]
workaround fix - block accessing to all classes in sun.plugin package

sorry for the indent problem. I should use space instead of tab.

Comment 46

[Xiaobin Lu]

Comment on attachment 141712 [details] [diff] [review]
workaround fix - block accessing to all classes in sun.plugin package

Kyle, please remove the comment about invoking java.lang.SecurityManager....
comment. That comment really needs to be investigated further.

Comment 47

[Kyle Yuan]

Comment on attachment 141712 [details] [diff] [review]
workaround fix - block accessing to all classes in sun.plugin package

Okay, I'll take those words back until Xiaobin and I find out the ultimate
solution.

Comment 48

[Brendan Eich [:brendan]]

Comment on attachment 141712 [details] [diff] [review]
workaround fix - block accessing to all classes in sun.plugin package

I see tabs in the + lines -- please use spaces, and indent to the right column.
 sr=[e-mail] with that nit picked.

/be

Comment 49

[Kyle Yuan]

checked in with comments/indent revised.