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();