Bug 172316

Summary: We overwrite the callee save space on the stack when throwing stack overflow from wasm
Product: WebKit Reporter: Ryan Haddad <ryanhaddad>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, jfbastien, keith_miller, mark.lam, msaboff, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Crash log
none
patch
none
patch
mark.lam: review-
patch
mark.lam: review+
patch for landing none

Ryan Haddad
Reported 2017-05-18 14:40:37 PDT
** The following JSC stress test failures have been introduced: wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-no-tls-context https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/1494
Attachments
Crash log (61.72 KB, text/plain)
2017-05-18 14:41 PDT, Ryan Haddad 		no flags
Details
patch (3.01 KB, patch)
2017-05-19 18:50 PDT, Saam Barati 		no flags
DetailsFormatted DiffDiff
patch (3.03 KB, patch)
2017-05-19 20:11 PDT, Saam Barati 		mark.lam: review-
DetailsFormatted DiffDiff
patch (3.03 KB, patch)
2017-05-20 15:46 PDT, Saam Barati 		mark.lam: review+
DetailsFormatted DiffDiff
patch for landing (3.02 KB, patch)
2017-05-21 13:48 PDT, Saam Barati 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Ryan Haddad
Comment 1 2017-05-18 14:41:08 PDT
Created attachment 310553 [details] Crash log
Saam Barati
Comment 2 2017-05-18 20:46:27 PDT
I’ll look into this tomorrow.
Saam Barati
Comment 3 2017-05-19 18:50:36 PDT
Created attachment 310746 [details] patch
Saam Barati
Comment 4 2017-05-19 20:11:48 PDT
Created attachment 310753 [details] patch
Mark Lam
Comment 5 2017-05-19 20:44:51 PDT
Comment on attachment 310753 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=310753&action=review > Source/JavaScriptCore/ChangeLog:23 > + move fp, sp > + sub maxNumCalleeSaveSpace, sp I think you want to use a ternary form of sub, or use a temp register. You don't want to pop sp all the way to fp and then reserving back some space. This is because a signal handler may fire at that moment between the 2 instructions, and overwrite anything below the sp. Instead, you should ensure that the sp never drops that low.
Saam Barati
Comment 6 2017-05-20 15:29:42 PDT
Sounds good.
Saam Barati
Comment 7 2017-05-20 15:46:21 PDT
Created attachment 310782 [details] patch
Mark Lam
Comment 8 2017-05-20 22:10:05 PDT
Comment on attachment 310782 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=310782&action=review r=me with ChangeLog fixed. > Source/JavaScriptCore/ChangeLog:22 > + add maxNumCalleeSaveSpace, fp, sp I think you mean -maxNumCalleeSaveSpace here. > Source/JavaScriptCore/wasm/WasmThunks.cpp:98 > + jit.addPtr(CCallHelpers::TrustedImm32(-stackSpace), GPRInfo::callFrameRegister, MacroAssembler::stackPointerRegister); I was worried that x86_64 will reduce this to a pair of binary ops (move fp, sp; add -X, sp), but I see that it really does perform a ternary operation using an lea instruction instead. So, all is well.
Saam Barati
Comment 9 2017-05-21 13:48:50 PDT
Created attachment 310817 [details] patch for landing
WebKit Commit Bot
Comment 10 2017-05-21 14:30:00 PDT
Comment on attachment 310817 [details] patch for landing Clearing flags on attachment: 310817 Committed r217198: <http://trac.webkit.org/changeset/217198>
WebKit Commit Bot
Comment 11 2017-05-21 14:30:02 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.