Closed Bug 456284 Opened 16 years ago Closed 16 years ago

Null derefs in js_Interpret with split objects

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9.1b3

People

(Reporter: bc, Assigned: mrbkap)

Details

(Keywords: verified1.9.1)

Attachments

(1 file, 1 obsolete file)

Updated
2.67 KB, patch
crowderbt
: review+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review 
mozilla-central

ecma/ExecutionContexts/10.2.2-1.js

#0  0x00000000 in ?? ()
#1  0x00065a6b in js_Interpret (cx=0x300d10) at jsinterp.cpp:3628
#2  0x00098d6f in js_Execute (cx=0x300d10, chain=0x250100, script=0x809a00, down=0x0, flags=0, result=0x0) at jsinterp.cpp:1550
#3  0x00018d06 in JS_ExecuteScript (cx=0x300d10, obj=0x250100, script=0x809a00, rval=0x0) at jsapi.cpp:4910
#4  0x00002b1a in Process (cx=0x300d10, obj=0x250100, filename=0xbffff635 "10.2.2-1.js", forceTTY=0) at js.cpp:277
#5  0x000081ee in ProcessArgs (cx=0x300d10, obj=0x250100, argv=0xbffff4d4, argc=11) at js.cpp:510
#6  0x000094b4 in main (argc=11, argv=0xbffff4d4, envp=0xbffff504) at js.cpp:3982

==

ecma/ExecutionContexts/10.2.2-2.js

#0  0x00000000 in ?? ()
#1  0x00065a6b in js_Interpret (cx=0x300d10) at jsinterp.cpp:3628
#2  0x00098d6f in js_Execute (cx=0x300d10, chain=0x2506e0, script=0x30b280, down=0xbfffe250, flags=18, result=0xbfffcadc) at jsinterp.cpp:1550
#3  0x000ab276 in js_obj_eval (cx=0x300d10, obj=0x2500c0, argc=1, argv=0x80e5b8, rval=0xbfffcadc) at jsobj.cpp:1338
#4  0x0009a1af in js_Invoke (cx=0x300d10, argc=1, vp=0x80e5b0, flags=2) at jsinterp.cpp:1306
#5  0x000761c6 in js_Interpret (cx=0x300d10) at jsinterp.cpp:4976
#6  0x00098d6f in js_Execute (cx=0x300d10, chain=0x250100, script=0x809a00, down=0x0, flags=0, result=0x0) at jsinterp.cpp:1550
#7  0x00018d06 in JS_ExecuteScript (cx=0x300d10, obj=0x250100, script=0x809a00, rval=0x0) at jsapi.cpp:4910
#8  0x00002b1a in Process (cx=0x300d10, obj=0x250100, filename=0xbffff635 "10.2.2-2.js", forceTTY=0) at js.cpp:277
#9  0x000081ee in ProcessArgs (cx=0x300d10, obj=0x250100, argv=0xbffff4d4, argc=11) at js.cpp:510
#10 0x000094b4 in main (argc=11, argv=0xbffff4d4, envp=0xbffff504) at js.cpp:3982

==

ecma/ExecutionContexts/10.2.3-1.js

#0  0x00000000 in ?? ()
#1  0x00065a6b in js_Interpret (cx=0x300d10) at jsinterp.cpp:3628
#2  0x00098d6f in js_Execute (cx=0x300d10, chain=0x250100, script=0x30a980, down=0x0, flags=0, result=0x0) at jsinterp.cpp:1550
#3  0x00018d06 in JS_ExecuteScript (cx=0x300d10, obj=0x250100, script=0x30a980, rval=0x0) at jsapi.cpp:4910
#4  0x00002b1a in Process (cx=0x300d10, obj=0x250100, filename=0xbffff635 "10.2.3-1.js", forceTTY=0) at js.cpp:277
#5  0x000081ee in ProcessArgs (cx=0x300d10, obj=0x250100, argv=0xbffff4d4, argc=11) at js.cpp:510
#6  0x000094b4 in main (argc=11, argv=0xbffff4d4, envp=0xbffff504) at js.cpp:3982

