[1.9>master] [1.8>1.9] [MERGE #5074 @kfarnung] Breakpoint APIs don't verify current context

Merge pull request #5074 from kfarnung:diagbp

The JsDiag APIs for breakpoints use the `GlobalAPIWrapper_NoRecord`
wrapper which does not validate that there's a current context. During
the function they then get the current context and attempt to use it
without checking whether it's `nullptr`.

The fix is to use `ContextAPIWrapper_NoRecord` instead which will
ensure that there's a current context.

.gitignore

@@ -39,6 +39,7 @@ build_*.log
 build_*.wrn
 Build/ipch/
 Build/swum-cache.txt
+Build/VCBuild.Lite/
 Build/VCBuild.NoJIT/
 Build/VCBuild.SWB/
 Build/VCBuild/

bin/NativeTests/JsDiagApiTest.cpp

@@ -0,0 +1,90 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+#include "stdafx.h"
+#include "catch.hpp"
+#include <process.h>
+
+#pragma warning(disable:4100) // unreferenced formal parameter
+#pragma warning(disable:6387) // suppressing preFAST which raises warning for passing null to the JsRT APIs
+#pragma warning(disable:6262) // CATCH is using stack variables to report errors, suppressing the preFAST warning.
+
+namespace JsDiagApiTest
+{
+    static void CALLBACK DebugEventCallback(JsDiagDebugEvent debugEvent, JsValueRef eventData, void* callbackState)
+    {
+    }
+
+    bool TestSetup(JsRuntimeHandle *runtime)
+    {
+        JsValueRef context = JS_INVALID_REFERENCE;
+        JsValueRef setContext = JS_INVALID_REFERENCE;
+
+        // Create runtime, context and set current context
+        REQUIRE(JsCreateRuntime(JsRuntimeAttributeNone, nullptr, runtime) == JsNoError);
+        REQUIRE(JsCreateContext(*runtime, &context) == JsNoError);
+        REQUIRE(JsSetCurrentContext(context) == JsNoError);
+        REQUIRE(((JsGetCurrentContext(&setContext) == JsNoError) || setContext == context));
+
+        // Enable debugging
+        REQUIRE(JsDiagStartDebugging(*runtime, JsDiagApiTest::DebugEventCallback, nullptr) == JsNoError);
+
+        return true;
+    }
+
+    bool TestCleanup(JsRuntimeHandle runtime)
+    {
+        if (runtime != nullptr)
+        {
+            // Disable debugging
+            JsDiagStopDebugging(runtime, nullptr);
+
+            JsSetCurrentContext(nullptr);
+            JsDisposeRuntime(runtime);
+        }
+        return true;
+    }
+
+    template <class Handler>
+    void WithSetup(Handler handler)
+    {
+        JsRuntimeHandle runtime = JS_INVALID_RUNTIME_HANDLE;
+        if (!TestSetup(&runtime))
+        {
+            REQUIRE(false);
+            return;
+        }
+
+        handler(runtime);
+
+        TestCleanup(runtime);
+    }
+
+#ifndef BUILD_WITHOUT_SCRIPT_DEBUG
+    void BreakpointsContextTest(JsRuntimeHandle runtime)
+    {
+        JsContextRef context = JS_INVALID_REFERENCE;
+        REQUIRE(JsGetCurrentContext(&context) == JsNoError);
+
+        // Verify no errors with a context set
+        JsValueRef scriptsArray = JS_INVALID_REFERENCE;
+        REQUIRE(JsDiagGetScripts(&scriptsArray) == JsNoError);
+
+        // Verify the APIs return an error when no current context set
+        JsSetCurrentContext(nullptr);
+        CHECK(JsDiagGetScripts(&scriptsArray) == JsErrorNoCurrentContext);
+
+        JsValueRef breakpoint = JS_INVALID_REFERENCE;
+        CHECK(JsDiagSetBreakpoint(0, 0, 0, &breakpoint) == JsErrorNoCurrentContext);
+
+        CHECK(JsDiagRemoveBreakpoint(0) == JsErrorNoCurrentContext);
+    }
+
+    TEST_CASE("JsDiagApiTest_BreakpointsContextTest", "[JsDiagApiTest]")
+    {
+        JsDiagApiTest::WithSetup(JsDiagApiTest::BreakpointsContextTest);
+    }
+#endif // BUILD_WITHOUT_SCRIPT_DEBUG
+}

bin/NativeTests/NativeTests.vcxproj

@@ -27,7 +27,6 @@
         $(ChakraCoreRootDirectory)bin\External;
         %(AdditionalIncludeDirectories)
       </AdditionalIncludeDirectories>
-
       <MultiProcessorCompilation>true</MultiProcessorCompilation>
       <SmallerTypeCheck>false</SmallerTypeCheck>
       <MinimalRebuild>false</MinimalRebuild>
@@ -47,6 +46,7 @@
     <ClCompile Include="CodexTests.cpp" />
     <ClCompile Include="FileLoadHelpers.cpp" />
     <ClCompile Include="FunctionExecutionTest.cpp" />
+    <ClCompile Include="JsDiagApiTest.cpp" />
     <ClCompile Include="JsRTApiTest.cpp" />
     <ClCompile Include="MemoryPolicyTest.cpp" />
     <ClCompile Include="NativeTests.cpp" />

lib/Jsrt/JsrtDiag.cpp

@@ -269,17 +269,12 @@ CHAKRA_API JsDiagGetBreakpoints(
 #ifndef ENABLE_SCRIPT_DEBUGGING
     return JsErrorCategoryUsage;
 #else
-    return GlobalAPIWrapper_NoRecord([&]() -> JsErrorCode {
-
+    return ContextAPIWrapper_NoRecord<false>([&](Js::ScriptContext* scriptContext) -> JsErrorCode {
         PARAM_NOT_NULL(breakpoints);
-
         *breakpoints = JS_INVALID_REFERENCE;
 
-        JsrtContext *currentContext = JsrtContext::GetCurrent();
-
-        Js::JavascriptArray* bpsArray = currentContext->GetScriptContext()->GetLibrary()->CreateArray();
-
-        JsrtRuntime * runtime = currentContext->GetRuntime();
+        JsrtContext* currentContext = JsrtContext::GetCurrent();
+        JsrtRuntime* runtime = currentContext->GetRuntime();
 
         ThreadContextScope scope(runtime->GetThreadContext());
 
@@ -289,18 +284,18 @@ CHAKRA_API JsDiagGetBreakpoints(
         }
 
         JsrtDebugManager* jsrtDebugManager = runtime->GetJsrtDebugManager();
-
         VALIDATE_IS_DEBUGGING(jsrtDebugManager);
 
-        for (Js::ScriptContext *scriptContext = runtime->GetThreadContext()->GetScriptContextList();
-        scriptContext != nullptr && !scriptContext->IsClosed();
-            scriptContext = scriptContext->next)
+        Js::JavascriptArray* bpsArray = currentContext->GetScriptContext()->GetLibrary()->CreateArray();
+
+        for (Js::ScriptContext* currentScriptContext = runtime->GetThreadContext()->GetScriptContextList();
+            currentScriptContext != nullptr && !currentScriptContext->IsClosed();
+            currentScriptContext = currentScriptContext->next)
         {
-            jsrtDebugManager->GetBreakpoints(&bpsArray, scriptContext);
+            jsrtDebugManager->GetBreakpoints(&bpsArray, currentScriptContext);
         }
 
         *breakpoints = bpsArray;
-
         return JsNoError;
     });
 #endif
@@ -315,15 +310,12 @@ CHAKRA_API JsDiagSetBreakpoint(
 #ifndef ENABLE_SCRIPT_DEBUGGING
     return JsErrorCategoryUsage;
 #else
-    return GlobalAPIWrapper_NoRecord([&]() -> JsErrorCode {
-
+    return ContextAPIWrapper_NoRecord<false>([&](Js::ScriptContext* scriptContext) -> JsErrorCode {
         PARAM_NOT_NULL(breakpoint);
-
         *breakpoint = JS_INVALID_REFERENCE;
 
-        JsrtContext *currentContext = JsrtContext::GetCurrent();
-
-        JsrtRuntime * runtime = currentContext->GetRuntime();
+        JsrtContext* currentContext = JsrtContext::GetCurrent();
+        JsrtRuntime* runtime = currentContext->GetRuntime();
 
         ThreadContextScope scope(runtime->GetThreadContext());
 
@@ -336,11 +328,11 @@ CHAKRA_API JsDiagSetBreakpoint(
 
         Js::Utf8SourceInfo* utf8SourceInfo = nullptr;
 
-        for (Js::ScriptContext *scriptContext = runtime->GetThreadContext()->GetScriptContextList();
-        scriptContext != nullptr && utf8SourceInfo == nullptr && !scriptContext->IsClosed();
-            scriptContext = scriptContext->next)
+        for (Js::ScriptContext* currentScriptContext = runtime->GetThreadContext()->GetScriptContextList();
+            currentScriptContext != nullptr && utf8SourceInfo == nullptr && !currentScriptContext->IsClosed();
+            currentScriptContext = currentScriptContext->next)
         {
-            scriptContext->MapScript([&](Js::Utf8SourceInfo* sourceInfo) -> bool
+            currentScriptContext->MapScript([&](Js::Utf8SourceInfo* sourceInfo) -> bool
             {
                 if (sourceInfo->GetSourceInfoId() == scriptId)
                 {
@@ -354,7 +346,6 @@ CHAKRA_API JsDiagSetBreakpoint(
         if (utf8SourceInfo != nullptr && utf8SourceInfo->HasDebugDocument())
         {
             JsrtDebugManager* jsrtDebugManager = runtime->GetJsrtDebugManager();
-
             Js::DynamicObject* bpObject = jsrtDebugManager->SetBreakPoint(currentContext->GetScriptContext(), utf8SourceInfo, lineNumber, columnNumber);
 
             if(bpObject != nullptr)
@@ -377,10 +368,8 @@ CHAKRA_API JsDiagRemoveBreakpoint(
 #ifndef ENABLE_SCRIPT_DEBUGGING
     return JsErrorCategoryUsage;
 #else
-    return GlobalAPIWrapper_NoRecord([&]() -> JsErrorCode {
-
-        JsrtContext *currentContext = JsrtContext::GetCurrent();
-
+    return ContextAPIWrapper_NoRecord<false>([&](Js::ScriptContext* scriptContext) -> JsErrorCode {
+        JsrtContext* currentContext = JsrtContext::GetCurrent();
         JsrtRuntime* runtime = currentContext->GetRuntime();
 
         ThreadContextScope scope(runtime->GetThreadContext());
@@ -391,7 +380,6 @@ CHAKRA_API JsDiagRemoveBreakpoint(
         }
 
         JsrtDebugManager* jsrtDebugManager = runtime->GetJsrtDebugManager();
-
         VALIDATE_IS_DEBUGGING(jsrtDebugManager);
 
         if (!jsrtDebugManager->RemoveBreakpoint(breakpointId))