Bug 182705 – REGRESSION(225695) : com.apple.WebKit.WebContent at com.apple.JavaScriptCore: JSC::RegExp::match + 630 :: stack overflow

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

[Michael Saboff]

Looks like adding the YARR JIT 8K buffer as a stack variable in https://trac.webkit.org/changeset/225695/webkit (relanded in https://trac.webkit.org/changeset/225930/webkit) causes an out of stack crash.  This buffer should be moved to the owning VM.

Crashes look similar to:
Thread 0 Crashed ↩:: Dispatch queue: com.apple.main-thread
0   libsystem_kernel.dylib        	0x00007fff5c5f4afe __pthread_kill + 10
1   libsystem_pthread.dylib       	0x00007fff5c7b3150 pthread_kill + 333
2   libsystem_c.dylib             	0x00007fff5c55024d __abort + 144
3   libsystem_c.dylib             	0x00007fff5c550af8 __stack_chk_fail + 205
4   com.apple.JavaScriptCore      	0x00007fff370da396 JSC::RegExp::match(JSC::VM&, WTF::String const&, unsigned int) + 630
5   com.apple.JavaScriptCore      	0x00007fff37b1929b JSC::RegExpObject::matchInline(JSC::ExecState*, JSC::JSGlobalObject*, JSC::JSString*) + 235
6   com.apple.JavaScriptCore      	0x00007fff37b1cd6b JSC::regExpProtoFuncTestFast(JSC::ExecState*) + 267
7   ???                           	0x00005db07fa01178 0 + 103012636823928
8   ???                           	0x00005db08054fc08 0 + 103012648680456
9   ???                           	0x00005db08054e42e 0 + 103012648674350
10  ???                           	0x00005db08055139b 0 + 103012648686491
11  ???                           	0x00005db0803e53c2 0 + 103012647195586
12  ???                           	0x00005db0805271d3 0 + 103012648514003
13  ???                           	0x00005db0804f1c31 0 + 103012648295473
14  com.apple.JavaScriptCore      	0x00007fff372384be llint_entry + 28860
...

[Michael Saboff]

<rdar://problem/36940735>

[Michael Saboff]

Created attachment 333718 [details]
Patch

[Mark Lam]

Comment on attachment 333718 [details]
Patch

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

r=me with suggested improvements.

> Source/JavaScriptCore/runtime/RegExpInlines.h:122
> +        if (m_regExpJITCode.usesPatternContextBuffer()) {
> +            patternContextBuffer = vm.acquireRegExpPatternContexBuffer();
> +            patternContextBufferSize = VM::patternContextBufferSize;
> +        }
>  #define EXTRA_JIT_PARAMS  , patternContextBuffer, patternContextBufferSize

This seems like a prime candidate for using the RAII pattern.  How about something like this:

    RegExpPatternContextBuffer buffer(vm);
    #define EXTRA_JIT_PARAMS  , buffer.buffer(), buffer.size()

You can either make RegExpPatternContextBuffer an embedded class in VM, or make it a friend of VM so that it can do the real work.

> Source/JavaScriptCore/runtime/RegExpInlines.h:135
> +#if ENABLE(YARR_JIT_ALL_PARENS_EXPRESSIONS)
> +        if (m_regExpJITCode.usesPatternContextBuffer())
> +            vm.releaseRegExpPatternContexBuffer();
> +#endif

This can be handled automatically by the RAII object.

> Source/JavaScriptCore/yarr/YarrJIT.h:102
> +#endif
> +
> +#if ENABLE(YARR_JIT_ALL_PARENS_EXPRESSIONS)

Why is this needed?

[Mark Lam]

(In reply to Mark Lam from comment #3)
> Comment on attachment 333718 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333718&action=review
> 
> r=me with suggested improvements.
> 
> > Source/JavaScriptCore/runtime/RegExpInlines.h:122
> > +        if (m_regExpJITCode.usesPatternContextBuffer()) {
> > +            patternContextBuffer = vm.acquireRegExpPatternContexBuffer();
> > +            patternContextBufferSize = VM::patternContextBufferSize;
> > +        }
> >  #define EXTRA_JIT_PARAMS  , patternContextBuffer, patternContextBufferSize
> 
> This seems like a prime candidate for using the RAII pattern.  How about
> something like this:
> 
>     RegExpPatternContextBuffer buffer(vm);
>     #define EXTRA_JIT_PARAMS  , buffer.buffer(), buffer.size()

Better yet, just pass a reference to the RAII object:
    #define EXTRA_JIT_PARAMS  , buffer

[Michael Saboff]

Created attachment 333813 [details]
Patch updated after reviewer's comments

[Michael Saboff]

(In reply to Mark Lam from comment #3)
> Comment on attachment 333718 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333718&action=review
> 
> r=me with suggested improvements.
> 
> > Source/JavaScriptCore/runtime/RegExpInlines.h:122
> > +        if (m_regExpJITCode.usesPatternContextBuffer()) {
> > +            patternContextBuffer = vm.acquireRegExpPatternContexBuffer();
> > +            patternContextBufferSize = VM::patternContextBufferSize;
> > +        }
> >  #define EXTRA_JIT_PARAMS  , patternContextBuffer, patternContextBufferSize
> 
> This seems like a prime candidate for using the RAII pattern.  How about
> something like this:
> 
>     RegExpPatternContextBuffer buffer(vm);
>     #define EXTRA_JIT_PARAMS  , buffer.buffer(), buffer.size()
> 
> You can either make RegExpPatternContextBuffer an embedded class in VM, or
> make it a friend of VM so that it can do the real work.

I added a RAII class to RegExpInlines.h.

> > Source/JavaScriptCore/runtime/RegExpInlines.h:135
> > +#if ENABLE(YARR_JIT_ALL_PARENS_EXPRESSIONS)
> > +        if (m_regExpJITCode.usesPatternContextBuffer())
> > +            vm.releaseRegExpPatternContexBuffer();
> > +#endif
> 
> This can be handled automatically by the RAII object.
> 
> > Source/JavaScriptCore/yarr/YarrJIT.h:102
> > +#endif
> > +
> > +#if ENABLE(YARR_JIT_ALL_PARENS_EXPRESSIONS)
> 
> Why is this needed?

Removed.

[EWS Watchlist]

Attachment 333813 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/RegExpInlines.h:147:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/RegExpInlines.h:257:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 333813 [details]
Patch updated after reviewer's comments

r=me if EWS bots show no issues.

[WebKit Commit Bot]

Comment on attachment 333813 [details]
Patch updated after reviewer's comments

Clearing flags on attachment: 333813

Committed r228481: <https://trac.webkit.org/changeset/228481>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.