[CVE-2018-0934] Chakra JIT - Incomplete fix for MSRC-41913 chakra-core#2

 - Google, Inc.

This change completes a previous fix by not caching deep-copied arrays to ensure that only shallow-copied arrays are reused.

lib/Backend/BailOut.cpp

@@ -1301,8 +1301,11 @@ BailOutRecord::BailOutInlinedHelper(Js::JavascriptCallStackLayout * layout, Bail
         InlineeFrameRecord* inlineeFrameRecord = entryPointInfo->FindInlineeFrame(returnAddress);
         if (inlineeFrameRecord)
         {
+            // While bailing out, RestoreFrames should box all Vars on the stack. If there are multiple Vars pointing to the same
+            // object, the cached version (that was previously boxed) will be reused to maintain pointer identity and correctness
+            // after the transition to the interpreter.
             InlinedFrameLayout* outerMostFrame = (InlinedFrameLayout *)(((uint8 *)Js::JavascriptCallStackLayout::ToFramePointer(layout)) - entryPointInfo->frameHeight);
-            inlineeFrameRecord->RestoreFrames(functionBody, outerMostFrame, layout, false /* deepCopy */);
+            inlineeFrameRecord->RestoreFrames(functionBody, outerMostFrame, layout, true /* boxArgs */);
         }
     }
 

lib/Backend/InlineeFrameInfo.cpp

@@ -199,14 +199,14 @@ void InlineeFrameRecord::Finalize(Func* inlinee, uint32 currentOffset)
     Assert(this->inlineDepth != 0);
 }
 
