Bug 48485 - Crash in Function.prototype.call.apply
Summary: Crash in Function.prototype.call.apply
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.5
: P1 Normal
Assignee: Nobody
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2010-10-27 16:55 PDT by Charles L
Modified: 2010-11-05 20:33 PDT (History)
4 users (show)

See Also:


Attachments
Crash test (423 bytes, text/html)
2010-10-27 16:55 PDT, Charles L
no flags Details
Patch (9.44 KB, patch)
2010-11-03 16:17 PDT, Oliver Hunt
barraclough: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Charles L 2010-10-27 16:55:47 PDT
Created attachment 72114 [details]
Crash test 

The browser crashes when the following function is run:

Function.prototype.call.apply(fn, arguments)

where fn is any function, and the arguments object has at least 3 items.

I have experienced this only in recent nightly builds, using Mac OS X 10.5.8. The crash doesn't happen while the script debugger is on, or when it is run from the location bar, or when fn has been successfully called before.
Comment 1 Alexey Proskuryakov 2010-10-27 21:51:03 PDT
Confirmed with a local debug build of r70400.

#0	0x101ccc9d0 in WTF::RefPtr<JSC::JSGlobalData>::get at RefPtr.h:59
#1	0x101d6cc95 in JSC::JSGlobalObject::globalData at JSGlobalObject.h:279
#2	0x101ce66e9 in JSC::Parser::parse<JSC::FunctionBodyNode> at Parser.h:87
#3	0x101cdee51 in JSC::FunctionExecutable::compileForCallInternal at Executable.cpp:181
#4	0x101c78d00 in JSC::FunctionExecutable::compileForCall at Executable.h:315
#5	0x101cf1bae in JSC::Interpreter::executeCall at Interpreter.cpp:795
#6	0x101ca909f in JSC::call at CallData.cpp:38
#7	0x101ceb580 in JSC::functionProtoFuncCall at FunctionPrototype.cpp:147
#8	0x5711998001aa in ??
#9	0x101cf6a86 in JSC::JITCode::execute at JITCode.h:77
#10	0x101cf2bf1 in JSC::Interpreter::execute at Interpreter.cpp:746
#11	0x101cc26c7 in JSC::evaluate at Completion.cpp:63
#12	0x10310b4a0 in WebCore::JSMainThreadExecState::evaluate at JSMainThreadExecState.h:54
Comment 2 Geoffrey Garen 2010-10-28 11:55:17 PDT
<rdar://problem/8606066>
Comment 3 Zoltan Herczeg 2010-10-28 15:00:40 PDT
Interpreter::executeCall

    CallFrame* newCallFrame = CallFrame::create(oldEnd);
    size_t dst = 0;
    newCallFrame->r(0) = thisValue;
    ArgList::const_iterator end = args.end();
    for (ArgList::const_iterator it = args.begin(); it != end; ++it)
        newCallFrame->r(++dst) = *it;

oldEnd < callFrame, so newCallFrame->r(...) overwrites callframe fields. I will continue the debugging tomorrow morning. Hopefully I could find a fix.
Comment 4 Zoltan Herczeg 2010-10-29 09:33:24 PDT
Hi guys,

actually I am little confused with this bug as I don't exactly remember how JS call system should work, and I need a little help.

This is a simplified example:

function g() {
    Function.prototype.call.apply(g, arguments);
}

g(0, 0, 0)

The byte-code of function g():

[   0] enter
[   1] init_lazy_reg     r1
[   3] init_lazy_reg     r0
[   5] resolve_global    r2, Function(@id0)
[  10] get_by_id         r2, r2, prototype(@id1)
[  18] get_by_id         r2, r2, call(@id2)
[  26] get_by_id         r3, r2, apply(@id3)
[  34] jneq_ptr          r3, r-1220017856, 22(->56)
[  38] mov               r4, r2
[  41] get_global_var    r6, -6
[  44] mov               r7, r1
[  47] load_varargs              r5, r7
[  50] call_varargs      r4, r5, 12
[  54] jmp               17(->71)
[  56] mov               r4, r2
[  59] get_global_var    r5, -6
[  62] create_arguments  r1
[  64] mov               r6, r1
[  67] call              r3, 3, 13
[  71] ret               undefined(@k0)

Before the "enter", the RegisterFile is extended, since the 'g' expects 0 parameter and got 3. Thus op_call_arityCheck() set the registerFile->m_end to 0x..100 (memory returned by mmap is always page aligned, thus the last 3 digit is always the same). As far as I remember, this should be the end of the memory used by the JS functon, shouldn't it?

The emit_op_load_varargs does not call the helper-stub function, it just extend %edi (call frame ptr) to 0x...118, which is 3 registers beyond the end of the registerFile, and should be never written by JIT code, shoudn't it?

call_varargs pass %edi as the call frame, and in Interpreter::executeCall oldEnd still points to 0x..100, and the callFrame to 0x...118. (I think an assert would be useful here).

    for (ArgList::const_iterator it = args.begin(); it != end; ++it)
        newCallFrame->r(++dst) = *it;

The 'for' above overwrites callFrame members (still in Interpreter::executeCall).

As far as I remember, registerFile->m_end points after the last available register, and no writes should happen after it. Thus I would blame emit_op_load_varargs at the moment, but I would be curious about your opinion.
Comment 5 Oliver Hunt 2010-11-03 14:16:37 PDT
I don't see any error in load_varargs, i see the badness in call_varargs, the callframe register is extended to include the registerOffset, but loadvarargs has not bounds checked that extension, only the space for arguments.
Comment 6 Oliver Hunt 2010-11-03 15:09:01 PDT
I have a fix, just building webkit
Comment 7 Oliver Hunt 2010-11-03 16:17:40 PDT
Created attachment 72882 [details]
Patch
Comment 8 Oliver Hunt 2010-11-03 16:43:07 PDT
Committed r71280: <http://trac.webkit.org/changeset/71280>
Comment 9 Charles L 2010-11-05 20:33:25 PDT
Thank you!