ecma/ObjectObjects/15.2.2.1.js

==

#0  0x00000000 in ?? ()
#1  0x00066655 in js_Interpret (cx=0x300d10) at jsinterp.cpp:3632
#2  0x00098d6f in js_Execute (cx=0x300d10, chain=0x250100, script=0x80be00, down=0x0, flags=0, result=0x0) at jsinterp.cpp:1550
#3  0x00018d06 in JS_ExecuteScript (cx=0x300d10, obj=0x250100, script=0x80be00, rval=0x0) at jsapi.cpp:4910
#4  0x00002b1a in Process (cx=0x300d10, obj=0x250100, filename=0xbffff63d "15.2.2.1.js", forceTTY=0) at js.cpp:277
#5  0x000081ee in ProcessArgs (cx=0x300d10, obj=0x250100, argv=0xbffff4dc, argc=11) at js.cpp:510
#6  0x000094b4 in main (argc=11, argv=0xbffff4dc, envp=0xbffff50c) at js.cpp:3982

==

js1_5/Function/15.3.4.4.js

#0  0x00000000 in ?? ()
#1  0x00066655 in js_Interpret (cx=0x300d10) at jsinterp.cpp:3632
#2  0x00098d6f in js_Execute (cx=0x300d10, chain=0x250100, script=0x80d000, down=0x0, flags=0, result=0x0) at jsinterp.cpp:1550
#3  0x00018d06 in JS_ExecuteScript (cx=0x300d10, obj=0x250100, script=0x80d000, rval=0x0) at jsapi.cpp:4910
#4  0x00002b1a in Process (cx=0x300d10, obj=0x250100, filename=0xbffff645 "15.3.4.4.js", forceTTY=0) at js.cpp:277
#5  0x000081ee in ProcessArgs (cx=0x300d10, obj=0x250100, argv=0xbffff4e4, argc=11) at js.cpp:510
#6  0x000094b4 in main (argc=11, argv=0xbffff4e4, envp=0xbffff514) at js.cpp:3982

==

js1_5/Regress/regress-281930.js

#0  0x00000000 in ?? ()
#1  0x00066655 in js_Interpret (cx=0x300d10) at jsinterp.cpp:3632
#2  0x00098d6f in js_Execute (cx=0x300d10, chain=0x250100, script=0x307aa0, down=0x0, flags=0, result=0x0) at jsinterp.cpp:1550
#3  0x00018d06 in JS_ExecuteScript (cx=0x300d10, obj=0x250100, script=0x307aa0, rval=0x0) at jsapi.cpp:4910
#4  0x00002b1a in Process (cx=0x300d10, obj=0x250100, filename=0xbffff63d "regress-281930.js", forceTTY=0) at js.cpp:277
#5  0x000081ee in ProcessArgs (cx=0x300d10, obj=0x250100, argv=0xbffff4dc, argc=11) at js.cpp:510
#6  0x000094b4 in main (argc=11, argv=0xbffff4dc, envp=0xbffff50c) at js.cpp:3982

==

js1_5/extensions/regress-164697.js

