Closed Bug 12367 Opened 21 years ago Closed 18 years ago

liveconnect should not abuse JSObjectOps.lookupProperty

Categories

(Core Graveyard :: Java: Live Connect, defect, P1, critical)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.4

People

(Reporter: jband_mozilla, Assigned: brendan)

References

()

Details

(Keywords: crash, js1.5)

Attachments

(1 file)

running lcshell.exe on NT...

js> foo = new java.lang.Object()
java.lang.Object@1f99c0
js> foo.constructor

crashes with stack:

LookupResult(JSContext * 0x004100f0, JSObject * 0x0032b290, JSObject *
0x0032b290, JSProperty * 0x00000001) line 1612 + 45 bytes
JS_LookupProperty(JSContext * 0x004100f0, JSObject * 0x0032b290, const char *
0x0045dac0, long * 0x0012ed68) line 1702 + 21 bytes
lookup_member_by_id(JSContext * 0x004100f0, const JNINativeInterface_ * *
0x21d5c994, JSObject * 0x0032b588, JavaObjectWrapper * * 0x0012e760, long
0x00411be0, JavaMemberDescriptor * * 0x0012e744, long * 0x0012ed68) line 435 +
28 bytes
JavaObject_getPropertyById(JSContext * 0x004100f0, JSObject * 0x0032b588, long
0x00411be0, long * 0x0012ed68) line 473 + 33 bytes
js_Interpret(JSContext * 0x004100f0, long * 0x0012fed8) line 2184 + 892 bytes
js_Execute(JSContext * 0x004100f0, JSObject * 0x0032a650, JSScript * 0x0045de60,
JSFunction * 0x00000000, JSStackFrame * 0x00000000, int 0x00000000, long *
0x0012fed8) line 827 + 13 bytes
JS_ExecuteScript(JSContext * 0x004100f0, JSObject * 0x0032a650, JSScript *
0x0045de60, long * 0x0012fed8) line 2523 + 27 bytes
Process(JSContext * 0x004100f0, JSObject * 0x0032a650, char * 0x00000000) line
248 + 22 bytes
ProcessArgs(JSContext * 0x004100f0, JSObject * 0x0032a650, char * * 0x00410264,
int 0x00000000) line 366 + 17 bytes
main(int 0x00000000, char * * 0x00410264) line 1724 + 21 bytes
LCSHELL! mainCRTStartup + 227 bytes
KERNEL32! 77f1ba3c()

crashed at line indicated below with sprop == 1

static jsval
LookupResult(JSContext *cx, JSObject *obj, JSObject *obj2, JSProperty *prop)
{
    JSScopeProperty *sprop;
    jsval rval;

    if (!prop) {
	/* XXX bad API: no way to tell "not defined" from "void value" */
	return JSVAL_VOID;
    }
    if (OBJ_IS_NATIVE(obj2)) {
	/* Peek at the native property's slot value, without doing a Get. */
	sprop = (JSScopeProperty *)prop;
here->	rval = LOCKED_OBJ_GET_SLOT(obj2, sprop->slot);
    } else {
	/* XXX bad API: no way to return "defined but value unknown" */
	rval = JSVAL_TRUE;
    }
    OBJ_DROP_PROPERTY(cx, obj2, prop);
    return rval;
}
The crash is happening because JS engine code is being executed that should only
be run with a native JS object, but the object being manipulated is a host
object, created by LiveConnect.  This is because the MAP_IS_NATIVE() macro, used
for detection of native objects, was changed to accomodate XPConnect:

#define MAP_IS_NATIVE(map)  \
                   ((map)->ops == &js_ObjectOps || \
                    ((map)->ops && (map)->ops->newObjectMap ==
                     js_ObjectOps.newObjectMap))

The JavaObject class hijacks the newObjectMap() function that is used by native
JS objects, so MAP_IS_NATIVE() now inappropriately returns true for LiveConnect
objects.  Ironically, this change was made by jband, with my review.

Now to find a reasonable fix...
Severity: normal → critical
Status: NEW → ASSIGNED
Assignee: fur → rogerl
Status: ASSIGNED → NEW
Scott has fix. But reassigning to Roger, new liveconnect owner.
Assignee: rogerl → fur
Actually, I'll see this through to completion. Reassigning to myself.
Target Milestone: M12
Moving what's not done for M12 to M13.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
I checked in a "fix" by putting a wrapper around the default newObjectMap()
function.  Now, we don't crash, but the result of foo.constructor is 'true'.  I
would rather it be 'undefined' instead, but that fix is not as straightforward
and probably not worth making - I'll leave it to rogerl if he wants to file a
new bug on that.
Adding crash keyword
Keywords: crash
Shaver and I are about to reopen this, to make progress and open the tree.  The 
real problem here, AFAICT, is the punning of 1 as a property pointer.  Anyway, 
this bug is not likely to bite anyone soon.  Taking for 0.9.

/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Mine.

/be
Assignee: fur → brendan
Status: REOPENED → NEW
Status: NEW → ASSIGNED
Keywords: mozilla0.9
Summary: crash doing lookingup of foo.constructor → crash doing looking up of foo.constructor
Target Milestone: M13 → mozilla0.9
Summary: crash doing looking up of foo.constructor → liveconnect should not abuse JSObjectOps.lookupProperty
Sliding.

/be
Keywords: mozilla0.9mozilla0.9.1
Target Milestone: mozilla0.9 → mozilla0.9.1
This never happens in the real world.  I'll fix it in the next milestone.

/be
Target Milestone: mozilla0.9.1 → mozilla0.9.2
unsetting tmilestone
Keywords: mozilla0.9.2
Target Milestone: mozilla0.9.2 → ---
Target Milestone: --- → mozilla0.9.3
Another easy fix for js1.5.

/be
Keywords: js1.5
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Upping priority, this does happen in the real world.

/be
OS: Windows NT → All
Priority: P3 → P1
Hardware: PC → All
*** Bug 94611 has been marked as a duplicate of this bug. ***
Want a fix for 0.9.4.  I'm working on it.

/be
Keywords: mozilla0.9.4
Comment on attachment 47921 [details] [diff] [review]
simple, safe, symmetric (see js_GetProperty and others) fix

sr=jband (and r=shaver - he's still supposed to do this, right? Else we have to  dig through the bug activity to see who set the has-review checkbox).

I had to think about this a bit. I'm convinced this is not going to trip up any code I know of. 

It would be nice if someone with liveconnect setup would do some testing.

Are you still going to fix the liveconnect ugliness of (JSProperty*)1 before this bug is closed?
Attachment #47921 - Flags: superreview+
jband: I tried to test, but the Java 2 plugin I downloaded upon going to the
test URL (above, in this bug now) wedged in PR_Read.  Joy.

/be
Attachment #47921 - Flags: approval+
Comment on attachment 47921 [details] [diff] [review]
simple, safe, symmetric (see js_GetProperty and others) fix

a=asa on behalf of drivers.
Fix is in.

jband: I see no abuse in LiveConnect, any longer.  1 cast to a pointer may be
ugly, but it works (cf. Unix signal.h) and it's LiveConnect's business, so long
as LiveConnect objects don't masquerade as native (JSObjectOps) ones -- which
they no longer do.  So I'm closing this bug.

/be
Status: ASSIGNED → RESOLVED
Closed: 20 years ago18 years ago
Resolution: --- → FIXED
-- Very old bug, looks like its fixed. Marking Verified.
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.