[1.9>master] [1.8>1.9] [MERGE #4812 @akroshg] ChakraCore 2018-03 Secu…

…rity updates

Merge pull request #4812 from akroshg:test1803_1

Pushing 18-03 changes.

Build/Common.Build.props

@@ -78,6 +78,9 @@
       <!-- Separate global variable for linker -->
       <AdditionalOptions>%(AdditionalOptions) /Gw</AdditionalOptions>
 
+      <!-- Enable warnings not included in W4 by default -->
+      <AdditionalOptions>%(AdditionalOptions) /w44242 /w44254</AdditionalOptions>
+
       <ProgramDataBaseFileName Condition="'$(ConfigurationType)'=='StaticLibrary'">$(IntDir)$(TargetName).pdb</ProgramDataBaseFileName>
       <ProgramDataBaseFileName Condition="'$(ConfigurationType)'!='StaticLibrary'">$(IntDir)</ProgramDataBaseFileName>
 

bin/NativeTests/NativeTests.cpp

@@ -7,6 +7,7 @@
 #define CATCH_CONFIG_RUNNER
 #pragma warning(push)
 // conversion from 'int' to 'char', possible loss of data
+#pragma warning(disable:4242)
 #pragma warning(disable:4244)
 #include "catch.hpp"
 #pragma warning(pop)

lib/Backend/BailOut.cpp

@@ -1122,8 +1122,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/GlobOpt.cpp

@@ -15154,7 +15154,9 @@ GlobOpt::DoArrayCheckHoist(const ValueType baseValueType, Loop* loop, IR::Instr
         return false;
     }
 
-    if(!baseValueType.IsLikelyArrayOrObjectWithArray() ||
+    // This includes typed arrays, but not virtual typed arrays, whose vtable can change if the buffer goes away.
+    // Note that in the virtual case the vtable check is the only way to catch this, since there's no bound check.
+    if(!(baseValueType.IsLikelyArrayOrObjectWithArray() || baseValueType.IsLikelyOptimizedVirtualTypedArray()) ||
         (loop ? ImplicitCallFlagsAllowOpts(loop) : ImplicitCallFlagsAllowOpts(func)))
     {
         return true;

lib/Backend/GlobOpt.h

@@ -359,12 +359,16 @@ class JsArrayKills
 public:
     bool KillsValueType(const ValueType valueType) const
     {
-        Assert(valueType.IsArrayOrObjectWithArray());
+        Assert(valueType.IsArrayOrObjectWithArray() || valueType.IsOptimizedVirtualTypedArray());
 
         return
             killsAllArrays ||
-            (killsArraysWithNoMissingValues && valueType.HasNoMissingValues()) ||
-            (killsNativeArrays && !valueType.HasVarElements());
+            (valueType.IsArrayOrObjectWithArray() && 
+             (
+              (killsArraysWithNoMissingValues && valueType.HasNoMissingValues()) ||
+              (killsNativeArrays && !valueType.HasVarElements())
+             )
+            );
     }
 
     bool AreSubsetOf(const JsArrayKills &other) const

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/Common/CommonDefines.h

@@ -331,7 +331,10 @@
 
 #ifndef NTBUILD
 #define DELAYLOAD_SECTIONAPI 1
-#else
+#define DELAYLOAD_UNLOCKMEMORY 1
+#endif
+
+#ifdef NTBUILD
 #define ENABLE_PROJECTION
 #define ENABLE_FOUNDATION_OBJECT
 #define ENABLE_EXPERIMENTAL_FLAGS

lib/Common/Core/DelayLoadLibrary.cpp

@@ -346,4 +346,43 @@ NtdllLibrary::NTSTATUS NtdllLibrary::Close(_In_ HANDLE Handle)
 #endif
 }
 
+#ifndef DELAYLOAD_UNLOCKMEMORY
+extern "C"
+WINBASEAPI
+NtdllLibrary::NTSTATUS
+WINAPI
+NtUnlockVirtualMemory(
+    _In_ HANDLE ProcessHandle,
+    _Inout_ PVOID *BaseAddress,
+    _Inout_ PSIZE_T RegionSize,
+    _In_ ULONG MapType
+);
+#endif
+
+NtdllLibrary::NTSTATUS NtdllLibrary::UnlockVirtualMemory(
+    _In_ HANDLE ProcessHandle,
+    _Inout_ PVOID *BaseAddress,
+    _Inout_ PSIZE_T RegionSize,
+    _In_ ULONG MapType)
+{
+#ifdef DELAYLOAD_UNLOCKMEMORY
+    if (m_hModule)
+    {
+        if (unlock == nullptr)
+        {
+            unlock = (PFnNtUnlockVirtualMemory)GetFunction("NtUnlockVirtualMemory");
+            if (unlock == nullptr)
+            {
+                Assert(false);
+                return -1;
+            }
+        }
+        return unlock(ProcessHandle, BaseAddress, RegionSize, MapType);
+    }
+    return -1;
+#else
+    return NtUnlockVirtualMemory(ProcessHandle, BaseAddress, RegionSize, MapType);
+#endif
+}
+
 #endif // _WIN32

lib/Common/Core/DelayLoadLibrary.h

@@ -63,6 +63,7 @@ class NtdllLibrary : protected DelayLoadLibrary
 public:
     // needed for InitializeObjectAttributes
     static const ULONG OBJ_KERNEL_HANDLE = 0x00000200;
+    static const ULONG MAP_PROCESS = 1;
 
     typedef struct _UNICODE_STRING {
         USHORT Length;
@@ -134,6 +135,13 @@ class NtdllLibrary : protected DelayLoadLibrary
     typedef NTSTATUS(NTAPI *PFnNtClose)(_In_ HANDLE Handle);
     PFnNtClose close;
 
+    typedef NTSTATUS(NTAPI *PFnNtUnlockVirtualMemory)(
+        _In_ HANDLE ProcessHandle,
+        _Inout_ PVOID *BaseAddress,
+        _Inout_ PSIZE_T RegionSize,
+        _In_ ULONG MapType);
+    PFnNtUnlockVirtualMemory unlock;
+
 public:
     static NtdllLibrary* Instance;
 
@@ -146,7 +154,8 @@ class NtdllLibrary : protected DelayLoadLibrary
         createSection(NULL),
         mapViewOfSection(NULL),
         unmapViewOfSection(NULL),
-        close(NULL)
+        close(NULL),
+        unlock(nullptr)
     {
         this->EnsureFromSystemDirOnly();
     }
@@ -205,5 +214,12 @@ class NtdllLibrary : protected DelayLoadLibrary
     NTSTATUS Close(
         _In_ HANDLE Handle
     );
+
+    NTSTATUS UnlockVirtualMemory(
+        _In_ HANDLE ProcessHandle,
+        _Inout_ PVOID *BaseAddress,
+        _Inout_ PSIZE_T RegionSize,
+        _In_ ULONG MapType
+    );
 };
 #endif

lib/Common/Memory/RecyclerSweep.cpp

@@ -290,7 +290,7 @@ RecyclerSweep::EndSweep()
 
     // Clean up the HeapBlockMap.
     // This will release any internal structures that are no longer needed after Sweep.
-    recycler->heapBlockMap.Cleanup(!recycler->IsMemProtectMode());
+    recycler->heapBlockMap.Cleanup(recycler->IsMemProtectMode());
 }
 
 #if ENABLE_CONCURRENT_GC

lib/Common/Memory/SectionAllocWrapper.cpp

@@ -10,11 +10,21 @@
 #ifdef NTDDI_WIN10_RS2
 #if (NTDDI_VERSION >= NTDDI_WIN10_RS2)
 #define USEFILEMAP2 1
+#define USEVIRTUALUNLOCKEX 1
 #endif
 #endif
 
 namespace Memory
 {
+
+void UnlockMemory(HANDLE process, LPVOID address, SIZE_T size)
+{
+#if USEVIRTUALUNLOCKEX
+    VirtualUnlockEx(process, address, size);
+#else
+    NtdllLibrary::Instance->UnlockVirtualMemory(process, &address, &size, NtdllLibrary::MAP_PROCESS);
+#endif
+}
 
 void CloseSectionHandle(HANDLE handle)
 {
@@ -593,12 +603,6 @@ SectionAllocWrapper::AllocPages(LPVOID requestAddress, size_t pageCount, DWORD a
             return nullptr;
         }
         address = requestAddress;
-
-        if ((allocationType & MEM_COMMIT) == MEM_COMMIT)
-        {
-            const DWORD allocProtectFlags = AutoSystemInfo::Data.IsCFGEnabled() ? PAGE_EXECUTE_RO_TARGETS_INVALID : PAGE_EXECUTE_READ;
-            address = VirtualAllocEx(this->process, address, dwSize, MEM_COMMIT, allocProtectFlags);
-        }
     }
 
     return address;
@@ -661,11 +665,7 @@ BOOL SectionAllocWrapper::Free(LPVOID lpAddress, size_t dwSize, DWORD dwFreeType
             ZeroMemory(localAddr, AutoSystemInfo::PageSize);
             FreeLocal(localAddr);
         }
-        DWORD oldFlags = NULL;
-        if (!VirtualProtectEx(this->process, lpAddress, dwSize, PAGE_NOACCESS, &oldFlags))
-        {
-            return FALSE;
-        }
+        UnlockMemory(this->process, lpAddress, dwSize);
     }
 
     return TRUE;
@@ -924,37 +924,15 @@ LPVOID PreReservedSectionAllocWrapper::AllocPages(LPVOID lpAddress, DECLSPEC_GUA
         AssertMsg(freeSegmentsBVIndex < PreReservedAllocationSegmentCount, "Invalid BitVector index calculation?");
         AssertMsg(dwSize % AutoSystemInfo::PageSize == 0, "COMMIT is managed at AutoSystemInfo::PageSize granularity");
 
-        char * allocatedAddress = nullptr;
-
-        if ((allocationType & MEM_COMMIT) != 0)
-        {
-#if defined(ENABLE_JIT_CLAMP)
-            AutoEnableDynamicCodeGen enableCodeGen;
-#endif
-
-            const DWORD allocProtectFlags = AutoSystemInfo::Data.IsCFGEnabled() ? PAGE_EXECUTE_RO_TARGETS_INVALID : PAGE_EXECUTE_READ;
-            allocatedAddress = (char *)VirtualAllocEx(this->process, addressToReserve, dwSize, MEM_COMMIT, allocProtectFlags);
-            if (allocatedAddress == nullptr)
-            {
-                MemoryOperationLastError::RecordLastError();
-            }
-        }
-        else
-        {
-            // Just return the uncommitted address if we didn't ask to commit it.
-            allocatedAddress = addressToReserve;
-        }
-
         // Keep track of the committed pages within the preReserved Memory Region
-        if (lpAddress == nullptr && allocatedAddress != nullptr)
+        if (lpAddress == nullptr)
         {
-            Assert(allocatedAddress == addressToReserve);
             Assert(requestedNumOfSegments != 0);
             freeSegments.ClearRange(freeSegmentsBVIndex, static_cast<uint>(requestedNumOfSegments));
-        }
+    }
 
-        PreReservedHeapTrace(_u("MEM_COMMIT: StartAddress: 0x%p of size: 0x%x * 0x%x bytes \n"), allocatedAddress, requestedNumOfSegments, AutoSystemInfo::Data.GetAllocationGranularityPageSize());
-        return allocatedAddress;
+        PreReservedHeapTrace(_u("MEM_COMMIT: StartAddress: 0x%p of size: 0x%x * 0x%x bytes \n"), addressToReserve, requestedNumOfSegments, AutoSystemInfo::Data.GetAllocationGranularityPageSize());
+        return addressToReserve;
     }
 }
 
@@ -989,11 +967,7 @@ PreReservedSectionAllocWrapper::Free(LPVOID lpAddress, size_t dwSize, DWORD dwFr
         FreeLocal(localAddr);
     }
 
-    DWORD oldFlags = NULL;
-    if(!VirtualProtectEx(this->process, lpAddress, dwSize, PAGE_NOACCESS, &oldFlags))
-    {
-        return FALSE;
-    }
+    UnlockMemory(this->process, lpAddress, dwSize);
 
     size_t requestedNumOfSegments = dwSize / AutoSystemInfo::Data.GetAllocationGranularityPageSize();
     Assert(requestedNumOfSegments <= MAXUINT32);