Skip to content

Commit

Permalink
Fix leak in JSRT
Browse files Browse the repository at this point in the history
1. Pinned object fakeGlobalFuncForUndefer (as well as profileInfoList in test/debug build) reference to javascriptLibrary -- directly or indirectly, these are rely on ScriptContext::Close() to unpin.
2. javascriptLibrary has a reference to JsrtContext
3. JsrtContext is pinned while setting to current thread, and unpinned when getting out of current thread
4. if user code didn't explicited pin JsrtContext (in following POC), at this stage it should be disposed in next GC, and hence call ScriptContext::Close()
5. the disposal in chakra-core#4 didn't because JsrtContext is reachable through fakeGlobalFuncForUndefer->javascriptLibrary->JsrtContext(chakra-core#2), so the whole graph is leaked
6. when there's external call to JsDisposeRuntime, it will directly dispose JsrtContext, and then ScriptContext::Close, unpin fakeGlobalFuncForUndefer then everything is collectable

the POC:
```c++
    JsRuntimeHandle runtime;
    unsigned currentSourceContext = 0;
    JsCreateRuntime(JsRuntimeAttributeNone, nullptr, &runtime);
    auto runJob = [&](wstring script, int i)
    {
        {
            JsValueRef result;
            JsContextRef context;
            JsCreateContext(runtime, &context);
            JsSetCurrentContext(context);
            JsRunScript(script.c_str(), currentSourceContext++, L"", &result);
            JsSetCurrentContext(JS_INVALID_REFERENCE);
            context = nullptr;
            result = nullptr;
        }

        if (i % 5 == 0) {
            JsCollectGarbage(runtime); // JsrtContext in above scope should be collectible at this point,
                                       // but the Finalize/Dispose of JsrtContext didn't happen
        }
    };

    for (int i = 0; i < 100; i++)
    {
        runJob(L"(()=>{return \'Hello world!\';})()", i);
    }

    printf("JsDisposeRuntime\n");
    JsDisposeRuntime(runtime); // all JsrtContext will be collected at this point
    printf("After JsDisposeRuntime\n");
```

The fix is, do not pin fakeGlobalFuncForUndefer and profileInfoList. However, there are a lot of code(mostly debugger related code) rely on the leak to do the cleanup. Most of the work is to make sure the cleanup working correctly (without either UAF or leak).
  • Loading branch information
leirocks committed Feb 24, 2017
1 parent 6ed25ec commit b4b5287
Show file tree
Hide file tree
Showing 17 changed files with 319 additions and 306 deletions.
18 changes: 6 additions & 12 deletions lib/Jsrt/Core/JsrtContextCore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ JsrtContextCore::JsrtContextCore(JsrtRuntime * runtime) :
{
EnsureScriptContext();
Link();
PinCurrentJsrtContext();
}

/* static */
Expand All @@ -50,22 +49,17 @@ JsrtContextCore *JsrtContextCore::New(JsrtRuntime * runtime)

void JsrtContextCore::Dispose(bool isShutdown)
{
if (nullptr != this->GetJavascriptLibrary())
if (this->GetJavascriptLibrary())
{
Js::ScriptContext* scriptContxt = this->GetJavascriptLibrary()->GetScriptContext();
if (this->GetRuntime()->GetJsrtDebugManager() != nullptr)
{
this->GetRuntime()->GetJsrtDebugManager()->ClearDebugDocument(scriptContxt);
}
scriptContxt->EnsureClearDebugDocument();
scriptContxt->GetDebugContext()->GetProbeContainer()->UninstallInlineBreakpointProbe(NULL);
scriptContxt->GetDebugContext()->GetProbeContainer()->UninstallDebuggerScriptOptionCallback();
scriptContxt->MarkForClose();
this->SetJavascriptLibrary(nullptr);
Unlink();
this->SetJavascriptLibrary(nullptr);
}
}

void JsrtContextCore::Finalize(bool isShutdown)
{
}