-void InlineeFrameRecord::Restore(Js::FunctionBody* functionBody, InlinedFrameLayout *inlinedFrame, Js::JavascriptCallStackLayout * layout, bool deepCopy) const
+void InlineeFrameRecord::Restore(Js::FunctionBody* functionBody, InlinedFrameLayout *inlinedFrame, Js::JavascriptCallStackLayout * layout, bool boxValues) const
 {
     Assert(this->inlineDepth != 0);
     Assert(inlineeStartOffset != 0);
 
     BAILOUT_VERBOSE_TRACE(functionBody, _u("Restore function object: "));
     // No deepCopy needed for just the function
-    Js::Var varFunction = this->Restore(this->functionOffset, /*isFloat64*/ false, /*isInt32*/ false, layout, functionBody, /*deepCopy*/ false);
+    Js::Var varFunction = this->Restore(this->functionOffset, /*isFloat64*/ false, /*isInt32*/ false, layout, functionBody, boxValues);
     Assert(Js::ScriptFunction::Is(varFunction));
 
     Js::ScriptFunction* function = Js::ScriptFunction::FromVar(varFunction);
@@ -222,9 +222,9 @@ void InlineeFrameRecord::Restore(Js::FunctionBody* functionBody, InlinedFrameLay
 
         // Forward deepCopy flag for the arguments in case their data must be guaranteed
         // to have its own lifetime
-        Js::Var var = this->Restore(this->argOffsets[i], isFloat64, isInt32, layout, functionBody, deepCopy);
+        Js::Var var = this->Restore(this->argOffsets[i], isFloat64, isInt32, layout, functionBody, boxValues);
 #if DBG
-        if (!Js::TaggedNumber::Is(var))
+        if (boxValues && !Js::TaggedNumber::Is(var))
         {
             Js::RecyclableObject *const recyclableObject = Js::RecyclableObject::FromVar(var);
             Assert(!ThreadContext::IsOnStack(recyclableObject));
@@ -236,7 +236,10 @@ void InlineeFrameRecord::Restore(Js::FunctionBody* functionBody, InlinedFrameLay
     BAILOUT_FLUSH(functionBody);
 }
 
-void InlineeFrameRecord::RestoreFrames(Js::FunctionBody* functionBody, InlinedFrameLayout* outerMostFrame, Js::JavascriptCallStackLayout* callstack, bool deepCopy)
+// Note: the boxValues parameter should be true when this is called from a Bailout codepath to ensure that multiple vars to
+// the same object reuse the cached value during the transition to the interpreter.
+// Otherwise, this parameter should be false as the values are not required to be moved to the heap to restore the frame.
+void InlineeFrameRecord::RestoreFrames(Js::FunctionBody* functionBody, InlinedFrameLayout* outerMostFrame, Js::JavascriptCallStackLayout* callstack, bool boxValues)
 {
     InlineeFrameRecord* innerMostRecord = this;
     class AutoReverse
@@ -274,7 +277,7 @@ void InlineeFrameRecord::RestoreFrames(Js::FunctionBody* functionBody, InlinedFr
 
     while (currentRecord)
     {
-        currentRecord->Restore(functionBody, currentFrame, callstack, deepCopy);
+        currentRecord->Restore(functionBody, currentFrame, callstack, boxValues);
         currentRecord = currentRecord->parent;
         currentFrame = currentFrame->Next();
     }
@@ -283,10 +286,10 @@ void InlineeFrameRecord::RestoreFrames(Js::FunctionBody* functionBody, InlinedFr
     currentFrame->callInfo.Count = 0;
 }
 
-Js::Var InlineeFrameRecord::Restore(int offset, bool isFloat64, bool isInt32, Js::JavascriptCallStackLayout * layout, Js::FunctionBody* functionBody, bool deepCopy) const
+Js::Var InlineeFrameRecord::Restore(int offset, bool isFloat64, bool isInt32, Js::JavascriptCallStackLayout * layout, Js::FunctionBody* functionBody, bool boxValue) const
 {
     Js::Var value;
-    bool boxStackInstance = true;
+    bool boxStackInstance = boxValue;
     double dblValue;
     if (offset >= 0)
     {
@@ -324,8 +327,11 @@ Js::Var InlineeFrameRecord::Restore(int offset, bool isFloat64, bool isInt32, Js
         BAILOUT_VERBOSE_TRACE(functionBody, _u(", value: 0x%p"), value);
         if (boxStackInstance)
         {
+            // Do not deepCopy in this call to BoxStackInstance because this should be used for
+            // bailing out, where a shallow copy that is cached is needed to ensure that multiple
+            // vars pointing to the same boxed object reuse the new boxed value.
             Js::Var oldValue = value;
-            value = Js::JavascriptOperators::BoxStackInstance(oldValue, functionBody->GetScriptContext(), /* allowStackFunction */ true, deepCopy);
+            value = Js::JavascriptOperators::BoxStackInstance(oldValue, functionBody->GetScriptContext(), /* allowStackFunction */ true, false /* deepCopy */);
 
 #if ENABLE_DEBUG_CONFIG_OPTIONS
             if (oldValue != value)

lib/Backend/InlineeFrameInfo.h

@@ -108,7 +108,7 @@ struct InlineeFrameRecord
     }
 
     void PopulateParent(Func* func);
-    void RestoreFrames(Js::FunctionBody* functionBody, InlinedFrameLayout* outerMostInlinee, Js::JavascriptCallStackLayout* callstack, bool deepCopy);
+    void RestoreFrames(Js::FunctionBody* functionBody, InlinedFrameLayout* outerMostInlinee, Js::JavascriptCallStackLayout* callstack, bool boxValues);
     void Finalize(Func* inlinee, uint currentOffset);
 #if DBG_DUMP
     void Dump() const;
@@ -123,8 +123,8 @@ struct InlineeFrameRecord
     }
 
 private:
-    void Restore(Js::FunctionBody* functionBody, InlinedFrameLayout *outerMostFrame, Js::JavascriptCallStackLayout * layout, bool deepCopy) const;
-    Js::Var Restore(int offset, bool isFloat64, bool isInt32, Js::JavascriptCallStackLayout * layout, Js::FunctionBody* functionBody, bool deepCopy) const;
+    void Restore(Js::FunctionBody* functionBody, InlinedFrameLayout *outerMostFrame, Js::JavascriptCallStackLayout * layout, bool boxValues) const;
+    Js::Var Restore(int offset, bool isFloat64, bool isInt32, Js::JavascriptCallStackLayout * layout, Js::FunctionBody* functionBody, bool boxValue) const;
     InlineeFrameRecord* Reverse();
 };
 

lib/Runtime/Base/Debug.cpp

@@ -43,7 +43,7 @@ WCHAR* DumpCallStackFull(uint frameCount, bool print)
 
             for (uint i = 0; i < callInfo.Count; i++)
             {
-                StringCchPrintf(buffer, _countof(buffer), _u(", 0x%p"), walker.GetJavascriptArgs()[i]);
+                StringCchPrintf(buffer, _countof(buffer), _u(", 0x%p"), walker.GetJavascriptArgs(false /*boxArgs*/)[i]);
                 sb.AppendSz(buffer);
             }
             StringCchPrintf(buffer, _countof(buffer), _u(")[%s (%u, %d)]\n"), sourceFileName, line + 1, column + 1);

lib/Runtime/Language/JavascriptStackWalker.cpp

@@ -263,14 +263,14 @@ namespace Js
         this->GetCurrentArgv()[JavascriptFunctionArgIndex_ArgumentsObject] = args;
     }
 
-    Js::Var * JavascriptStackWalker::GetJavascriptArgs() const
+    Js::Var * JavascriptStackWalker::GetJavascriptArgs(bool boxArgsAndDeepCopy) const
     {
         Assert(this->IsJavascriptFrame());
 
 #if ENABLE_NATIVE_CODEGEN
         if (inlinedFramesBeingWalked)
         {
-            return inlinedFrameWalker.GetArgv(/* includeThis = */ false);
+            return inlinedFrameWalker.GetArgv(/* includeThis */ false, boxArgsAndDeepCopy);
         }
         else
 #endif
@@ -450,7 +450,7 @@ namespace Js
             // are inlined frames on the stack the InlineeCallInfo of the first inlined frame
             // has the native offset of the current physical frame.
             Assert(!*inlinee);
-            InlinedFrameWalker::FromPhysicalFrame(tmpFrameWalker, currentFrame, ScriptFunction::FromVar(parentFunction), PreviousInterpreterFrameIsFromBailout(), loopNum, this, useInternalFrameInfo, false /*noAlloc*/, false /*deepCopy*/);
+            InlinedFrameWalker::FromPhysicalFrame(tmpFrameWalker, currentFrame, ScriptFunction::FromVar(parentFunction), PreviousInterpreterFrameIsFromBailout(), loopNum, this, useInternalFrameInfo, false /*noAlloc*/);
             inlineeOffset = tmpFrameWalker.GetBottomMostInlineeOffset();
             tmpFrameWalker.Close();
         }
@@ -555,7 +555,7 @@ namespace Js
                         }
 
                         bool hasInlinedFramesOnStack = InlinedFrameWalker::FromPhysicalFrame(inlinedFrameWalker, currentFrame,
-                            ScriptFunction::FromVar(function), true /*fromBailout*/, loopNum, this, false /*useInternalFrameInfo*/, false /*noAlloc*/, this->deepCopyForArgs);
+                            ScriptFunction::FromVar(function), true /*fromBailout*/, loopNum, this, false /*useInternalFrameInfo*/, false /*noAlloc*/);
 
                         if (hasInlinedFramesOnStack)
                         {
@@ -611,8 +611,7 @@ namespace Js
                         -1,     // loopNum
                         nullptr,// walker
                         false,  // useInternalFrameInfo
-                        false,  // noAlloc
-                        this->deepCopyForArgs
+                        false   // noAlloc
                     );
                     if (inlinedFramesFound)
                     {
@@ -658,8 +657,7 @@ namespace Js
     _NOINLINE
     JavascriptStackWalker::JavascriptStackWalker(ScriptContext * scriptContext, bool useEERContext, PVOID returnAddress, bool _forceFullWalk /*=false*/) :
         inlinedFrameCallInfo(CallFlags_None, 0), shouldDetectPartiallyInitializedInterpreterFrame(true), forceFullWalk(_forceFullWalk),
-        previousInterpreterFrameIsFromBailout(false), previousInterpreterFrameIsForLoopBody(false), hasInlinedFramesOnStack(false),
-        deepCopyForArgs(false)
+        previousInterpreterFrameIsFromBailout(false), previousInterpreterFrameIsForLoopBody(false), hasInlinedFramesOnStack(false)
     {
         if (scriptContext == NULL)
         {
@@ -955,7 +953,7 @@ namespace Js
                 Assert(this->interpreterFrame->TestFlags(Js::InterpreterStackFrameFlags_FromBailOut));
                 InlinedFrameWalker tmpFrameWalker;
                 Assert(InlinedFrameWalker::FromPhysicalFrame(tmpFrameWalker, currentFrame, Js::ScriptFunction::FromVar(argv[JavascriptFunctionArgIndex_Function]),
-                    true /*fromBailout*/, this->tempInterpreterFrame->GetCurrentLoopNum(), this, false /*useInternalFrameInfo*/, true /*noAlloc*/, false /*deepCopy*/));
+                    true /*fromBailout*/, this->tempInterpreterFrame->GetCurrentLoopNum(), this, false /*useInternalFrameInfo*/, true /*noAlloc*/));
                 tmpFrameWalker.Close();
             }
 #endif //DBG
@@ -1002,7 +1000,7 @@ namespace Js
             {
                 if (includeInlineFrames &&
                     InlinedFrameWalker::FromPhysicalFrame(inlinedFrameWalker, currentFrame, Js::ScriptFunction::FromVar(argv[JavascriptFunctionArgIndex_Function]),
-                        false /*fromBailout*/, this->tempInterpreterFrame->GetCurrentLoopNum(), this, false /*useInternalFrameInfo*/, false /*noAlloc*/, this->deepCopyForArgs))
+                        false /*fromBailout*/, this->tempInterpreterFrame->GetCurrentLoopNum(), this, false /*useInternalFrameInfo*/, false /*noAlloc*/))
                 {
                     // Found inlined frames in a jitted loop body. We dont want to skip the inlined frames; walk all of them before setting codeAddress on lastInternalFrameInfo.
                     // DeepCopy here because, if there is an inlinee in a loop body, FromPhysicalFrame won't be called from UpdateFrame
@@ -1246,7 +1244,7 @@ namespace Js
 
 #if ENABLE_NATIVE_CODEGEN
     bool InlinedFrameWalker::FromPhysicalFrame(InlinedFrameWalker& self, StackFrame& physicalFrame, Js::ScriptFunction *parent, bool fromBailout,
-        int loopNum, const JavascriptStackWalker * const stackWalker, bool useInternalFrameInfo, bool noAlloc, bool deepCopy)
+        int loopNum, const JavascriptStackWalker * const stackWalker, bool useInternalFrameInfo, bool noAlloc)
     {
         bool inlinedFramesFound = false;
         FunctionBody* parentFunctionBody = parent->GetFunctionBody();
@@ -1299,7 +1297,7 @@ namespace Js
 
                 if (record)
                 {
-                    record->RestoreFrames(parent->GetFunctionBody(), outerMostFrame, JavascriptCallStackLayout::FromFramePointer(framePointer), deepCopy);
+                    record->RestoreFrames(parent->GetFunctionBody(), outerMostFrame, JavascriptCallStackLayout::FromFramePointer(framePointer), false /* boxValues */);
                 }
             }
 
@@ -1366,26 +1364,46 @@ namespace Js
         return currentFrame->callInfo.Count;
     }
 
-    Js::Var *InlinedFrameWalker::GetArgv(bool includeThis /* = true */) const
+    // Note: the boxArgsAndDeepCopy parameter should be true when a copy of the JS args must be ensured to
+    // be on the heap. This results in a new array of Vars with deep copied boxed values (where
+    // appropriate).
+    // Otherwise, this parameter should be false. For instance, if the args will only be used
+    // internally to gather type info, the values are not boxed (so, some Vars may still be on
+    // the stack) and the array of the current frame is returned.
+    Js::Var *InlinedFrameWalker::GetArgv(bool includeThis, bool boxArgsAndDeepCopy) const
     {
         InlinedFrameWalker::InlinedFrame *const currentFrame = GetCurrentFrame();
         Assert(currentFrame);
 
         uint firstArg = includeThis ? InlinedFrameArgIndex_This : InlinedFrameArgIndex_SecondScriptArg;
-        Js::Var *args = &currentFrame->argv[firstArg];
+        size_t argCount = this->GetArgc() - firstArg;
+        Js::Var *args;
+        if (!boxArgsAndDeepCopy)
+        {
+            args = &currentFrame->argv[firstArg];
+
+        }
+        else
+        {
+            args = RecyclerNewArray(parentFunction->GetScriptContext()->GetRecycler(), Js::Var, argCount);
+            for (size_t i = 0; i < argCount; i++)
+            {
+                args[i] = currentFrame->argv[firstArg + i];
+            }
 
-        this->FinalizeStackValues(args, this->GetArgc() - firstArg);
+            this->FinalizeStackValues(args, argCount, true /*deepCopy*/);
+        }
 
         return args;
     }
 
-    void InlinedFrameWalker::FinalizeStackValues(__in_ecount(argCount) Js::Var args[], size_t argCount) const
+    void InlinedFrameWalker::FinalizeStackValues(__in_ecount(argCount) Js::Var args[], size_t argCount, bool deepCopy) const
     {
         ScriptContext *scriptContext = this->GetFunctionObject()->GetScriptContext();
 
         for (size_t i = 0; i < argCount; i++)
         {
-            args[i] = Js::JavascriptOperators::BoxStackInstance(args[i], scriptContext, false /*allowStackFunction*/, false /*deepCopy*/);
+            args[i] = Js::JavascriptOperators::BoxStackInstance(args[i], scriptContext, false /*allowStackFunction*/, deepCopy);
         }
     }
 

lib/Runtime/Language/JavascriptStackWalker.h

@@ -97,11 +97,11 @@ namespace Js
         }
 
         static bool             FromPhysicalFrame(InlinedFrameWalker& self, StackFrame& physicalFrame, Js::ScriptFunction *parent, bool fromBailout,
-                                                  int loopNum, const JavascriptStackWalker * const walker, bool useInternalFrameInfo, bool noAlloc, bool deepCopy);
+                                                  int loopNum, const JavascriptStackWalker * const walker, bool useInternalFrameInfo, bool noAlloc);
         void                    Close();
         bool                    Next(CallInfo& callInfo);
         size_t                  GetArgc() const;
