Closed Bug 867946 Opened 7 years ago Closed 6 years ago

Crash [@ __strlen_sse2_bsf] through [@ JS_BasicObjectToString]

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla25
Tracking Status
firefox22 --- unaffected
firefox23 + fixed
firefox24 + fixed
firefox25 + fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: decoder, Assigned: jimb)

References

(Blocks 2 open bugs)

Details

(5 keywords, Whiteboard: [fuzzblocker] [jsbugmon:update][adv-main23-])

Crash Data

Attachments

(2 files, 2 obsolete files)

The following testcase crashes on mozilla-central revision 70e0955ccc87 (run with --ion-eager):


b = {};
b.__proto__ = evalcx("lazy");
function g(a1, a2) {
    var d = g(b.toString()) 
}
g(typeof g.set, "function");
Crash trace:


Program received signal SIGSEGV, Segmentation fault.
__strlen_sse2_bsf () at ../sysdeps/i386/i686/multiarch/strlen-sse2-bsf.S:51
51      ../sysdeps/i386/i686/multiarch/strlen-sse2-bsf.S: No such file or directory.
(gdb) bt
#0  __strlen_sse2_bsf () at ../sysdeps/i386/i686/multiarch/strlen-sse2-bsf.S:51
#1  0x0828d489 in JS_BasicObjectToString (cx=0x9368450, obj=(JSObject * const) 0xf7449010 [object Proxy]) at js/src/builtin/Object.cpp:295
#2  0x0828d84b in obj_toString (cx=0x9368450, argc=0, vp=0xf7693148) at js/src/builtin/Object.cpp:327
#3  0x0814b66d in js::CallJSNative (cx=0x9368450, native=0x828d7e0 <obj_toString(JSContext*, unsigned int, JS::Value*)>, args=...) at ../jscntxtinlines.h:337
#4  0x08161069 in js::InvokeKernel (cx=0x9368450, args=..., construct=js::NO_CONSTRUCT) at js/src/jsinterp.cpp:428
#5  0x08161d5c in Invoke (args=..., cx=0x9368450, construct=<optimized out>) at js/src/jsinterp.h:134
#6  js::Invoke (cx=0x9368450, thisv=..., fval=..., argc=0, argv=0xf7693148, rval=0xf7693138) at js/src/jsinterp.cpp:475
#7  0x081b6b32 in js::DirectProxyHandler::call (this=0x93304f8, cx=0x9368450, proxy=(JSObject * const) 0xf743e060 [object Proxy], args=...) at js/src/jsproxy.cpp:479
#8  0x082513f5 in js::CrossCompartmentWrapper::call (this=0x93304f8, cx=0x9368450, wrapper=(JSObject * const) 0xf743e060 [object Proxy], args=...) at js/src/jswrapper.cpp:445
#9  0x081bbf1c in js::Proxy::call (cx=0x9368450, proxy=(JSObject * const) 0xf743e060 [object Proxy], args=...) at js/src/jsproxy.cpp:2611
#10 0x081bc029 in proxy_Call (cx=0x9368450, argc=0, vp=0xf7693138) at js/src/jsproxy.cpp:3175
#11 0x0814b66d in js::CallJSNative (cx=0x9368450, native=0x81bbf90 <proxy_Call(JSContext*, unsigned int, JS::Value*)>, args=...) at ../jscntxtinlines.h:337
#12 0x08161258 in js::InvokeKernel (cx=0x9368450, args=..., construct=js::NO_CONSTRUCT) at js/src/jsinterp.cpp:421
#13 0x08161d5c in Invoke (args=..., cx=0x9368450, construct=<optimized out>) at js/src/jsinterp.h:134
#14 js::Invoke (cx=0x9368450, thisv=..., fval=..., argc=0, argv=0xffefe7b0, rval=0xffefe778) at js/src/jsinterp.cpp:475
#15 0x08717af9 in js::ion::DoCallFallback (cx=0x9368450, frame=0xffefe7e8, stub=0x93848c0, argc=0, vp=0xffefe7a0, res=$jsval(-nan(0xfff8200000000))) at js/src/ion/BaselineIC.cpp:6587
[... lots! of jit frames ... ]
(gdb) x /i $pc
=> 0xf7d3b6b6 <__strlen_sse2_bsf+22>:   movdqu (%edi),%xmm1
(gdb) info reg edi
edi            0x0      0


