Attachment 245918 Details for Bug 141183 – the patch

[patch] the patch

blah.patch (text/plain), 20.18 KB, created by Filip Pizlo on 2015-02-02 18:53:07 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Filip Pizlo

Created: 2015-02-02 18:53:07 PST

Size: 20.18 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 179533)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,60 @@
>+2015-02-02  Filip Pizlo  <fpizlo@apple.com>
>+
>+        arguments[-1] should have well-defined behavior
>+        https://bugs.webkit.org/show_bug.cgi?id=141183
>+
>+        Reviewed by NOBODY (OOPS!).
>+        
>+        According to JSC's internal argument numbering, 0 is "this" and 1 is the first argument.
>+        In the "arguments[i]" expression, "this" is not accessible and i = 0 refers to the first
>+        argument. Previously we handled the bounds check in "arguments[i]" - where "arguments" is
>+        statically known to be the current function's arguments object - as follows:
>+        
>+            add 1, i
>+            branchAboveOrEqual i, callFrame.ArgumentCount, slowPath
>+        
>+        The problem with this is that if i = -1, this passes the test, and we end up accessing
>+        what would be the "this" argument slot. That's wrong, since we should really be bottoming
>+        out in arguments["-1"], which is usually undefined but could be anything. It's even worse
>+        if the function is inlined or if we're in a constructor - in that case the "this" slot
>+        could be garbage.
>+        
>+        It turns out that we had this bug in all of our engines.
>+        
>+        This fixes the issue by changing the algorithm to:
>+        
>+            load32 callFrame.ArgumentCount, tmp
>+            sub 1, tmp
>+            branchAboveOrEqual i, tmp, slowPath
>+        
>+        In some engines, we would have used the modified "i" (the one that had 1 added to it) for
>+        the subsequent argument load; since we don't do this anymore I also had to change some of
>+        the offsets on the BaseIndex arguments load.
>+        
>+        This also includes tests that are written in such a way as to get coverage on LLInt and
>+        Baseline JIT (get-my-argument-by-val-wrap-around-no-warm-up), DFG and FTL
>+        (get-my-argument-by-val-wrap-around), and DFG when we're being paranoid about the user
>+        overwriting the "arguments" variable (get-my-argument-by-val-safe-wrap-around).
>+
>+        * dfg/DFGSpeculativeJIT32_64.cpp:
>+        (JSC::DFG::SpeculativeJIT::compile):
>+        * dfg/DFGSpeculativeJIT64.cpp:
>+        (JSC::DFG::SpeculativeJIT::compile):
>+        * ftl/FTLLowerDFGToLLVM.cpp:
>+        (JSC::FTL::LowerDFGToLLVM::compileGetMyArgumentByVal):
>+        * jit/JITOpcodes.cpp:
>+        (JSC::JIT::emit_op_get_argument_by_val):
>+        * jit/JITOpcodes32_64.cpp:
>+        (JSC::JIT::emit_op_get_argument_by_val):
>+        * llint/LowLevelInterpreter32_64.asm:
>+        * llint/LowLevelInterpreter64.asm:
>+        * tests/stress/get-my-argument-by-val-safe-wrap-around.js: Added.
>+        (foo):
>+        * tests/stress/get-my-argument-by-val-wrap-around-no-warm-up.js: Added.
>+        (foo):
>+        * tests/stress/get-my-argument-by-val-wrap-around.js: Added.
>+        (foo):
>+
> 2015-02-02  Filip Pizlo  <fpizlo@apple.com>
> 
>         [FTL] inlined GetMyArgumentByVal with no arguments passed causes instant crash
>Index: Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp	(revision 179531)
>+++ Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp	(working copy)
>@@ -4347,22 +4347,19 @@ void SpeculativeJIT::compile(Node* node)
>                     TrustedImm32(JSValue::EmptyValueTag)));
>         }
>             
>-        m_jit.add32(TrustedImm32(1), indexGPR, resultPayloadGPR);
>-            
>         if (node->origin.semantic.inlineCallFrame) {
>             speculationCheck(
>                 Uncountable, JSValueRegs(), 0,
>                 m_jit.branch32(
>                     JITCompiler::AboveOrEqual,
>-                    resultPayloadGPR,
>+                    indexGPR,
>                     Imm32(node->origin.semantic.inlineCallFrame->arguments.size())));
>         } else {
>+            m_jit.load32(JITCompiler::payloadFor(JSStack::ArgumentCount), resultPayloadGPR);
>+            m_jit.sub32(TrustedImm32(1), resultPayloadGPR);
>             speculationCheck(
>                 Uncountable, JSValueRegs(), 0,
>-                m_jit.branch32(
>-                    JITCompiler::AboveOrEqual,
>-                    resultPayloadGPR,
>-                    JITCompiler::payloadFor(JSStack::ArgumentCount)));
>+                m_jit.branch32(JITCompiler::AboveOrEqual, indexGPR, resultPayloadGPR));
>         }
>         
>         JITCompiler::JumpList slowArgument;
>@@ -4399,13 +4396,13 @@ void SpeculativeJIT::compile(Node* node)
> 
>         m_jit.load32(
>             JITCompiler::BaseIndex(
>-                GPRInfo::callFrameRegister, resultPayloadGPR, JITCompiler::TimesEight,
>-                m_jit.offsetOfArgumentsIncludingThis(node->origin.semantic) + OBJECT_OFFSETOF(EncodedValueDescriptor, asBits.tag)),
>+                GPRInfo::callFrameRegister, indexGPR, JITCompiler::TimesEight,
>+                m_jit.offsetOfArgumentsIncludingThis(node->origin.semantic) + OBJECT_OFFSETOF(EncodedValueDescriptor, asBits.tag) + sizeof(EncodedJSValue)),
>             resultTagGPR);
>         m_jit.load32(
>             JITCompiler::BaseIndex(
>-                GPRInfo::callFrameRegister, resultPayloadGPR, JITCompiler::TimesEight,
>-                m_jit.offsetOfArgumentsIncludingThis(node->origin.semantic) + OBJECT_OFFSETOF(EncodedValueDescriptor, asBits.payload)),
>+                GPRInfo::callFrameRegister, indexGPR, JITCompiler::TimesEight,
>+                m_jit.offsetOfArgumentsIncludingThis(node->origin.semantic) + OBJECT_OFFSETOF(EncodedValueDescriptor, asBits.payload) + sizeof(EncodedJSValue)),
>             resultPayloadGPR);
>             
>         slowArgument.link(&m_jit);
>@@ -4427,19 +4424,17 @@ void SpeculativeJIT::compile(Node* node)
>                 JITCompiler::tagFor(m_jit.graph().machineArgumentsRegisterFor(node->origin.semantic)),
>                 TrustedImm32(JSValue::EmptyValueTag)));
>         
>-        m_jit.add32(TrustedImm32(1), indexGPR, resultPayloadGPR);
>         if (node->origin.semantic.inlineCallFrame) {
>             slowPath.append(
>                 m_jit.branch32(
>                     JITCompiler::AboveOrEqual,
>-                    resultPayloadGPR,
>+                    indexGPR,
>                     Imm32(node->origin.semantic.inlineCallFrame->arguments.size())));
>         } else {
>+            m_jit.load32(JITCompiler::payloadFor(JSStack::ArgumentCount), resultPayloadGPR);
>+            m_jit.sub32(TrustedImm32(1), resultPayloadGPR);
>             slowPath.append(
>-                m_jit.branch32(
>-                    JITCompiler::AboveOrEqual,
>-                    resultPayloadGPR,
>-                    JITCompiler::payloadFor(JSStack::ArgumentCount)));
>+                m_jit.branch32(JITCompiler::AboveOrEqual, indexGPR, resultPayloadGPR));
>         }
>         
>         JITCompiler::JumpList slowArgument;
>@@ -4475,13 +4470,13 @@ void SpeculativeJIT::compile(Node* node)
> 
>         m_jit.load32(
>             JITCompiler::BaseIndex(
>-                GPRInfo::callFrameRegister, resultPayloadGPR, JITCompiler::TimesEight,
>-                m_jit.offsetOfArgumentsIncludingThis(node->origin.semantic) + OBJECT_OFFSETOF(EncodedValueDescriptor, asBits.tag)),
>+                GPRInfo::callFrameRegister, indexGPR, JITCompiler::TimesEight,
>+                m_jit.offsetOfArgumentsIncludingThis(node->origin.semantic) + OBJECT_OFFSETOF(EncodedValueDescriptor, asBits.tag) + sizeof(EncodedJSValue)),
>             resultTagGPR);
>         m_jit.load32(
>             JITCompiler::BaseIndex(
>-                GPRInfo::callFrameRegister, resultPayloadGPR, JITCompiler::TimesEight,
>-                m_jit.offsetOfArgumentsIncludingThis(node->origin.semantic) + OBJECT_OFFSETOF(EncodedValueDescriptor, asBits.payload)),
>+                GPRInfo::callFrameRegister, indexGPR, JITCompiler::TimesEight,
>+                m_jit.offsetOfArgumentsIncludingThis(node->origin.semantic) + OBJECT_OFFSETOF(EncodedValueDescriptor, asBits.payload) + sizeof(EncodedJSValue)),
>             resultPayloadGPR);
>         
>         if (node->origin.semantic.inlineCallFrame) {
>Index: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp	(revision 179531)
>+++ Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp	(working copy)
>@@ -4394,21 +4394,19 @@ void SpeculativeJIT::compile(Node* node)
>                         m_jit.graph().machineArgumentsRegisterFor(node->origin.semantic))));
>         }
> 
>-        m_jit.add32(TrustedImm32(1), indexGPR, resultGPR);
>         if (node->origin.semantic.inlineCallFrame) {
>             speculationCheck(
>                 Uncountable, JSValueRegs(), 0,
>                 m_jit.branch32(
>                     JITCompiler::AboveOrEqual,
>-                    resultGPR,
>-                    Imm32(node->origin.semantic.inlineCallFrame->arguments.size())));
>+                    indexGPR,
>+                    Imm32(node->origin.semantic.inlineCallFrame->arguments.size() - 1)));
>         } else {
>+            m_jit.load32(JITCompiler::payloadFor(JSStack::ArgumentCount), resultGPR);
>+            m_jit.sub32(TrustedImm32(1), resultGPR);
>             speculationCheck(
>                 Uncountable, JSValueRegs(), 0,
>-                m_jit.branch32(
>-                    JITCompiler::AboveOrEqual,
>-                    resultGPR,
>-                    JITCompiler::payloadFor(JSStack::ArgumentCount)));
>+                m_jit.branch32(JITCompiler::AboveOrEqual, indexGPR, resultGPR));
>         }
> 
>         JITCompiler::JumpList slowArgument;
>@@ -4438,11 +4436,9 @@ void SpeculativeJIT::compile(Node* node)
>         }
>         slowArgumentOutOfBounds.link(&m_jit);
> 
>-        m_jit.signExtend32ToPtr(resultGPR, resultGPR);
>-            
>         m_jit.load64(
>             JITCompiler::BaseIndex(
>-                GPRInfo::callFrameRegister, resultGPR, JITCompiler::TimesEight, m_jit.offsetOfArgumentsIncludingThis(node->origin.semantic)),
>+                GPRInfo::callFrameRegister, indexGPR, JITCompiler::TimesEight, m_jit.offsetOfArgumentsIncludingThis(node->origin.semantic) + sizeof(EncodedJSValue)),
>             resultGPR);
> 
>         slowArgument.link(&m_jit);
>@@ -4463,19 +4459,17 @@ void SpeculativeJIT::compile(Node* node)
>                 JITCompiler::addressFor(
>                     m_jit.graph().machineArgumentsRegisterFor(node->origin.semantic))));
>         
>-        m_jit.add32(TrustedImm32(1), indexGPR, resultGPR);
>         if (node->origin.semantic.inlineCallFrame) {
>             slowPath.append(
>                 m_jit.branch32(
>                     JITCompiler::AboveOrEqual,
>                     resultGPR,
>-                    Imm32(node->origin.semantic.inlineCallFrame->arguments.size())));
>+                    Imm32(node->origin.semantic.inlineCallFrame->arguments.size() - 1)));
>         } else {
>+            m_jit.load32(JITCompiler::payloadFor(JSStack::ArgumentCount), resultGPR);
>+            m_jit.sub32(TrustedImm32(1), resultGPR);
>             slowPath.append(
>-                m_jit.branch32(
>-                    JITCompiler::AboveOrEqual,
>-                    resultGPR,
>-                    JITCompiler::payloadFor(JSStack::ArgumentCount)));
>+                m_jit.branch32(JITCompiler::AboveOrEqual, indexGPR, resultGPR));
>         }
>         
>         JITCompiler::JumpList slowArgument;
>@@ -4505,11 +4499,9 @@ void SpeculativeJIT::compile(Node* node)
>         }
>         slowArgumentOutOfBounds.link(&m_jit);
> 
>-        m_jit.signExtend32ToPtr(resultGPR, resultGPR);
>-        
>         m_jit.load64(
>             JITCompiler::BaseIndex(
>-                GPRInfo::callFrameRegister, resultGPR, JITCompiler::TimesEight, m_jit.offsetOfArgumentsIncludingThis(node->origin.semantic)),
>+                GPRInfo::callFrameRegister, indexGPR, JITCompiler::TimesEight, m_jit.offsetOfArgumentsIncludingThis(node->origin.semantic) + sizeof(EncodedJSValue)),
>             resultGPR);
>         
>         if (node->origin.semantic.inlineCallFrame) {
>Index: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp
>===================================================================
>--- Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp	(revision 179531)
>+++ Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp	(working copy)
>@@ -1997,16 +1997,15 @@ private:
>         
>         CodeOrigin codeOrigin = m_node->origin.semantic;
>         
>-        LValue zeroBasedIndex = lowInt32(m_node->child1());
>-        LValue oneBasedIndex = m_out.add(zeroBasedIndex, m_out.int32One);
>+        LValue index = lowInt32(m_node->child1());
>         
>         LValue limit;
>         if (codeOrigin.inlineCallFrame)
>-            limit = m_out.constInt32(codeOrigin.inlineCallFrame->arguments.size());
>+            limit = m_out.constInt32(codeOrigin.inlineCallFrame->arguments.size() - 1);
>         else
>-            limit = m_out.load32(payloadFor(JSStack::ArgumentCount));
>+            limit = m_out.sub(m_out.load32(payloadFor(JSStack::ArgumentCount)), m_out.int32One);
>         
>-        speculate(Uncountable, noValue(), 0, m_out.aboveOrEqual(oneBasedIndex, limit));
>+        speculate(Uncountable, noValue(), 0, m_out.aboveOrEqual(index, limit));
>         
>         SymbolTable* symbolTable = m_graph.baselineCodeBlockFor(codeOrigin)->symbolTable();
>         if (symbolTable->slowArguments()) {
>@@ -2032,7 +2031,7 @@ private:
>             base = addressFor(virtualRegisterForArgument(1));
>         
>         LValue pointer = m_out.baseIndex(
>-            base.value(), m_out.zeroExt(zeroBasedIndex, m_out.intPtr), ScaleEight);
>+            base.value(), m_out.zeroExt(index, m_out.intPtr), ScaleEight);
>         setJSValue(m_out.load64(TypedPointer(m_heaps.variables.atAnyIndex(), pointer)));
>     }
> 
>Index: Source/JavaScriptCore/jit/JITOpcodes32_64.cpp
>===================================================================
>--- Source/JavaScriptCore/jit/JITOpcodes32_64.cpp	(revision 179531)
>+++ Source/JavaScriptCore/jit/JITOpcodes32_64.cpp	(working copy)
>@@ -1044,13 +1044,13 @@ void JIT::emit_op_get_argument_by_val(In
>     addSlowCase(branch32(NotEqual, tagFor(argumentsRegister), TrustedImm32(JSValue::EmptyValueTag)));
>     emitLoad(property, regT1, regT2);
>     addSlowCase(branch32(NotEqual, regT1, TrustedImm32(JSValue::Int32Tag)));
>-    add32(TrustedImm32(1), regT2);
>     // regT2 now contains the integer index of the argument we want, including this
>     load32(payloadFor(JSStack::ArgumentCount), regT3);
>+    sub32(TrustedImm32(1), regT3);
>     addSlowCase(branch32(AboveOrEqual, regT2, regT3));
>     
>-    loadPtr(BaseIndex(callFrameRegister, regT2, TimesEight, OBJECT_OFFSETOF(JSValue, u.asBits.payload) + CallFrame::thisArgumentOffset() * static_cast<int>(sizeof(Register))), regT0);
>-    loadPtr(BaseIndex(callFrameRegister, regT2, TimesEight, OBJECT_OFFSETOF(JSValue, u.asBits.tag) + CallFrame::thisArgumentOffset() * static_cast<int>(sizeof(Register))), regT1);
>+    loadPtr(BaseIndex(callFrameRegister, regT2, TimesEight, OBJECT_OFFSETOF(JSValue, u.asBits.payload) + CallFrame::thisArgumentOffset() * static_cast<int>(sizeof(Register)) + sizeof(Register)), regT0);
>+    loadPtr(BaseIndex(callFrameRegister, regT2, TimesEight, OBJECT_OFFSETOF(JSValue, u.asBits.tag) + CallFrame::thisArgumentOffset() * static_cast<int>(sizeof(Register)) + sizeof(Register)), regT1);
>     emitValueProfilingSite();
>     emitStore(dst, regT1, regT0);
> }
>Index: Source/JavaScriptCore/jit/JITOpcodes.cpp
>===================================================================
>--- Source/JavaScriptCore/jit/JITOpcodes.cpp	(revision 179531)
>+++ Source/JavaScriptCore/jit/JITOpcodes.cpp	(working copy)
>@@ -920,13 +920,12 @@ void JIT::emit_op_get_argument_by_val(In
>     addSlowCase(branchTest64(NonZero, addressFor(argumentsRegister)));
>     emitGetVirtualRegister(property, regT1);
>     addSlowCase(emitJumpIfNotImmediateInteger(regT1));
>-    add32(TrustedImm32(1), regT1);
>-    // regT1 now contains the integer index of the argument we want, including this
>     emitGetFromCallFrameHeader32(JSStack::ArgumentCount, regT2);
>+    sub32(TrustedImm32(1), regT2);
>     addSlowCase(branch32(AboveOrEqual, regT1, regT2));
> 
>     signExtend32ToPtr(regT1, regT1);
>-    load64(BaseIndex(callFrameRegister, regT1, TimesEight, CallFrame::thisArgumentOffset() * static_cast<int>(sizeof(Register))), regT0);
>+    load64(BaseIndex(callFrameRegister, regT1, TimesEight, CallFrame::thisArgumentOffset() * static_cast<int>(sizeof(Register)) + sizeof(Register)), regT0);
>     emitValueProfilingSite();
>     emitPutVirtualRegister(dst, regT0);
> }
>Index: Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm
>===================================================================
>--- Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm	(revision 179531)
>+++ Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm	(working copy)
>@@ -1613,12 +1613,12 @@ _llint_op_get_argument_by_val:
>     loadi 12[PC], t1
>     bineq TagOffset[cfr, t0, 8], EmptyValueTag, .opGetArgumentByValSlow
>     loadConstantOrVariablePayload(t1, Int32Tag, t2, .opGetArgumentByValSlow)
>-    addi 1, t2
>     loadi ArgumentCount + PayloadOffset[cfr], t1
>+    subi 1, t2
>     biaeq t2, t1, .opGetArgumentByValSlow
>     loadi 4[PC], t3
>-    loadi ThisArgumentOffset + TagOffset[cfr, t2, 8], t0
>-    loadi ThisArgumentOffset + PayloadOffset[cfr, t2, 8], t1
>+    loadi ThisArgumentOffset + 8 + TagOffset[cfr, t2, 8], t0
>+    loadi ThisArgumentOffset + 8 + PayloadOffset[cfr, t2, 8], t1
>     storei t0, TagOffset[cfr, t3, 8]
>     storei t1, PayloadOffset[cfr, t3, 8]
>     valueProfile(t0, t1, 24, t2)
>Index: Source/JavaScriptCore/llint/LowLevelInterpreter64.asm
>===================================================================
>--- Source/JavaScriptCore/llint/LowLevelInterpreter64.asm	(revision 179531)
>+++ Source/JavaScriptCore/llint/LowLevelInterpreter64.asm	(working copy)
>@@ -1471,12 +1471,13 @@ _llint_op_get_argument_by_val:
>     loadisFromInstruction(3, t1)
>     btqnz [cfr, t0, 8], .opGetArgumentByValSlow
>     loadConstantOrVariableInt32(t1, t2, .opGetArgumentByValSlow)
>-    addi 1, t2
>     loadi ArgumentCount + PayloadOffset[cfr], t1
>+    sxi2q t2, t2
>+    subi 1, t1
>     biaeq t2, t1, .opGetArgumentByValSlow
>     loadisFromInstruction(1, t3)
>     loadpFromInstruction(6, t1)
>-    loadq ThisArgumentOffset[cfr, t2, 8], t0
>+    loadq ThisArgumentOffset + 8[cfr, t2, 8], t0
>     storeq t0, [cfr, t3, 8]
>     valueProfile(t0, 6, t1)
>     dispatch(7)
>Index: Source/JavaScriptCore/tests/stress/get-my-argument-by-val-safe-wrap-around.js
>===================================================================
>--- Source/JavaScriptCore/tests/stress/get-my-argument-by-val-safe-wrap-around.js	(revision 0)
>+++ Source/JavaScriptCore/tests/stress/get-my-argument-by-val-safe-wrap-around.js	(working copy)
>@@ -0,0 +1,17 @@
>+function foo(index) {
>+    if (index > 1000)
>+        arguments = [1, 2, 3];
>+    return arguments[index];
>+}
>+
>+noInline(foo);
>+
>+for (var i = 0; i < 100000; ++i) {
>+    var result = foo(1, 42);
>+    if (result != 42)
>+        throw "Error: bad result in loop: " + result;
>+}
>+
>+var result = foo(-1);
>+if (result !== void 0)
>+    throw "Error: bad result at end: " + result;
>Index: Source/JavaScriptCore/tests/stress/get-my-argument-by-val-wrap-around-no-warm-up.js
>===================================================================
>--- Source/JavaScriptCore/tests/stress/get-my-argument-by-val-wrap-around-no-warm-up.js	(revision 0)
>+++ Source/JavaScriptCore/tests/stress/get-my-argument-by-val-wrap-around-no-warm-up.js	(working copy)
>@@ -0,0 +1,9 @@
>+function foo(index) {
>+    return arguments[index];
>+}
>+
>+noInline(foo);
>+
>+var result = foo(-1);
>+if (result !== void 0)
>+    throw "Error: bad result at end: " + result;
>Index: Source/JavaScriptCore/tests/stress/get-my-argument-by-val-wrap-around.js
>===================================================================
>--- Source/JavaScriptCore/tests/stress/get-my-argument-by-val-wrap-around.js	(revision 0)
>+++ Source/JavaScriptCore/tests/stress/get-my-argument-by-val-wrap-around.js	(working copy)
>@@ -0,0 +1,15 @@
>+function foo(index) {
>+    return arguments[index];
>+}
>+
>+noInline(foo);
>+
>+for (var i = 0; i < 100000; ++i) {
>+    var result = foo(1, 42);
>+    if (result != 42)
>+        throw "Error: bad result in loop: " + result;
>+}
>+
>+var result = foo(-1);
>+if (result !== void 0)
>+    throw "Error: bad result at end: " + result;

Actions: View | Formatted Diff | Diff

Attachments on bug 141183: 245916 | 245917 | 245918 | 245920