Bug 184883 – Correctly detect string overflow when using the 'Function' constructor

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Robin Morisset]

<rdar://problem/36320331>

[Robin Morisset]

Created attachment 338582 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 338582 [details]
Patch

Clearing flags on attachment: 338582

Committed r231197: <https://trac.webkit.org/changeset/231197>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

The test added with this change is failing on the 32-bit JSC bot:

** The following JSC stress test failures have been introduced:
	slowMicrobenchmarks.yaml/slowMicrobenchmarks/function-constructor-with-huge-strings.js.default
	slowMicrobenchmarks.yaml/slowMicrobenchmarks/function-constructor-with-huge-strings.js.no-cjit

https://build.webkit.org/builders/Apple%20High%20Sierra%2032-bit%20JSC%20%28BuildAndTest%29/builds/1802

[Ryan Haddad]

Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x00000000bbadbeef
Exception Note:        EXC_CORPSE_NOTIFY

Termination Signal:    Segmentation fault: 11
Termination Reason:    Namespace SIGNAL, Code 0xb
Terminating Process:   exc handler [0]

VM Regions Near 0xbbadbeef:
    Stack                  00000000b084d000-00000000b08ce000 [  516K] rw-/rwx SM=COW  
--> 
    Stack Guard            00000000bbfea000-00000000bbfeb000 [    4K] ---/rwx SM=NUL  

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x003651e6 bmalloc::Heap::allocateLarge(std::__1::unique_lock<bmalloc::Mutex>&, unsigned long, unsigned long) + 86 (Heap.cpp:596)
1   com.apple.JavaScriptCore      	0x0035856f bmalloc::Allocator::allocateLarge(unsigned long) + 127 (Allocator.cpp:172)
2   com.apple.JavaScriptCore      	0x00358859 bmalloc::Allocator::allocateSlowCase(unsigned long) + 345 (Allocator.cpp:199)
3   com.apple.JavaScriptCore      	0x002c3738 bmalloc::Allocator::allocate(unsigned long) + 72 (Allocator.h:91)
4   com.apple.JavaScriptCore      	0x00357ff6 bmalloc::Allocator::reallocate(void*, unsigned long) + 694 (Allocator.cpp:126)
5   com.apple.JavaScriptCore      	0x002c3a35 bmalloc::Cache::reallocate(bmalloc::HeapKind, void*, unsigned long) + 213 (Cache.h:111)
6   com.apple.JavaScriptCore      	0x002c2f31 bmalloc::api::realloc(void*, unsigned long, bmalloc::HeapKind) + 49 (bmalloc.h:69)
7   com.apple.JavaScriptCore      	0x00323a64 WTF::stringRealloc(void*, unsigned long) + 52 (StringMalloc.cpp:73)
8   com.apple.JavaScriptCore      	0x0031574f WTF::Ref<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> > WTF::StringImpl::reallocateInternal<unsigned char>(WTF::Ref<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> >&&, unsigned int, unsigned char*&) + 383 (StringImpl.cpp:224)
9   com.apple.JavaScriptCore      	0x003155b1 WTF::StringImpl::reallocate(WTF::Ref<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> >&&, unsigned int, unsigned char*&) + 177 (StringImpl.cpp:233)
10  com.apple.JavaScriptCore      	0x003105eb void WTF::StringBuilder::reallocateBuffer<unsigned char>(unsigned int) + 395 (StringBuilder.cpp:163)
11  com.apple.JavaScriptCore      	0x003128b5 unsigned char* WTF::StringBuilder::appendUninitializedSlow<unsigned char>(unsigned int) + 277 (StringBuilder.cpp:247)
12  com.apple.JavaScriptCore      	0x00311666 unsigned char* WTF::StringBuilder::appendUninitialized<unsigned char>(unsigned int) + 454 (StringBuilder.cpp:232)
13  com.apple.JavaScriptCore      	0x00311039 WTF::StringBuilder::tryAppend(unsigned char const*, unsigned int) + 169 (StringBuilder.cpp:320)
14  com.apple.JavaScriptCore      	0x0112b39c WTF::StringBuilder::tryAppend(WTF::StringView) + 92 (StringBuilder.h:121)
15  com.apple.JavaScriptCore      	0x0112efaf JSC::constructFunctionSkippingEvalEnabledCheck(JSC::ExecState*, JSC::JSGlobalObject*, JSC::ArgList const&, JSC::Identifier const&, JSC::SourceOrigin const&, WTF::String const&, WTF::TextPosition const&, int, JSC::FunctionConstructionMode, JSC::JSValue) + 1935 (FunctionConstructor.cpp:141)
16  com.apple.JavaScriptCore      	0x0112e7f3 JSC::constructFunction(JSC::ExecState*, JSC::JSGlobalObject*, JSC::ArgList const&, JSC::Identifier const&, JSC::SourceOrigin const&, WTF::String const&, WTF::TextPosition const&, JSC::FunctionConstructionMode, JSC::JSValue) + 499 (FunctionConstructor.cpp:75)
17  com.apple.JavaScriptCore      	0x011300d1 JSC::constructFunction(JSC::ExecState*, JSC::JSGlobalObject*, JSC::ArgList const&, JSC::FunctionConstructionMode, JSC::JSValue) + 273 (FunctionConstructor.cpp:222)
18  com.apple.JavaScriptCore      	0x0112e3ce JSC::constructWithFunctionConstructor(JSC::ExecState*) + 126 (FunctionConstructor.cpp:44)
19  ???                           	0xd1ea807b 0 + 3521806459
20  com.apple.JavaScriptCore      	0x0037e0ae llint_entry + 25064 (LowLevelInterpreter.asm:828)
21  com.apple.JavaScriptCore      	0x00377d10 vmEntryToJavaScript + 292 (LowLevelInterpreter.asm:555)
22  com.apple.JavaScriptCore      	0x00e38829 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 185 (JITCode.cpp:75)
23  com.apple.JavaScriptCore      	0x00dcfd93 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*) + 6995 (Interpreter.cpp:969)
24  com.apple.JavaScriptCore      	0x010fa8b2 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 626 (Completion.cpp:103)
25  jsc                           	0x000663f0 runWithOptions(GlobalObject*, CommandLine&, bool&) + 2576 (jsc.cpp:2359)
26  jsc                           	0x00037d8a jscmain(int, char**)::$_3::operator()(JSC::VM&, GlobalObject*, bool&) const + 58 (jsc.cpp:2764)
27  jsc                           	0x0001d87a int runJSC<jscmain(int, char**)::$_3>(CommandLine, bool, jscmain(int, char**)::$_3 const&) + 1258 (jsc.cpp:2665)
28  jsc                           	0x0001c010 jscmain(int, char**) + 192 (jsc.cpp:2760)
29  jsc                           	0x0001bf37 main + 55 (jsc.cpp:2190)
30  libdyld.dylib                 	0xa73f4611 start + 1

