Don't leak CFunctionInfo and CTypeInfo #832
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Note that this was done before and reverted, there is an re-apply PR for this at #714, but AFAIU it's blocked on denoland/deno#24169 passing CI. |
|
It looks like this PR is passing CI in deno, but the diff looks essentially identical (the only difference being putting that args and ret into a tuple instead of separate fields). Why does this work while the original PR failed? |
|
I've been trying to figure that out but its likely that the failures were caused by something unrelated back then. This one is passing so i'll add the original author as co-author and merge. |
|
Thanks! |
bartlomieju
added a commit
to denoland/deno
that referenced
this pull request
Jul 21, 2024
Trying out the deno_core patch Ref denoland/deno_core#832 Closes #24575 --------- Co-authored-by: Bartek Iwańczuk <[email protected]>
bartlomieju
added a commit
to denoland/deno
that referenced
this pull request
Jul 22, 2024
Trying out the deno_core patch Ref denoland/deno_core#832 Closes #24575 --------- Co-authored-by: Bartek Iwańczuk <[email protected]>
nathanwhit
added a commit
that referenced
this pull request
Jul 23, 2024
Fixes crashes caused by #832 by making it the responsibility of `InnerIsolateState` to free the cfunctioninfo/ctypeinfo _after_ dropping the isolate, instead of the `OpCtx` freeing those on drop. The cause of the crash is that the `OpCtx` gets dropped when the realm is destroyed (and we free the fast api info), but the isolate still lives because we don't destroy the isolate until [after destroying the realm](https://github.com/denoland/deno_core/blob/19d2d603e506b4d6b6aa1f88c092dbd59f4000b6/core/runtime/jsrealm.rs#L183-L184). In that time before we destroy the isolate, a V8 worker thread still has a reference to the dropped CFunctionInfo, and boom UAF. Here's a proper stacktrace for the check failure ``` # # Fatal error in , line 0 # Check failed: c_argument_count >= kReceiver. # # # #FailureMessage Object: 0x17031cf28 ==== C stack trace =============================== 0 url_ops-d558488f29975416 0x0000000100a1aff4 v8::base::debug::StackTrace::StackTrace() + 24 1 url_ops-d558488f29975416 0x0000000100a1f854 v8::platform::(anonymous namespace)::PrintStackTrace() + 24 2 url_ops-d558488f29975416 0x0000000100a17cf8 V8_Fatal(char const*, ...) + 356 3 url_ops-d558488f29975416 0x00000001018854c4 v8::internal::compiler::FastApiCallReducerAssembler::ReduceFastApiCall() + 1608 4 url_ops-d558488f29975416 0x00000001018841a0 v8::internal::compiler::JSCallReducer::ReduceCallApiFunction(v8::internal::compiler::Node*, v8::internal::compiler::SharedFunctionInfoRef) + 856 5 url_ops-d558488f29975416 0x0000000101886bac v8::internal::compiler::JSCallReducer::ReduceJSCall(v8::internal::compiler::Node*, v8::internal::compiler::SharedFunctionInfoRef) + 356 6 url_ops-d558488f29975416 0x000000010187d564 v8::internal::compiler::JSCallReducer::ReduceJSCall(v8::internal::compiler::Node*) + 1120 7 url_ops-d558488f29975416 0x000000010187d720 v8::internal::compiler::JSCallReducer::ReduceJSCall(v8::internal::compiler::Node*) + 1564 8 url_ops-d558488f29975416 0x0000000101796c8c v8::internal::compiler::GraphReducer::Reduce(v8::internal::compiler::Node*) + 180 9 url_ops-d558488f29975416 0x00000001017968e4 v8::internal::compiler::GraphReducer::ReduceTop() + 584 10 url_ops-d558488f29975416 0x00000001017964d4 v8::internal::compiler::GraphReducer::ReduceNode(v8::internal::compiler::Node*) + 172 11 url_ops-d558488f29975416 0x00000001018d96a8 v8::internal::compiler::InliningPhase::Run(v8::internal::compiler::TFPipelineData*, v8::internal::Zone*) + 704 12 url_ops-d558488f29975416 0x00000001018cd664 auto v8::internal::compiler::PipelineImpl::Run<v8::internal::compiler::InliningPhase>() + 132 13 url_ops-d558488f29975416 0x00000001018ca290 v8::internal::compiler::PipelineImpl::CreateGraph() + 232 14 url_ops-d558488f29975416 0x00000001018c9edc v8::internal::compiler::PipelineCompilationJob::ExecuteJobImpl(v8::internal::RuntimeCallStats*, v8::internal::LocalIsolate*) + 248 15 url_ops-d558488f29975416 0x0000000100aea274 v8::internal::OptimizedCompilationJob::ExecuteJob(v8::internal::RuntimeCallStats*, v8::internal::LocalIsolate*) + 80 16 url_ops-d558488f29975416 0x0000000100b1d9e8 v8::internal::OptimizingCompileDispatcher::CompileNext(v8::internal::TurbofanCompilationJob*, v8::internal::LocalIsolate*) + 44 17 url_ops-d558488f29975416 0x0000000100b1f298 v8::internal::OptimizingCompileDispatcher::CompileTask::Run(v8::JobDelegate*) + 396 18 url_ops-d558488f29975416 0x0000000100a1c75c v8::platform::DefaultJobWorker::Run() + 216 19 url_ops-d558488f29975416 0x0000000100a20d3c v8::platform::DefaultWorkerThreadsTaskRunner::WorkerThread::Run() + 160 20 url_ops-d558488f29975416 0x0000000100a1a968 v8::base::ThreadEntry(void*) + 160 21 libsystem_pthread.dylib 0x00000001810d6f94 _pthread_start + 136 ``` The opctx gets dropped during [JsRuntime::cleanup](https://github.com/denoland/deno_core/blob/19d2d603e506b4d6b6aa1f88c092dbd59f4000b6/core/runtime/jsruntime.rs#L175-L176), called in [InnerIsolateState::drop](https://github.com/denoland/deno_core/blob/19d2d603e506b4d6b6aa1f88c092dbd59f4000b6/core/runtime/jsruntime.rs#L207), then we [drop the isolate](https://github.com/denoland/deno_core/blob/19d2d603e506b4d6b6aa1f88c092dbd59f4000b6/core/runtime/jsruntime.rs#L218) to destroy it
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #649
Fixes #806