-        Js::Var                *GetArgv(bool includeThis = true) const;
+        Js::Var                *GetArgv(bool includeThis, bool boxArgsAndDeepCopy) const;
         Js::JavascriptFunction *GetFunctionObject() const;
         void                    SetFunctionObject(Js::JavascriptFunction * function);
         Js::Var                 GetArgumentsObject() const;
@@ -113,7 +113,7 @@ namespace Js
         uint32                  GetCurrentInlineeOffset() const;
         uint32                  GetBottomMostInlineeOffset() const;
         Js::JavascriptFunction *GetBottomMostFunctionObject() const;
-        void                    FinalizeStackValues(__in_ecount(argCount) Js::Var args[], size_t argCount) const;
+        void                    FinalizeStackValues(__in_ecount(argCount) Js::Var args[], size_t argCount, bool deepCopy) const;
         int32                   GetFrameCount() { return frameCount; }
 
     private:
@@ -218,7 +218,7 @@ namespace Js
         CallInfo GetCallInfo(bool includeInlinedFrames = true) const;
         CallInfo GetCallInfoFromPhysicalFrame() const;
         bool GetThis(Var *pThis, int moduleId) const;
-        Js::Var * GetJavascriptArgs() const;
+        Js::Var * GetJavascriptArgs(bool boxArgsAndDeepCopy) const;
         void **GetCurrentArgv() const;
 
         ScriptContext* GetCurrentScriptContext() const;