This crashes at NULL but marking s-s because I don't know what's going on here.
Crash Signature: [@ __strlen_sse2_bsf] through [@ JS_BasicObjectToString] → [@ __strlen_sse2_bsf] [@ JS_BasicObjectToString]
Whiteboard: [jsbugmon:update,bisect]
Do we have versions this affects? Only 23 or farther back?
Crash Signature: [@ __strlen_sse2_bsf] [@ JS_BasicObjectToString] → [@ __strlen_sse2_bsf] [@ JS_BasicObjectToString]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   130404:e9d66cb5f791
user:        Jim Blandy
date:        Tue Apr 30 14:44:50 2013 -0700
summary:     Bug 862531: Replace BaseProxyHandler::obj_toString with className. r=jorendorff

This iteration took 343.107 seconds to run.
Crash Signature: [@ __strlen_sse2_bsf] [@ JS_BasicObjectToString] → [@ __strlen_sse2_bsf] [@ JS_BasicObjectToString]
Needinfo from jimb based on comment 3 :)
Flags: needinfo?(jimb)
Assignee: general → jimb
Blocks: 862531
Keywords: regression
This looks like a regression from your patch, Jim, could you take a look?  Thanks.
Yes, I'll take this.
Flags: needinfo?(jimb)
QA Contact: general → jimb
jsfunfuzz hit this as well, but the testcases were somewhat less reliable.
Blocks: 349611
This throws up many different assorted signatures, but all seem to involve JS_BasicObjectToString on the stack.
Whiteboard: [jsbugmon:update] → [fuzzblocker] [jsbugmon:update]
Attachment #754419 - Attachment is obsolete: true
Jimb, what's the status on this bug? I still see it in the fuzzer.
Flags: needinfo?(jimb)
Following up in email.
The bug here is that className ought to be infallible, but Proxy::className can return NULL in the case of overrecursion:

http://dxr.mozilla.org/mozilla-central/source/js/src/jsproxy.cpp#l2688

The test case trips this because it builds a deep stack by having g call itself, and manages to arrange for Proxy::className to be the first thing that notices the deep stack.

So, this crash will only occur in scripts that are about to get terminated for too much recursion anyway.

I'm looking at fixes.
Flags: needinfo?(jimb)
Attached patch className-recursion.patch (obsolete) — Splinter Review
This isn't classy, but I think it's probably adequate.
Attachment #780699 - Flags: review?(jwalden+bmo)
Attachment #780699 - Flags: review?(jwalden+bmo) → review+
Patch revised slightly, and added regression test. Carrying over the r=jwalden.
Attachment #780699 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/6051977db2a8
Flags: in-testsuite+
OS: Linux → All
Hardware: x86 → All
Target Milestone: --- → mozilla25
Do we have time to get this into the beta?
(In reply to Jim Blandy :jimb from comment #17)
> Do we have time to get this into the beta?

You could nominate for approval for the branches and let the release drivers decide.
Comment on attachment 780715 [details] [diff] [review]
Bug 876946: Never return null from Proxy::className, even if we're over-recursed. r=jwalden

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 862531

User impact if declined: content JavaScript that recurses deeply and uses cross-compartment wrappers could crash instead of being terminated for unbounded recursion.

Testing completed (on m-c, etc.): Regression test included in patch; landed in inbound, JS tests green: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=6051977db2a8

Risk to taking this patch (and alternatives if risky): none

String or IDL/UUID changes made by this patch: none
Attachment #780715 - Flags: approval-mozilla-beta?
Attachment #780715 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/b602ded747d0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Comment on attachment 780715 [details] [diff] [review]
Bug 876946: Never return null from Proxy::className, even if we're over-recursed. r=jwalden

Given the low risk and the win (less crashes for the user) we'll take this today and get it into our second-to-last beta which goes to build today.
Attachment #780715 - Flags: approval-mozilla-beta?
Attachment #780715 - Flags: approval-mozilla-beta+
Attachment #780715 - Flags: approval-mozilla-aurora?
Attachment #780715 - Flags: approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
Crash Signature: [@ __strlen_sse2_bsf] [@ JS_BasicObjectToString] → [@ __strlen_sse2_bsf] [@ JS_BasicObjectToString]
JSBugMon: This bug has been automatically verified fixed.
Crash Signature: [@ __strlen_sse2_bsf] [@ JS_BasicObjectToString] → [@ __strlen_sse2_bsf] [@ JS_BasicObjectToString]
Keywords: checkin-needed
Whiteboard: [fuzzblocker] [jsbugmon:update] → [fuzzblocker] [jsbugmon:update][adv-main23-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.