ArityFixup should adjust SP first
authorutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 1 Feb 2017 11:29:25 +0000 (11:29 +0000)
committerutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 1 Feb 2017 11:29:25 +0000 (11:29 +0000)
https://bugs.webkit.org/show_bug.cgi?id=167239

Reviewed by Michael Saboff.

JSTests:

Significantly large arity fixup reliably causes this crash.

* stress/arity-fixup-should-not-touch-stack-area-below-sp.js: Added.

Source/JavaScriptCore:

Arity fixup extends the stack and copy/fill the stack with
the values. At that time, we accidentally read/write stack
space below the stack pointer. As a result, we touch the area
of the stack space below the x64 red zone. These areas are unsafe.
OS may corrupt this space when constructing a signal stack.
The Linux kernel could not populate the pages for this space
and causes segmentation fault. This patch changes the stack
pointer before performing the arity fixup.

* jit/ThunkGenerators.cpp:
(JSC::arityFixupGenerator):
* llint/LowLevelInterpreter32_64.asm:
* llint/LowLevelInterpreter64.asm:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@211479 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/stress/arity-fixup-should-not-touch-stack-area-below-sp.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/jit/ThunkGenerators.cpp
Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm
Source/JavaScriptCore/llint/LowLevelInterpreter64.asm

index 9b2511d..b40153b 100644 (file)
@@ -1,3 +1,14 @@
+2017-02-01  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        ArityFixup should adjust SP first
+        https://bugs.webkit.org/show_bug.cgi?id=167239
+
+        Reviewed by Michael Saboff.
+
+        Significantly large arity fixup reliably causes this crash.
+
+        * stress/arity-fixup-should-not-touch-stack-area-below-sp.js: Added.
+
 2017-01-31  Filip Pizlo  <fpizlo@apple.com>
 
         Move slow-running microbenchmarks out of JSTests/microbenchmarks
diff --git a/JSTests/stress/arity-fixup-should-not-touch-stack-area-below-sp.js b/JSTests/stress/arity-fixup-should-not-touch-stack-area-below-sp.js
new file mode 100644 (file)
index 0000000..afa2564
--- /dev/null
@@ -0,0 +1,5 @@
+var args = "y,".repeat(30000);
+var g = Function(args, "return 0");
+for (var i = 0; i < 1e3; ++i) {
+    g();
+}
index a2f5e0d..a298fd5 100644 (file)
@@ -1,3 +1,24 @@
+2017-02-01  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        ArityFixup should adjust SP first
+        https://bugs.webkit.org/show_bug.cgi?id=167239
+
+        Reviewed by Michael Saboff.
+
+        Arity fixup extends the stack and copy/fill the stack with
+        the values. At that time, we accidentally read/write stack
+        space below the stack pointer. As a result, we touch the area
+        of the stack space below the x64 red zone. These areas are unsafe.
+        OS may corrupt this space when constructing a signal stack.
+        The Linux kernel could not populate the pages for this space
+        and causes segmentation fault. This patch changes the stack
+        pointer before performing the arity fixup.
+
+        * jit/ThunkGenerators.cpp:
+        (JSC::arityFixupGenerator):
+        * llint/LowLevelInterpreter32_64.asm:
+        * llint/LowLevelInterpreter64.asm:
+
 2017-01-31  Filip Pizlo  <fpizlo@apple.com>
 
         Make verifyEdge a RELEASE_ASSERT
index 5d4b0be..9a65506 100644 (file)
@@ -440,6 +440,15 @@ MacroAssemblerCodeRef arityFixupGenerator(VM* vm)
 
     jit.neg64(JSInterfaceJIT::argumentGPR0);
 
+    // Adjust call frame register and stack pointer to account for missing args.
+    // We need to change the stack pointer first before performing copy/fill loops.
+    // This stack space below the stack pointer is considered unsed by OS. Therefore,
+    // OS may corrupt this space when constructing a signal stack.
+    jit.move(JSInterfaceJIT::argumentGPR0, extraTemp);
+    jit.lshift64(JSInterfaceJIT::TrustedImm32(3), extraTemp);
+    jit.addPtr(extraTemp, JSInterfaceJIT::callFrameRegister);
+    jit.addPtr(extraTemp, JSInterfaceJIT::stackPointerRegister);
+
     // Move current frame down argumentGPR0 number of slots
     JSInterfaceJIT::Label copyLoop(jit.label());
     jit.load64(JSInterfaceJIT::regT3, extraTemp);
@@ -455,12 +464,6 @@ MacroAssemblerCodeRef arityFixupGenerator(VM* vm)
     jit.addPtr(JSInterfaceJIT::TrustedImm32(8), JSInterfaceJIT::regT3);
     jit.branchAdd32(MacroAssembler::NonZero, JSInterfaceJIT::TrustedImm32(1), JSInterfaceJIT::argumentGPR2).linkTo(fillUndefinedLoop, &jit);
     
-    // Adjust call frame register and stack pointer to account for missing args
-    jit.move(JSInterfaceJIT::argumentGPR0, extraTemp);
-    jit.lshift64(JSInterfaceJIT::TrustedImm32(3), extraTemp);
-    jit.addPtr(extraTemp, JSInterfaceJIT::callFrameRegister);
-    jit.addPtr(extraTemp, JSInterfaceJIT::stackPointerRegister);
-
     done.link(&jit);
 
 #  if CPU(X86_64)
index e15ae38..d9fed6b 100644 (file)
@@ -613,6 +613,10 @@ macro functionArityCheck(doneLabel, slowPath)
     // Move frame up t1 slots
     negi t1
     move cfr, t3
+    move t1, t0
+    lshiftp 3, t0
+    addp t0, cfr
+    addp t0, sp
 .copyLoop:
     loadi PayloadOffset[t3], t0
     storei t0, PayloadOffset[t3, t1, 8]
@@ -631,9 +635,6 @@ macro functionArityCheck(doneLabel, slowPath)
     addp 8, t3
     baddinz 1, t2, .fillLoop
 
-    lshiftp 3, t1
-    addp t1, cfr
-    addp t1, sp
 .continue:
     # Reload CodeBlock and PC, since the slow_path clobbered it.
     loadp CodeBlock[cfr], t1
index 28101f1..0881d37 100644 (file)
@@ -525,6 +525,10 @@ macro functionArityCheck(doneLabel, slowPath)
     move cfr, t3
     subp CalleeSaveSpaceAsVirtualRegisters * 8, t3
     addi CalleeSaveSpaceAsVirtualRegisters, t2
+    move t1, t0
+    lshiftp 3, t0
+    addp t0, cfr
+    addp t0, sp
 .copyLoop:
     loadq [t3], t0
     storeq t0, [t3, t1, 8]
@@ -539,10 +543,6 @@ macro functionArityCheck(doneLabel, slowPath)
     addp 8, t3
     baddinz 1, t2, .fillLoop
 
-    lshiftp 3, t1
-    addp t1, cfr
-    addp t1, sp
-
 .continue:
     # Reload CodeBlock and reset PC, since the slow_path clobbered them.
     loadp CodeBlock[cfr], t1