@@ -310,7 +310,6 @@ namespace Js
             return previousInterpreterFrameIsFromBailout;
         }
 
-        void SetDeepCopyForArguments() { deepCopyForArgs = true; }
 #if DBG
         static bool ValidateTopJitFrame(Js::ScriptContext* scriptContext);
 #endif
@@ -335,7 +334,6 @@ namespace Js
         bool                    previousInterpreterFrameIsFromBailout : 1;
         bool                    previousInterpreterFrameIsForLoopBody : 1;
         bool                    forceFullWalk               : 1; // ignoring hasCaller
-        bool                    deepCopyForArgs             : 1; // indicates when Var's data should be deep-copied when gathering Arguments for the frame
 
         Var GetThisFromFrame() const;                   // returns 'this' object from the physical frame
         Var GetCurrentArgumentsObject() const;          // returns arguments object from the current frame, which may be virtual (belonging to an inlinee)

lib/Runtime/Language/StackTraceArguments.cpp

@@ -73,7 +73,9 @@ namespace Js {
             if (numberOfArguments > 0) numberOfArguments --; // Don't consider 'this'
             for (int64 j = 0; j < numberOfArguments && j < MaxNumberOfDisplayedArgumentsInStack; j ++)
             {
-                types |= ObjectToTypeCode(walker.GetJavascriptArgs()[j]) << 3*j; // maximal code is 7, so we can use 3 bits to store it
+                // Since the Args are only used to get the type, no need to box the Vars to
+                // move them to the heap from the stack
+                types |= ObjectToTypeCode(walker.GetJavascriptArgs(false /*boxArgsAndDeepCopy*/)[j]) << 3 * j; // maximal code is 7, so we can use 3 bits to store it
             }
             if (numberOfArguments > MaxNumberOfDisplayedArgumentsInStack)
             {