#0  0x00000000 in ?? ()
#1  0x00065a6b in js_Interpret (cx=0x300d10) at jsinterp.cpp:3628
#2  0x00098d6f in js_Execute (cx=0x300d10, chain=0x250600, script=0x3081b0, down=0x80e514, flags=16, result=0xbfffcadc) at jsinterp.cpp:1550
#3  0x000ab276 in js_obj_eval (cx=0x300d10, obj=0x2500c0, argc=1, argv=0x80e594, rval=0xbfffcadc) at jsobj.cpp:1338
#4  0x0009a1af in js_Invoke (cx=0x300d10, argc=1, vp=0x80e58c, flags=2) at jsinterp.cpp:1306
#5  0x000761c6 in js_Interpret (cx=0x300d10) at jsinterp.cpp:4976
#6  0x00098d6f in js_Execute (cx=0x300d10, chain=0x250100, script=0x813a00, down=0x0, flags=0, result=0x0) at jsinterp.cpp:1550
#7  0x00018d06 in JS_ExecuteScript (cx=0x300d10, obj=0x250100, script=0x813a00, rval=0x0) at jsapi.cpp:4910
#8  0x00002b1a in Process (cx=0x300d10, obj=0x250100, filename=0xbffff635 "regress-164697.js", forceTTY=0) at js.cpp:277
#9  0x000081ee in ProcessArgs (cx=0x300d10, obj=0x250100, argv=0xbffff4d4, argc=11) at js.cpp:510
#10 0x000094b4 in main (argc=11, argv=0xbffff4d4, envp=0xbffff504) at js.cpp:3982

==

js1_6/extensions/regress-312385-01.js

#0  0x00000000 in ?? ()
#1  0x00065a6b in js_Interpret (cx=0x300d10) at jsinterp.cpp:3628
#2  0x00098d6f in js_Execute (cx=0x300d10, chain=0x250100, script=0x809800, down=0x0, flags=0, result=0x0) at jsinterp.cpp:1550
#3  0x00018d06 in JS_ExecuteScript (cx=0x300d10, obj=0x250100, script=0x809800, rval=0x0) at jsapi.cpp:4910
#4  0x00002b1a in Process (cx=0x300d10, obj=0x250100, filename=0xbffff62d "regress-312385-01.js", forceTTY=0) at js.cpp:277
#5  0x000081ee in ProcessArgs (cx=0x300d10, obj=0x250100, argv=0xbffff4cc, argc=11) at js.cpp:510
#6  0x000094b4 in main (argc=11, argv=0xbffff4cc, envp=0xbffff4fc) at js.cpp:3982
Flags: in-testsuite+
Flags: in-litmus-
still happening?
Yeah.
Flags: blocking1.9.1+
Attached patch Fix (obsolete) — — Splinter Review 
The comment in the patch says it all...
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #349349 - Flags: review?(crowder)
Comment on attachment 349349 [details] [diff] [review]
Fix

If it is a "rule" that JSExtendedClass host objects must have equality-hooks, then we should enforce with an assertion somewhere, right?
Attachment #349349 - Flags: review?(crowder) → review+
(In reply to comment #4)
> (From update of attachment 349349 [details] [diff] [review])
> If it is a "rule" that JSExtendedClass host objects must have equality-hooks,
> then we should enforce with an assertion somewhere, right?

We generally don't assert non-null, since a null pointer dereference (excluding insanely buggy OSes) is non-exploitable and as good a blame-arrow as an assert (less clear due to the lack of an assertion message, but more direct because the pc is at the faulting instruction).

/be
I think in this case, an assertion at the time that the class is inited would be much more helpful to 3rd parties using spidermonkey.  I think it makes the "rule" much more clear, since an external developer could easily ponder over a null-dereference like this for a long while without knowing that the equality hook is a requirement of the API.
Brendan: also, in this case the pc is not at the faulting instruction because we're *calling* the null pointer. I also keep forgetting that equality is not optional since it's the only required JSExtendedClass member.
Oh, sure -- if you can get coverage early and unconditionally via an assertion, go for it.

/be
Attached patch Updated — — Splinter Review 
Not all classes go through JS_InitClass, so this is the most central place we can assert.
Attachment #349349 - Attachment is obsolete: true
Attachment #349503 - Flags: review?(crowder)
Attachment #349503 - Flags: review?(crowder) → review+
Attachment #349503 - Flags: approval1.9.1?
Comment on attachment 349503 [details] [diff] [review]
Updated

This allows us to test harder!
Attachment #349503 - Flags: approval1.9.1? → approval1.9.1+
http://hg.mozilla.org/mozilla-central/rev/defd28229dd7

Next time please follow instructions ("make sure you include a commit message
and an username in the attachment").
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b3
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: