Crash [@ js_AllocSlot] or "Assertion failure: obj->map->ops->defineProperty == js_DefineProperty" with evalcx

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: jorendorff)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
assertion, crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+, blocking1.9.2 -, status1.9.2 wontfix, status1.9.1 unaffected)

Details

(Whiteboard: [sg:nse][ccbr][critsmash:investigating][fixed-in-tracemonkey])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 years ago
evalcx('var p;', [])

Assertion failure: obj->map->ops->defineProperty == js_DefineProperty, at ../jsops.cpp:2752
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   32011:369b7fbc2304
user:        Jason Orendorff
date:        Wed Aug 26 14:28:36 2009 -0700
summary:     Bug 508685 - Remove last parameter of defineProperty op. r=brendan.
Blocks: 508685
Keywords: regression
OS: Mac OS X → All
Hardware: x86 → All
evalcx('')
Function("evalcx(\"var p\",[])")()

crashes opt shell at js_AllocSlot on TM tip without -j and gives a similar assert for a debug shell.

Looks dangerous (mov instruction from a weird memory address), so setting s-s and assuming [sg:critical?] unless otherwise.

===

(gdb) bt
#0  0x0000000000465803 in js_AllocSlot ()
#1  0x00000000004c032f in JSScope::getChildProperty(JSContext*, JSScopeProperty*, JSScopeProperty&) ()
#2  0x00000000004c0607 in JSScope::addPropertyHelper(JSContext*, long, int (*)(JSContext*, JSObject*, long, long*), int (*)(JSContext*, JSObject*, long, long*), unsigned int, unsigned int, unsigned int, int, JSScopeProperty**) ()
#3  0x00000000004c0ace in JSScope::putProperty(JSContext*, long, int (*)(JSContext*, JSObject*, long, long*), int (*)(JSContext*, JSObject*, long, long*), unsigned int, unsigned int, unsigned int, int) ()
#4  0x0000000000469474 in js_DefineNativeProperty ()
#5  0x000000000054abcb in js_Interpret ()
#6  0x0000000000457b51 in js_Execute ()
#7  0x000000000040b815 in JS_EvaluateUCScriptForPrincipals ()
#8  0x000000000040b8e2 in JS_EvaluateUCScript ()
#9  0x00000000004071ae in EvalInContext(JSContext*, JSObject*, unsigned int, long*, long*) ()
#10 0x0000000000458352 in js_Invoke ()
#11 0x000000000054680a in js_Interpret ()
#12 0x0000000000457b51 in js_Execute ()
#13 0x000000000040b176 in JS_ExecuteScript ()
#14 0x00000000004062c0 in Process(JSContext*, JSObject*, char*, int) ()
#15 0x0000000000406ecf in main ()
(gdb) x/i $rip
=> 0x465803 <js_AllocSlot+35>:	mov    0x8(%rsi),%rax
(gdb) x/b $rsi
0x4179bfcc00000000:	Cannot access memory at address 0x4179bfcc00000000
Group: core-security
blocking1.9.2: --- → ?
blocking2.0: --- → ?
Whiteboard: [sg:critical?][ccbr]
Comment #2 was tested on 64-bit Ubuntu Linux 10.04.
(Assignee)

Updated

8 years ago
Assignee: general → jorendorff
(Assignee)

Comment 4

8 years ago
This affects code that does *not* use JSOPTION_VAROBJFIX and that can pass a non-native object to the "obj" parameter of JS_ExecuteScript or similar APIs.

So the fix is either "don't do that" (document the API restriction and fix the shell) or change JSOP_DEFVAR to cope with a non-native varobj.
(Assignee)

Comment 5

8 years ago
Created attachment 446017 [details] [diff] [review]
v1

This patch restores the functionality. It adds a (predictable) branch in the interpreter.

No changes needed to jstracer because we don't trace these instructions. (I don't think they can occur in a loop.)
Attachment #446017 - Flags: review?(brendan)
(In reply to comment #4)
> So the fix is either "don't do that" (document the API restriction and fix the
> shell) or change JSOP_DEFVAR to cope with a non-native varobj.

Not looking for more non-native variable objects at this point, so let's say "don't do that". But make it stronger than doc -- assert that the obj passed into the API is native.

/be
(Assignee)

Comment 7

8 years ago
Comment on attachment 446017 [details] [diff] [review]
v1

Withdrawing v1.
Attachment #446017 - Flags: review?(brendan)

Updated

8 years ago
Whiteboard: [sg:critical?][ccbr] → [sg:critical?][ccbr][critsmash:investigating]
(Assignee)

Comment 8

8 years ago
Historical note - bug 508685 comment 0 mentions this possibility, but I never followed up. :(
(Assignee)

Comment 9

8 years ago
Created attachment 446045 [details] [diff] [review]
v2
Attachment #446017 - Attachment is obsolete: true
Attachment #446045 - Flags: review?(brendan)
Comment on attachment 446045 [details] [diff] [review]
v2

Could just assert initialVarObj->isNative() but I'll defer to you at this point.

/be
Attachment #446045 - Flags: review?(brendan) → review+
(Assignee)

Comment 11

8 years ago
isNative() would be true for XML objects and With objects, at least for the time being--pending bug 561785 and bug 564936, respectively.
(Assignee)

Comment 12

8 years ago
Try-servering, on the off chance that we expose some kind of eval-like API internally that could be tickled by this.

It is, I guess, possible that this could cause a surprise or two when we switch security wrappers from the current implementation (as native objects) to proxies.
Summary: "Assertion failure: obj->map->ops->defineProperty == js_DefineProperty" with evalcx → Crash [@ js_AllocSlot] or "Assertion failure: obj->map->ops->defineProperty == js_DefineProperty" with evalcx
blocking1.9.2: ? → needed
status1.9.1: --- → unaffected
status1.9.2: --- → wanted
(Assignee)

Comment 13

8 years ago
http://hg.mozilla.org/tracemonkey/rev/5d30188ef5a9
Whiteboard: [sg:critical?][ccbr][critsmash:investigating] → [sg:critical?][ccbr][critsmash:investigating][fixed-in-tracemonkey]

Comment 14

8 years ago
Gary found another way of hitting this:

x = Proxy.create({}, this)
evalcx("x", x)

proxies are non-native, like the dense array above

Updated

8 years ago
Duplicate of this bug: 566908

Comment 16

8 years ago
http://hg.mozilla.org/mozilla-central/rev/5d30188ef5a9
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
blocking1.9.2: needed → .5+
blocking1.9.2: .5+ → .6+
(Assignee)

Comment 17

8 years ago
Hi! Why is this bug blocking 1.9.2.6? The patch doesn't apply cleanly to mozilla-1.9.2 and I'd rather not backport it without understanding why we want it there. All the test cases are shell-only and involve evalcx.

Comment 18

8 years ago
Generally any fixed sg-crit that has landed on trunk will get a blocking flag. Because this was found internally and of comment 17 we'll not block on this for 1.9.2.6.

Updated

8 years ago
blocking1.9.2: .6+ → needed

Updated

7 years ago
blocking2.0: ? → betaN+
If this is shell-only we don't need it on the older branches, and it probably wasn't ever "sg:critical" if it was a bug in the non-shipping js-shell only.
Group: core-security
blocking1.9.2: needed → -
status1.9.2: wanted → wontfix
Whiteboard: [sg:critical?][ccbr][critsmash:investigating][fixed-in-tracemonkey] → [sg:nse][ccbr][critsmash:investigating][fixed-in-tracemonkey]
(Reporter)

Updated

6 years ago
Keywords: crash
A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-566549.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.