From 4df2a9515dd176dfa601ab10a1de4fb1e126d185 Mon Sep 17 00:00:00 2001 From: Liam Appelbe Date: Wed, 17 Sep 2025 18:16:01 -0700 Subject: [PATCH] [vm] Simplify FfiCallbackMetadata locking Remove the locks in DLRT_GetFfiCallbackMetadata, and switch from safepoint locks to ordinary locks in FfiCallbackMetadata. This fixes the deadlock bugs at the cost of reducing thread safety in error cases. Some cases that would have failed gracefully will now have undefined behavior. Also, FATAL instead of no-op if a dead callback is invoked. Fixes: https://github.com/dart-lang/sdk/issues/61372 Change-Id: Ie09fca3c629ad61b2ffbdd029269338f2706df4b TEST=CI, particularly many_listener_callbacks_test on reload bot Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/449160 Commit-Queue: Liam Appelbe Reviewed-by: Martin Kustermann Reviewed-by: Slava Egorov --- runtime/vm/ffi_callback_metadata.cc | 7 +- runtime/vm/ffi_callback_metadata.h | 12 +- runtime/vm/ffi_callback_metadata_test.cc | 42 +++-- runtime/vm/runtime_entry.cc | 164 ++++++++++-------- .../async_void_function_callbacks_test.dart | 17 -- 5 files changed, 128 insertions(+), 114 deletions(-) diff --git a/runtime/vm/ffi_callback_metadata.cc b/runtime/vm/ffi_callback_metadata.cc index 8154b375b78..117ad4b741a 100644 --- a/runtime/vm/ffi_callback_metadata.cc +++ b/runtime/vm/ffi_callback_metadata.cc @@ -251,7 +251,7 @@ FfiCallbackMetadata::Trampoline FfiCallbackMetadata::CreateMetadataEntry( uword target_entry_point, uint64_t context, MetadataEntry** list_head) { - SafepointMutexLocker locker(&lock_); + MutexLocker locker(&lock_); EnsureFreeListNotEmptyLocked(); ASSERT(free_list_head_ != nullptr); MetadataEntry* entry = free_list_head_; @@ -316,7 +316,7 @@ void FfiCallbackMetadata::DeleteAllCallbacks(MetadataEntry** list_head) { void FfiCallbackMetadata::DeleteCallback(Trampoline trampoline, MetadataEntry** list_head) { - SafepointMutexLocker locker(&lock_); + MutexLocker locker(&lock_); auto* entry = MetadataEntryOfTrampoline(trampoline); ASSERT(entry->metadata()->IsLive()); auto* prev = entry->list_prev_; @@ -561,7 +561,8 @@ FfiCallbackMetadata::MetadataEntryOfTrampoline(Trampoline trampoline) const { #endif } -FfiCallbackMetadata::Metadata FfiCallbackMetadata::LookupMetadataForTrampoline( +FfiCallbackMetadata::Metadata +FfiCallbackMetadata::LookupMetadataForTrampolineUnlocked( Trampoline trampoline) const { return *MetadataEntryOfTrampoline(trampoline)->metadata(); } diff --git a/runtime/vm/ffi_callback_metadata.h b/runtime/vm/ffi_callback_metadata.h index 2dc90bb34d5..79bff2c411f 100644 --- a/runtime/vm/ffi_callback_metadata.h +++ b/runtime/vm/ffi_callback_metadata.h @@ -256,10 +256,7 @@ class FfiCallbackMetadata { }; // Returns the Metadata object for the given trampoline. - Metadata LookupMetadataForTrampoline(Trampoline trampoline) const; - - // The mutex that guards creation and destruction of callbacks. - Mutex* lock() { return &lock_; } + Metadata LookupMetadataForTrampolineUnlocked(Trampoline trampoline) const; // The number of trampolines that can be stored on a single page. static constexpr intptr_t NumCallbackTrampolinesPerPage() { @@ -351,6 +348,13 @@ class FfiCallbackMetadata { ClosurePtr closure_ptr); // Visible for testing. +#if defined(TESTING) + + public: +#else // TESTING + + private: +#endif // TESTING MetadataEntry* MetadataEntryOfTrampoline(Trampoline trampoline) const; Trampoline TrampolineOfMetadataEntry(MetadataEntry* metadata) const; diff --git a/runtime/vm/ffi_callback_metadata_test.cc b/runtime/vm/ffi_callback_metadata_test.cc index 515069cd829..8496a6b3ff2 100644 --- a/runtime/vm/ffi_callback_metadata_test.cc +++ b/runtime/vm/ffi_callback_metadata_test.cc @@ -102,7 +102,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateSyncFfiCallback) { { FfiCallbackMetadata::Metadata m1 = - fcm->LookupMetadataForTrampoline(tramp1); + fcm->LookupMetadataForTrampolineUnlocked(tramp1); EXPECT(m1.IsLive()); EXPECT_EQ(m1.target_isolate(), isolate); EXPECT_EQ(m1.target_entry_point(), code.EntryPoint()); @@ -124,7 +124,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateSyncFfiCallback) { { FfiCallbackMetadata::Metadata m2 = - fcm->LookupMetadataForTrampoline(tramp2); + fcm->LookupMetadataForTrampolineUnlocked(tramp2); EXPECT(m2.IsLive()); EXPECT_EQ(m2.target_isolate(), isolate); EXPECT_EQ(m2.target_entry_point(), code.EntryPoint()); @@ -147,7 +147,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateSyncFfiCallback) { { isolate->DeleteFfiCallback(tramp1); FfiCallbackMetadata::Metadata m1 = - fcm->LookupMetadataForTrampoline(tramp1); + fcm->LookupMetadataForTrampolineUnlocked(tramp1); EXPECT(!m1.IsLive()); // head -> tramp2 @@ -160,10 +160,12 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateSyncFfiCallback) { { // Isolate has shut down, so all callbacks should be deleted. - FfiCallbackMetadata::Metadata m1 = fcm->LookupMetadataForTrampoline(tramp1); + FfiCallbackMetadata::Metadata m1 = + fcm->LookupMetadataForTrampolineUnlocked(tramp1); EXPECT(!m1.IsLive()); - FfiCallbackMetadata::Metadata m2 = fcm->LookupMetadataForTrampoline(tramp2); + FfiCallbackMetadata::Metadata m2 = + fcm->LookupMetadataForTrampolineUnlocked(tramp2); EXPECT(!m2.IsLive()); } } @@ -197,7 +199,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateAsyncFfiCallback) { { FfiCallbackMetadata::Metadata m1 = - fcm->LookupMetadataForTrampoline(tramp1); + fcm->LookupMetadataForTrampolineUnlocked(tramp1); EXPECT(m1.IsLive()); EXPECT_EQ(m1.target_isolate(), isolate); EXPECT_EQ(m1.target_entry_point(), code.EntryPoint()); @@ -219,7 +221,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateAsyncFfiCallback) { { FfiCallbackMetadata::Metadata m2 = - fcm->LookupMetadataForTrampoline(tramp2); + fcm->LookupMetadataForTrampolineUnlocked(tramp2); EXPECT(m2.IsLive()); EXPECT_EQ(m2.target_isolate(), isolate); EXPECT_EQ(m2.target_entry_point(), code.EntryPoint()); @@ -242,7 +244,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateAsyncFfiCallback) { { isolate->DeleteFfiCallback(tramp2); FfiCallbackMetadata::Metadata m2 = - fcm->LookupMetadataForTrampoline(tramp2); + fcm->LookupMetadataForTrampolineUnlocked(tramp2); EXPECT(!m2.IsLive()); // head -> tramp1 @@ -255,10 +257,12 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateAsyncFfiCallback) { { // Isolate has shut down, so all callbacks should be deleted. - FfiCallbackMetadata::Metadata m1 = fcm->LookupMetadataForTrampoline(tramp1); + FfiCallbackMetadata::Metadata m1 = + fcm->LookupMetadataForTrampolineUnlocked(tramp1); EXPECT(!m1.IsLive()); - FfiCallbackMetadata::Metadata m2 = fcm->LookupMetadataForTrampoline(tramp2); + FfiCallbackMetadata::Metadata m2 = + fcm->LookupMetadataForTrampolineUnlocked(tramp2); EXPECT(!m2.IsLive()); } } @@ -299,7 +303,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateIsolateLocalFfiCallback) { { FfiCallbackMetadata::Metadata m1 = - fcm->LookupMetadataForTrampoline(tramp1); + fcm->LookupMetadataForTrampolineUnlocked(tramp1); EXPECT(m1.IsLive()); EXPECT_EQ(m1.target_isolate(), isolate); EXPECT_EQ(m1.target_entry_point(), code.EntryPoint()); @@ -323,7 +327,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateIsolateLocalFfiCallback) { { FfiCallbackMetadata::Metadata m2 = - fcm->LookupMetadataForTrampoline(tramp2); + fcm->LookupMetadataForTrampolineUnlocked(tramp2); EXPECT(m2.IsLive()); EXPECT_EQ(m2.target_isolate(), isolate); EXPECT_EQ(m2.target_entry_point(), code.EntryPoint()); @@ -346,7 +350,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateIsolateLocalFfiCallback) { { isolate->DeleteFfiCallback(tramp2); FfiCallbackMetadata::Metadata m2 = - fcm->LookupMetadataForTrampoline(tramp2); + fcm->LookupMetadataForTrampolineUnlocked(tramp2); EXPECT(!m2.IsLive()); // head -> tramp1 @@ -359,10 +363,12 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateIsolateLocalFfiCallback) { { // Isolate has shut down, so all callbacks should be deleted. - FfiCallbackMetadata::Metadata m1 = fcm->LookupMetadataForTrampoline(tramp1); + FfiCallbackMetadata::Metadata m1 = + fcm->LookupMetadataForTrampolineUnlocked(tramp1); EXPECT(!m1.IsLive()); - FfiCallbackMetadata::Metadata m2 = fcm->LookupMetadataForTrampoline(tramp2); + FfiCallbackMetadata::Metadata m2 = + fcm->LookupMetadataForTrampolineUnlocked(tramp2); EXPECT(!m2.IsLive()); } } @@ -460,7 +466,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_DeleteTrampolines) { // Verify all the callbacks. for (FfiCallbackMetadata::Trampoline tramp : tramps) { - auto metadata = fcm->LookupMetadataForTrampoline(tramp); + auto metadata = fcm->LookupMetadataForTrampolineUnlocked(tramp); EXPECT(metadata.IsLive()); EXPECT_EQ(metadata.target_isolate(), isolate); EXPECT_EQ(static_cast(metadata.trampoline_type()), @@ -494,7 +500,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_DeleteTrampolines) { fcm->DeleteAllCallbacks(&list_head); EXPECT_EQ(list_head, nullptr); for (FfiCallbackMetadata::Trampoline tramp : tramps) { - EXPECT(!fcm->LookupMetadataForTrampoline(tramp).IsLive()); + EXPECT(!fcm->LookupMetadataForTrampolineUnlocked(tramp).IsLive()); } } @@ -574,7 +580,7 @@ static void RunBigRandomMultithreadedTest(uint64_t seed) { // Verify all the callbacks. for (const auto& tramp : tramps) { - auto metadata = fcm->LookupMetadataForTrampoline(tramp.tramp); + auto metadata = fcm->LookupMetadataForTrampolineUnlocked(tramp.tramp); EXPECT(metadata.IsLive()); EXPECT_EQ(metadata.target_isolate(), isolate); if (metadata.trampoline_type() == diff --git a/runtime/vm/runtime_entry.cc b/runtime/vm/runtime_entry.cc index 6805d37bf23..dbfecb1c82f 100644 --- a/runtime/vm/runtime_entry.cc +++ b/runtime/vm/runtime_entry.cc @@ -4962,85 +4962,58 @@ DEFINE_LEAF_RUNTIME_ENTRY(ExitSafepoint, /*argument_count=*/0, DLRT_ExitSafepoint); -// This is called by a native callback trampoline -// (see StubCodeCompiler::GenerateFfiCallbackTrampolineStub). Not registered as -// a runtime entry because we can't use Thread to look it up. -extern "C" Thread* DLRT_GetFfiCallbackMetadata( - FfiCallbackMetadata::Trampoline trampoline, - uword* out_entry_point, - uword* out_trampoline_type) { - CHECK_STACK_ALIGNMENT; - TRACE_RUNTIME_CALL("GetFfiCallbackMetadata %p", - reinterpret_cast(trampoline)); - ASSERT(out_entry_point != nullptr); - ASSERT(out_trampoline_type != nullptr); +namespace { +Thread* HandleAsyncFfiCallback(FfiCallbackMetadata::Metadata metadata, + uword* out_entry_point, + uword* out_trampoline_type) { + // NOTE: This is only thread safe if the user is using the API correctly. + // Otherwise, the callback could have been deleted and replaced, in which case + // IsLive would still be true. Or it could have been deleted after we looked + // it up, and the target isolate could be shut down. We delay recycling + // callbacks as long as possible, so this check is better than nothing, but + // it's not infallible. Ultimately it's the user's responsibility to avoid use + // after free errors. Trying to lock FfiCallbackMetadata::lock_, or any + // similar lock, leads to deadlocks. - if (!Isolate::IsolateCreationEnabled()) { - TRACE_RUNTIME_CALL("GetFfiCallbackMetadata called after shutdown %p", - reinterpret_cast(trampoline)); - return nullptr; - } + *out_trampoline_type = static_cast(metadata.trampoline_type()); + *out_entry_point = metadata.target_entry_point(); + Isolate* target_isolate = metadata.target_isolate(); + Isolate* current_isolate = nullptr; Thread* current_thread = Thread::Current(); - auto* fcm = FfiCallbackMetadata::Instance(); - auto metadata = fcm->LookupMetadataForTrampoline(trampoline); - - // Is this an async callback? - if (metadata.trampoline_type() == - FfiCallbackMetadata::TrampolineType::kAsync) { - // It's possible that the callback was deleted, or the target isolate was - // shut down, in between looking up the metadata above, and this point. So - // grab the lock and then check that the callback is still alive. - MutexLocker locker(fcm->lock()); - auto metadata2 = fcm->LookupMetadataForTrampoline(trampoline); - *out_trampoline_type = static_cast(metadata2.trampoline_type()); - - // Check IsLive, but also check that the metdata hasn't changed. This is - // for the edge case that the callback was destroyed and recycled in between - // the two lookups. - if (!metadata.IsLive() || !metadata.IsSameCallback(metadata2)) { - TRACE_RUNTIME_CALL("GetFfiCallbackMetadata callback deleted %p", - reinterpret_cast(trampoline)); - return nullptr; + if (current_thread != nullptr) { + current_isolate = current_thread->isolate(); + if (current_thread->execution_state() != Thread::kThreadInNative) { + FATAL("Cannot invoke native callback from a leaf call."); } - - *out_entry_point = metadata.target_entry_point(); - Isolate* target_isolate = metadata.target_isolate(); - - Isolate* current_isolate = nullptr; - if (current_thread != nullptr) { - current_isolate = current_thread->isolate(); - if (current_thread->execution_state() != Thread::kThreadInNative) { - FATAL("Cannot invoke native callback from a leaf call."); - } - current_thread->ExitSafepointFromNative(); - current_thread->set_execution_state(Thread::kThreadInVM); - } - - // Enter the temporary isolate. If the current isolate is in the same group - // as the target isolate, we can skip entering the temp isolate, and marshal - // the args on the current isolate. - if (current_isolate == nullptr || - current_isolate->group() != target_isolate->group()) { - if (current_isolate != nullptr) { - Thread::ExitIsolate(/*isolate_shutdown=*/false); - } - target_isolate->group()->EnterTemporaryIsolate(); - } - Thread* const temp_thread = Thread::Current(); - ASSERT(temp_thread != nullptr); - temp_thread->set_unboxed_int64_runtime_arg(metadata.send_port()); - temp_thread->set_unboxed_int64_runtime_second_arg( - reinterpret_cast(current_isolate)); - ASSERT(!temp_thread->IsAtSafepoint()); - return temp_thread; + current_thread->ExitSafepointFromNative(); + current_thread->set_execution_state(Thread::kThreadInVM); } - // Otherwise, this is a sync callback, so verify that we're already entered - // into the target isolate. - if (!metadata.IsLive()) { - FATAL("Callback invoked after it has been deleted."); + // Enter the temporary isolate. If the current isolate is in the same group + // as the target isolate, we can skip entering the temp isolate, and marshal + // the args on the current isolate. + if (current_isolate == nullptr || + current_isolate->group() != target_isolate->group()) { + if (current_isolate != nullptr) { + Thread::ExitIsolate(/*isolate_shutdown=*/false); + } + target_isolate->group()->EnterTemporaryIsolate(); } + Thread* const temp_thread = Thread::Current(); + ASSERT(temp_thread != nullptr); + temp_thread->set_unboxed_int64_runtime_arg(metadata.send_port()); + temp_thread->set_unboxed_int64_runtime_second_arg( + reinterpret_cast(current_isolate)); + ASSERT(!temp_thread->IsAtSafepoint()); + return temp_thread; +} + +Thread* HandleSyncFfiCallback(FfiCallbackMetadata::Metadata metadata, + uword* out_entry_point, + uword* out_trampoline_type) { + Thread* current_thread = Thread::Current(); + if (metadata.is_isolate_group_bound()) { *out_entry_point = metadata.target_entry_point(); *out_trampoline_type = static_cast(metadata.trampoline_type()); @@ -5102,6 +5075,53 @@ extern "C" Thread* DLRT_GetFfiCallbackMetadata( (void*)*out_trampoline_type); return current_thread; } +} // namespace + +// This is called by a native callback trampoline +// (see StubCodeCompiler::GenerateFfiCallbackTrampolineStub). Not registered as +// a runtime entry because we can't use Thread to look it up. +extern "C" Thread* DLRT_GetFfiCallbackMetadata( + FfiCallbackMetadata::Trampoline trampoline, + uword* out_entry_point, + uword* out_trampoline_type) { + CHECK_STACK_ALIGNMENT; + TRACE_RUNTIME_CALL("GetFfiCallbackMetadata %p", + reinterpret_cast(trampoline)); + ASSERT(out_entry_point != nullptr); + ASSERT(out_trampoline_type != nullptr); + + if (!Isolate::IsolateCreationEnabled()) { + FATAL("GetFfiCallbackMetadata called after shutdown %p", + reinterpret_cast(trampoline)); + } + + // NOTE: We access the metadata for `trampoline` without a lock. This is safe + // because nobody will touch the metadata of the `trampoline` until it's + // deleted and the `NativeCallable` API requires the isolate to keep the + // trampoline (and therefore the metadata) alive until C code no longer + // attempts to call it. + // + // If a user of the `NativeCallable` API violates this agreement, we may + // have a use-after-free scenario here and therefore undefined behavior. + // We make some best effort to `FATAL()` in obvious cases of undefined + // behavior, but not all cases will be caught. + auto metadata = + FfiCallbackMetadata::Instance()->LookupMetadataForTrampolineUnlocked( + trampoline); + + if (!metadata.IsLive()) { + FATAL("Callback invoked after it has been deleted."); + } + + if (metadata.trampoline_type() == + FfiCallbackMetadata::TrampolineType::kAsync) { + return HandleAsyncFfiCallback(metadata, out_entry_point, + out_trampoline_type); + } else { + return HandleSyncFfiCallback(metadata, out_entry_point, + out_trampoline_type); + } +} extern "C" void DLRT_ExitIsolateGroupBoundIsolate() { TRACE_RUNTIME_CALL("ExitIsolateGroupBoundIsolate%s", ""); diff --git a/tests/ffi/async_void_function_callbacks_test.dart b/tests/ffi/async_void_function_callbacks_test.dart index 424a0492821..9246b5c47d2 100644 --- a/tests/ffi/async_void_function_callbacks_test.dart +++ b/tests/ffi/async_void_function_callbacks_test.dart @@ -33,7 +33,6 @@ main(args, message) async { // Simple tests. await testNativeCallableHelloWorld(); testNativeCallableDoubleCloseError(); - await testNativeCallableUseAfterFree(); await testNativeCallableNestedCloseCall(); await testNativeCallableThrowInsideCallback(); await testNativeCallableDontKeepAlive(); @@ -96,22 +95,6 @@ testNativeCallableDoubleCloseError() { Expect.isFalse(callback.keepIsolateAlive); } -Future testNativeCallableUseAfterFree() async { - final lib = NativeLibrary(); - - final callback = NativeCallable.listener(simpleFunction); - final nativeFunction = callback.nativeFunction; - callback.close(); - - simpleFunctionResult = Completer(); - lib.callFunctionOnSameThread(123, nativeFunction); - - await Future.delayed(Duration(milliseconds: 100)); - - // The callback wasn't invoked, but we didn't crash either. - Expect.equals(false, simpleFunctionResult.isCompleted); -} - NativeCallable? simpleFunctionAndCloseSelf_callable; void simpleFunctionAndCloseSelf(int a, int b) { simpleFunctionAndCloseSelf_callable!.close();