Skip to content

Commit c0c1d63

Browse files
kfarnungkfarnung
committed
[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.
2 parents 46611d3 + 380cb04 commit c0c1d63

File tree

4 files changed

+110
-31
lines changed

4 files changed

+110
-31
lines changed

‎.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ build_*.log
3939
build_*.wrn
4040
Build/ipch/
4141
Build/swum-cache.txt
42+
Build/VCBuild.Lite/
4243
Build/VCBuild.NoJIT/
4344
Build/VCBuild.SWB/
4445
Build/VCBuild/

‎bin/NativeTests/JsDiagApiTest.cpp

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
//-------------------------------------------------------------------------------------------------------
2+
// Copyright (C) Microsoft. All rights reserved.
3+
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
4+
//-------------------------------------------------------------------------------------------------------
5+
6+
#include "stdafx.h"
7+
#include "catch.hpp"
8+
#include <process.h>
9+
10+
#pragma warning(disable:4100) // unreferenced formal parameter
11+
#pragma warning(disable:6387) // suppressing preFAST which raises warning for passing null to the JsRT APIs
12+
#pragma warning(disable:6262) // CATCH is using stack variables to report errors, suppressing the preFAST warning.
13+
14+
namespace JsDiagApiTest
15+
{
16+
static void CALLBACK DebugEventCallback(JsDiagDebugEvent debugEvent, JsValueRef eventData, void* callbackState)
17+
{
18+
}
19+
20+
bool TestSetup(JsRuntimeHandle *runtime)
21+
{
22+
JsValueRef context = JS_INVALID_REFERENCE;
23+
JsValueRef setContext = JS_INVALID_REFERENCE;
24+
25+
// Create runtime, context and set current context
26+
REQUIRE(JsCreateRuntime(JsRuntimeAttributeNone, nullptr, runtime) == JsNoError);
27+
REQUIRE(JsCreateContext(*runtime, &context) == JsNoError);
28+
REQUIRE(JsSetCurrentContext(context) == JsNoError);
29+
REQUIRE(((JsGetCurrentContext(&setContext) == JsNoError) || setContext == context));
30+
31+
// Enable debugging
32+
REQUIRE(JsDiagStartDebugging(*runtime, JsDiagApiTest::DebugEventCallback, nullptr) == JsNoError);
33+
34+
return true;
35+
}
36+
37+
bool TestCleanup(JsRuntimeHandle runtime)
38+
{
39+
if (runtime != nullptr)
40+
{
41+
// Disable debugging
42+
JsDiagStopDebugging(runtime, nullptr);
43+
44+
JsSetCurrentContext(nullptr);
45+
JsDisposeRuntime(runtime);
46+
}
47+
return true;
48+
}
49+
50+
template <class Handler>
51+
void WithSetup(Handler handler)
52+
{
53+
JsRuntimeHandle runtime = JS_INVALID_RUNTIME_HANDLE;
54+
if (!TestSetup(&runtime))
55+
{
56+
REQUIRE(false);
57+
return;
58+
}
59+
60+
handler(runtime);
61+
62+
TestCleanup(runtime);
63+
}
64+
65+
#ifndef BUILD_WITHOUT_SCRIPT_DEBUG
66+
void BreakpointsContextTest(JsRuntimeHandle runtime)
67+
{
68+
JsContextRef context = JS_INVALID_REFERENCE;
69+
REQUIRE(JsGetCurrentContext(&context) == JsNoError);
70+
71+
// Verify no errors with a context set
72+
JsValueRef scriptsArray = JS_INVALID_REFERENCE;
73+
REQUIRE(JsDiagGetScripts(&scriptsArray) == JsNoError);
74+
75+
// Verify the APIs return an error when no current context set
76+
JsSetCurrentContext(nullptr);
77+
CHECK(JsDiagGetScripts(&scriptsArray) == JsErrorNoCurrentContext);
78+
79+
JsValueRef breakpoint = JS_INVALID_REFERENCE;
80+
CHECK(JsDiagSetBreakpoint(0, 0, 0, &breakpoint) == JsErrorNoCurrentContext);
81+
82+
CHECK(JsDiagRemoveBreakpoint(0) == JsErrorNoCurrentContext);
83+
}
84+
85+
TEST_CASE("JsDiagApiTest_BreakpointsContextTest", "[JsDiagApiTest]")
86+
{
87+
JsDiagApiTest::WithSetup(JsDiagApiTest::BreakpointsContextTest);
88+
}
89+
#endif // BUILD_WITHOUT_SCRIPT_DEBUG
90+
}

‎bin/NativeTests/NativeTests.vcxproj

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@
2727
$(ChakraCoreRootDirectory)bin\External;
2828
%(AdditionalIncludeDirectories)
2929
</AdditionalIncludeDirectories>
30-
3130
<MultiProcessorCompilation>true</MultiProcessorCompilation>
3231
<SmallerTypeCheck>false</SmallerTypeCheck>
3332
<MinimalRebuild>false</MinimalRebuild>
@@ -47,6 +46,7 @@
4746
<ClCompile Include="CodexTests.cpp" />
4847
<ClCompile Include="FileLoadHelpers.cpp" />
4948
<ClCompile Include="FunctionExecutionTest.cpp" />
49+
<ClCompile Include="JsDiagApiTest.cpp" />
5050
<ClCompile Include="JsRTApiTest.cpp" />
5151
<ClCompile Include="MemoryPolicyTest.cpp" />
5252
<ClCompile Include="NativeTests.cpp" />

‎lib/Jsrt/JsrtDiag.cpp

Lines changed: 18 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -263,17 +263,12 @@ CHAKRA_API JsDiagGetBreakpoints(
263263
#ifndef ENABLE_SCRIPT_DEBUGGING
264264
return JsErrorCategoryUsage;
265265
#else
266-
return GlobalAPIWrapper_NoRecord([&]() -> JsErrorCode {
267-
266+
return ContextAPIWrapper_NoRecord<false>([&](Js::ScriptContext* scriptContext) -> JsErrorCode {
268267
PARAM_NOT_NULL(breakpoints);
269-
270268
*breakpoints = JS_INVALID_REFERENCE;
271269

272-
JsrtContext *currentContext = JsrtContext::GetCurrent();
273-
274-
Js::JavascriptArray* bpsArray = currentContext->GetScriptContext()->GetLibrary()->CreateArray();
275-
276-
JsrtRuntime * runtime = currentContext->GetRuntime();
270+
JsrtContext* currentContext = JsrtContext::GetCurrent();
271+
JsrtRuntime* runtime = currentContext->GetRuntime();
277272

278273
ThreadContextScope scope(runtime->GetThreadContext());
279274

@@ -283,18 +278,18 @@ CHAKRA_API JsDiagGetBreakpoints(
283278
}
284279

285280
JsrtDebugManager* jsrtDebugManager = runtime->GetJsrtDebugManager();
286-
287281
VALIDATE_IS_DEBUGGING(jsrtDebugManager);
288282

289-
for (Js::ScriptContext *scriptContext = runtime->GetThreadContext()->GetScriptContextList();
290-
scriptContext != nullptr && !scriptContext->IsClosed();
291-
scriptContext = scriptContext->next)
283+
Js::JavascriptArray* bpsArray = currentContext->GetScriptContext()->GetLibrary()->CreateArray();
284+
285+
for (Js::ScriptContext* currentScriptContext = runtime->GetThreadContext()->GetScriptContextList();
286+
currentScriptContext != nullptr && !currentScriptContext->IsClosed();
287+
currentScriptContext = currentScriptContext->next)
292288
{
293-
jsrtDebugManager->GetBreakpoints(&bpsArray, scriptContext);
289+
jsrtDebugManager->GetBreakpoints(&bpsArray, currentScriptContext);
294290
}
295291

296292
*breakpoints = bpsArray;
297-
298293
return JsNoError;
299294
});
300295
#endif
@@ -309,15 +304,12 @@ CHAKRA_API JsDiagSetBreakpoint(
309304
#ifndef ENABLE_SCRIPT_DEBUGGING
310305
return JsErrorCategoryUsage;
311306
#else
312-
return GlobalAPIWrapper_NoRecord([&]() -> JsErrorCode {
313-
307+
return ContextAPIWrapper_NoRecord<false>([&](Js::ScriptContext* scriptContext) -> JsErrorCode {
314308
PARAM_NOT_NULL(breakpoint);
315-
316309
*breakpoint = JS_INVALID_REFERENCE;
317310

318-
JsrtContext *currentContext = JsrtContext::GetCurrent();
319-
320-
JsrtRuntime * runtime = currentContext->GetRuntime();
311+
JsrtContext* currentContext = JsrtContext::GetCurrent();
312+
JsrtRuntime* runtime = currentContext->GetRuntime();
321313

322314
ThreadContextScope scope(runtime->GetThreadContext());
323315

@@ -330,11 +322,11 @@ CHAKRA_API JsDiagSetBreakpoint(
330322

331323
Js::Utf8SourceInfo* utf8SourceInfo = nullptr;
332324

333-
for (Js::ScriptContext *scriptContext = runtime->GetThreadContext()->GetScriptContextList();
334-
scriptContext != nullptr && utf8SourceInfo == nullptr && !scriptContext->IsClosed();
335-
scriptContext = scriptContext->next)
325+
for (Js::ScriptContext* currentScriptContext = runtime->GetThreadContext()->GetScriptContextList();
326+
currentScriptContext != nullptr && utf8SourceInfo == nullptr && !currentScriptContext->IsClosed();
327+
currentScriptContext = currentScriptContext->next)
336328
{
337-
scriptContext->MapScript([&](Js::Utf8SourceInfo* sourceInfo) -> bool
329+
currentScriptContext->MapScript([&](Js::Utf8SourceInfo* sourceInfo) -> bool
338330
{
339331
if (sourceInfo->GetSourceInfoId() == scriptId)
340332
{
@@ -348,7 +340,6 @@ CHAKRA_API JsDiagSetBreakpoint(
348340
if (utf8SourceInfo != nullptr && utf8SourceInfo->HasDebugDocument())
349341
{
350342
JsrtDebugManager* jsrtDebugManager = runtime->GetJsrtDebugManager();
351-
352343
Js::DynamicObject* bpObject = jsrtDebugManager->SetBreakPoint(currentContext->GetScriptContext(), utf8SourceInfo, lineNumber, columnNumber);
353344

354345
if(bpObject != nullptr)
@@ -371,10 +362,8 @@ CHAKRA_API JsDiagRemoveBreakpoint(
371362
#ifndef ENABLE_SCRIPT_DEBUGGING
372363
return JsErrorCategoryUsage;
373364
#else
374-
return GlobalAPIWrapper_NoRecord([&]() -> JsErrorCode {
375-
376-
JsrtContext *currentContext = JsrtContext::GetCurrent();
377-
365+
return ContextAPIWrapper_NoRecord<false>([&](Js::ScriptContext* scriptContext) -> JsErrorCode {
366+
JsrtContext* currentContext = JsrtContext::GetCurrent();
378367
JsrtRuntime* runtime = currentContext->GetRuntime();
379368

380369
ThreadContextScope scope(runtime->GetThreadContext());
@@ -385,7 +374,6 @@ CHAKRA_API JsDiagRemoveBreakpoint(
385374
}
386375

387376
JsrtDebugManager* jsrtDebugManager = runtime->GetJsrtDebugManager();
388-
389377
VALIDATE_IS_DEBUGGING(jsrtDebugManager);
390378

391379
if (!jsrtDebugManager->RemoveBreakpoint(breakpointId))

0 commit comments

Comments
 (0)