Bug 177293 – [Win64] Crashes in Yarr JIT compiled code

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Fujii Hironori]

[Win64] Crashes in Yarr JIT compiled code

WinCairo port, 64bitk, Debug build, trunk@222298, MiniBrowser

1) Start MiniBrowser
2) Load http://google.com/
3) Crash

Callstack:

> 000001c500001b61()	Unknown
> 0000003c51cfc6b0()	Unknown
> JavaScriptCore.dll!JSC::Yarr::YarrCodeBlock::execute(const unsigned char * input, unsigned int start, unsigned int length, int * output) Line 87	C++
> JavaScriptCore.dll!JSC::RegExp::matchInline<WTF::Vector<int,32,WTF::CrashOnOverflow,16,WTF::FastMalloc> >(JSC::VM & vm, const WTF::String & s, unsigned int startOffset, WTF::Vector<int,32,WTF::CrashOnOverflow,16,WTF::FastMalloc> & ovector) Line 115	C++
> JavaScriptCore.dll!JSC::createRegExpMatchesArray(JSC::VM & vm, JSC::JSGlobalObject * globalObject, JSC::JSString * input, const WTF::String & inputValue, JSC::RegExp * regExp, unsigned int startOffset, JSC::MatchResult & result) Line 66	C++
> JavaScriptCore.dll!JSC::RegExpObject::execInline(JSC::ExecState * exec, JSC::JSGlobalObject * globalObject, JSC::JSString * string) Line 86	C++
> JavaScriptCore.dll!JSC::RegExpObject::exec(JSC::ExecState * exec, JSC::JSGlobalObject * globalObject, JSC::JSString * string) Line 170	C++
> JavaScriptCore.dll!JSC::regExpProtoFuncExec(JSC::ExecState * exec) Line 130	C++
> [External Code]	


code:

> 000002293D8B1A22  xor         ecx,ecx  
> 000002293D8B1A24  cmp         r8d,r9d  
> 000002293D8B1A27  je          000002293D8B1A51  
> 000002293D8B1A2D  movzx       eax,byte ptr [rdx+r8]  
> 000002293D8B1A32  mov         r11,7FFB5E1BAF00h  
> 000002293D8B1A3C  cmp         byte ptr [r11+rax],0  
> 000002293D8B1A41  jne         000002293D8B1A51  
> 000002293D8B1A47  inc         r8d  
> 000002293D8B1A4A  inc         ecx  
> 000002293D8B1A4C  jmp         000002293D8B1A24  
> 000002293D8B1A51  mov         qword ptr [rsp+8],rcx  
> 000002293D8B1A56  mov         dword ptr [r10+0Ch],r8d  
> 000002293D8B1A5A  add         rsp,40h  
> 000002293D8B1A5E  mov         eax,dword ptr [r10]  
> 000002293D8B1A61  mov         dword ptr [r10+4],r8d  
> 000002293D8B1A65  mov         rdx,r8  
> 000002293D8B1A68  mov         r11,2297D942110h  
> 000002293D8B1A72  mov         byte ptr [r11],0  
> 000002293D8B1A76  mov         qword ptr [rcx],rax          <==rip
> 000002293D8B1A79  mov         qword ptr [rcx+8],rdx  
> 000002293D8B1A7D  mov         rax,rcx  
> 000002293D8B1A80  pop         rbp  
> 000002293D8B1A81  ret  

registers:

> RAX = 000000000000002E RBX = 0000000000000001 RCX = 0000000000000004 RDX = 000000000000003A
> RSI = 0000004559FEC490 RDI = 0000004559FEBF58 R8  = 000000000000003A R9  = 000000000000004E
> R10 = 0000004559FEC060 R11 = 000002297D942110 R12 = 00000000003405FA R13 = 000002290223E4A8
> R14 = FFFF000000000000 R15 = FFFF000000000002
> RIP = 000002293D8B1A76 RSP = 0000004559FEBEE0 RBP = 0000004559FEBEE0 EFL = 00010202 


This is the code generated by generateReturn().
rcx was 0x4. But, it should be the address where the return values are stored.

[Fujii Hironori]

I can avoid this annoying crash by setting a env var.

> set JSC_useRegExpJIT=0

[Fujii Hironori]

Created attachment 321415 [details]
WIP patch

I don't know how to fix correctly. This WIP patch just saves rcx.

[Yusuke Suzuki]

Comment on attachment 321415 [details]
WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321415&action=review

Looks good to me.
Could you upload the patch with ChangeLog and test?

> Source/JavaScriptCore/yarr/YarrJIT.cpp:2855
> +        push(X86Registers::ecx);

It would be good to add comment here that it is the pointer to the result in x64 Windows.

[Fujii Hironori]

(In reply to Yusuke Suzuki from comment #3)
> Looks good to me.
> Could you upload the patch with ChangeLog and test?

Thank you for your feedback.
I don't think a new test is needed because existing LayoutTests (ex. js/string_replace_regexp.html) can reproduce the crash.
I will create a patch.

> > Source/JavaScriptCore/yarr/YarrJIT.cpp:2855
> > +        push(X86Registers::ecx);
> 
> It would be good to add comment here that it is the pointer to the result in
> x64 Windows.

Agreed.

[Fujii Hironori]

Created attachment 321525 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 321525 [details]
Patch

r=me

[Yusuke Suzuki]

BTW, I'm not sure why MatchResult needs 128 bytes in 64bit environments.
Do we really need size_t? I guess we can use `unsigned` pair for this since the length of StringImpl is unsigned. But this is just my guess.
Anyway, the above refactoring is separated from this patch. I'm ok to land it.

[WebKit Commit Bot]

Comment on attachment 321525 [details]
Patch

Rejecting attachment 321525 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 321525, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
d/mechanize/_urllib2_fork.py", line 332, in _call_chain
    result = func(*args)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1170, in https_open
    return self.do_open(conn_factory, req)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1118, in do_open
    raise URLError(err)
urllib2.URLError: <urlopen error ('_ssl.c:574: The handshake operation timed out',)>

Full output: http://webkit-queues.webkit.org/results/4625938

[WebKit Commit Bot]

Comment on attachment 321525 [details]
Patch

Clearing flags on attachment: 321525

Committed r222417: <http://trac.webkit.org/changeset/222417>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34693120>