[Ryan Haddad]

Reverted r231197 for reason:

The test added with this change crashes on the 32-bit JSC bot.

Committed r231310: <https://trac.webkit.org/changeset/231310>

[Mark Lam]

Created attachment 353269 [details]
work in progress.

Let's try this on the EWS.

[EWS Watchlist]

Comment on attachment 353269 [details]
work in progress.

Attachment 353269 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9763167

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 353270 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 353269 [details]
work in progress.

Attachment 353269 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9763170

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 353271 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 353269 [details]
work in progress.

Attachment 353269 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9763149

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 353272 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Mark Lam]

Created attachment 353290 [details]
proposed patch.

[Saam Barati]

Comment on attachment 353290 [details]
proposed patch.

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

> Source/WTF/ChangeLog:40
> +           However, I was not able to get clang to export the explicitly instantiated
> +           instances (despite the methods having being decorated with WTF_EXPORT_PRIVATE).
> +           Since the StringBuilder is a transient object (not stored for a long time on
> +           the heap), and the runtime check of the boolean is not that costly compared
> +           to other work that StringBuilder routinely does (e.g. memcpy), using the
> +           new ConditionalCrashOnOverflow (which does a runtime check to determine to
> +           crash or not on overflow) is fine.

I don't think this is the end of the world, but it seems like this would be strictly better if you made it a template parameter. Can you at least file a bug? Not doing something because it's tricky to get it to compile correctly isn't a very convincing reason.

> Source/WTF/wtf/CheckedArithmetic.h:231
> +    typedef typename RemoveChecked<T>::CleanType CleanType;

nit: I'd use 'using' here:
using CleanType = typename RemoveChecked<T>::CleanType;

> Source/WTF/wtf/text/StringBuilder.cpp:40
> +    return std::max(requiredLength, std::max(minimumCapacity, std::min(capacity * 2, static_cast<unsigned>(roundUpToMultipleOf<16>(String::MaxLength)))));

Why 16? Just because that's the minimum? If so, you should use it by name.

Why not just round to multiple of 2?

> Source/WTF/wtf/text/StringBuilder.cpp:43
> +

please revert

[Mark Lam]

Comment on attachment 353290 [details]
proposed patch.

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

Thanks for the review.

>> Source/WTF/ChangeLog:40
>> +           crash or not on overflow) is fine.
> 
> I don't think this is the end of the world, but it seems like this would be strictly better if you made it a template parameter. Can you at least file a bug? Not doing something because it's tricky to get it to compile correctly isn't a very convincing reason.

Oh I got it to compile.  It's the linking that is the issue.  I've spent 1 day googling for a solution to this issue, but couldn't find one.  There were other folks who also encountered the issue, and there were folks who came forth with supposed solutions for clang on linux, but no one verified a solution for Mac.  The linux solution (which was to declare the equivalent of WTF_EXPORT_PRIVATE after the function declarations) did not work for me.  After that, I just decided to move on with this solution.  I'll file a bug.

>> Source/WTF/wtf/CheckedArithmetic.h:231
>> +    typedef typename RemoveChecked<T>::CleanType CleanType;
> 
> nit: I'd use 'using' here:
> using CleanType = typename RemoveChecked<T>::CleanType;

Will fix.

>> Source/WTF/wtf/text/StringBuilder.cpp:40
>> +    return std::max(requiredLength, std::max(minimumCapacity, std::min(capacity * 2, static_cast<unsigned>(roundUpToMultipleOf<16>(String::MaxLength)))));
> 
> Why 16? Just because that's the minimum? If so, you should use it by name.
> 
> Why not just round to multiple of 2?

I was thinking that a multiple of the minimum capacity is more appropriate than 2, but in the end, all I really care about here is that StringMax is INT_MAX which is 0x7fffffff, and that seems like a bad to pick as a max capacity.  So, 2 will actually work, or I can define a maxCapacity constant that is String::MaxLength + 1.  I'll go with this maxCapacity constant.

>> Source/WTF/wtf/text/StringBuilder.cpp:43
>> +
> 
> please revert

Will fix.

[Mark Lam]

Created attachment 353341 [details]
patch for landing.

[WebKit Commit Bot]

Comment on attachment 353341 [details]
patch for landing.

Clearing flags on attachment: 353341

Committed r237577: <https://trac.webkit.org/changeset/237577>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.