Js::ScriptContext* JsrtContextCore::EnsureScriptContext()
{
Assert(this->GetJavascriptLibrary() == nullptr);
Expand Down
1 change: 1 addition & 0 deletions lib/Jsrt/Core/JsrtContextCore.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ class JsrtContextCore sealed : public JsrtContext
{
public:
static JsrtContextCore *New(JsrtRuntime * runtime);
virtual void Finalize(bool isShutdown) override;
virtual void Dispose(bool isShutdown) override;
ChakraCoreHostScriptContext* GetHostScriptContext() const { return hostContext; }

Expand Down
6 changes: 3 additions & 3 deletions lib/Jsrt/Core/JsrtCore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ JsModuleEvaluation(
*result = JS_INVALID_REFERENCE;
}
Js::ScriptContext* scriptContext = moduleRecord->GetScriptContext();
JsrtContext* jsrtContext = (JsrtContext*)scriptContext->GetLibrary()->GetPinnedJsrtContextObject();
JsrtContext* jsrtContext = (JsrtContext*)scriptContext->GetLibrary()->GetJsrtContext();
JsErrorCode errorCode = SetContextAPIWrapper(jsrtContext, [&](Js::ScriptContext *scriptContext) -> JsErrorCode {
SmartFPUControl smartFpuControl;
if (smartFpuControl.HasErr())
Expand Down Expand Up @@ -148,7 +148,7 @@ JsSetModuleHostInfo(
}
Js::SourceTextModuleRecord* moduleRecord = Js::SourceTextModuleRecord::FromHost(requestModule);
Js::ScriptContext* scriptContext = moduleRecord->GetScriptContext();
JsrtContext* jsrtContext = (JsrtContext*)scriptContext->GetLibrary()->GetPinnedJsrtContextObject();
JsrtContext* jsrtContext = (JsrtContext*)scriptContext->GetLibrary()->GetJsrtContext();
JsErrorCode errorCode = SetContextAPIWrapper(jsrtContext, [&](Js::ScriptContext *scriptContext) -> JsErrorCode {
JsrtContextCore* currentContext = static_cast<JsrtContextCore*>(JsrtContextCore::GetCurrent());
switch (moduleHostInfo)
Expand Down Expand Up @@ -186,7 +186,7 @@ JsGetModuleHostInfo(
*hostInfo = nullptr;
Js::SourceTextModuleRecord* moduleRecord = Js::SourceTextModuleRecord::FromHost(requestModule);
Js::ScriptContext* scriptContext = moduleRecord->GetScriptContext();
JsrtContext* jsrtContext = (JsrtContext*)scriptContext->GetLibrary()->GetPinnedJsrtContextObject();
JsrtContext* jsrtContext = (JsrtContext*)scriptContext->GetLibrary()->GetJsrtContext();
JsErrorCode errorCode = SetContextAPIWrapper(jsrtContext, [&](Js::ScriptContext *scriptContext) -> JsErrorCode {
JsrtContextCore* currentContext = static_cast<JsrtContextCore*>(JsrtContextCore::GetCurrent());
switch (moduleHostInfo)
Expand Down
12 changes: 11 additions & 1 deletion lib/Jsrt/Jsrt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,16 @@ CHAKRA_API JsDisposeRuntime(_In_ JsRuntimeHandle runtimeHandle)
runtime->GetJsrtDebugManager()->ClearDebuggerObjects();
}

Js::ScriptContext *scriptContext;
for (scriptContext = threadContext->GetScriptContextList(); scriptContext; scriptContext = scriptContext->next)
{
if (runtime->GetJsrtDebugManager() != nullptr)
{
runtime->GetJsrtDebugManager()->ClearDebugDocument(scriptContext);
}
scriptContext->MarkForClose();
}

// Close any open Contexts.
// We need to do this before recycler shutdown, because ScriptEngine->Close won't work then.
runtime->CloseContexts();
Expand Down Expand Up @@ -807,7 +817,7 @@ CHAKRA_API JsGetContextOfObject(_In_ JsValueRef object, _Out_ JsContextRef *cont
RETURN_NO_EXCEPTION(JsErrorArgumentNotObject);
}
Js::RecyclableObject* obj = Js::RecyclableObject::FromVar(object);
*context = (JsContextRef)obj->GetScriptContext()->GetLibrary()->GetPinnedJsrtContextObject();
*context = (JsContextRef)obj->GetScriptContext()->GetLibrary()->GetJsrtContext();
}
END_JSRT_NO_EXCEPTION
}
Expand Down
14 changes: 4 additions & 10 deletions lib/Jsrt/JsrtContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,10 @@ JsrtContext::JsrtContext(JsrtRuntime * runtime) :
void JsrtContext::SetJavascriptLibrary(Js::JavascriptLibrary * library)
{
this->javascriptLibrary = library;
}

void JsrtContext::PinCurrentJsrtContext()
{
Assert(this->javascriptLibrary);
this->javascriptLibrary->PinJsrtContextObject(this);
if (this->javascriptLibrary)
{
this->javascriptLibrary->SetJsrtContext(this);
}
}

void JsrtContext::Link()
Expand Down Expand Up @@ -109,10 +107,6 @@ bool JsrtContext::TrySetCurrent(JsrtContext * context)
return true;
}

void JsrtContext::Finalize(bool isShutdown)
{
}

void JsrtContext::Mark(Recycler * recycler)
{
AssertMsg(false, "Mark called on object that isn't TrackableObject");
Expand Down
2 changes: 0 additions & 2 deletions lib/Jsrt/JsrtContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ class JsrtContext : public FinalizableObject
static bool TrySetCurrent(JsrtContext * context);
static bool Is(void * ref);

virtual void Finalize(bool isShutdown) override sealed;
virtual void Mark(Recycler * recycler) override sealed;

#if ENABLE_TTD
Expand All @@ -35,7 +34,6 @@ class JsrtContext : public FinalizableObject
void Link();
void Unlink();
void SetJavascriptLibrary(Js::JavascriptLibrary * library);
void PinCurrentJsrtContext();
private:
Field(Js::JavascriptLibrary *) javascriptLibrary;

Expand Down
Loading

0 comments on commit b4b5287

Please sign in to comment.