dart-lang/sdk
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

[vm] Simplify FfiCallbackMetadata locking

Browse Source 
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 <liama@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
Reviewed-by: Slava Egorov <vegorov@google.com>
This commit is contained in:
Liam Appelbe
2025-09-17 18:16:01 -07:00
committed by Commit Queue
parent d866054c4d
commit 4df2a9515d
5 changed files with 128 additions and 114 deletions
Download Patch File Download Diff File Expand all files Collapse all files
runtime/vm/ffi_callback_metadata.cc
+4 -3
View File
@@ -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();
}
runtime/vm/ffi_callback_metadata.h
+8 -4
View File
@@ -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;
runtime/vm/ffi_callback_metadata_test.cc
+24 -18
View File
@@ -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<int>(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() ==
runtime/vm/runtime_entry.cc
+92 -72
View File
@@ -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<void*>(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<void*>(trampoline));
return nullptr;
}
*out_trampoline_type = static_cast<uword>(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<uword>(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<void*>(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<intptr_t>(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<intptr_t>(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<uword>(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<void*>(trampoline));
ASSERT(out_entry_point != nullptr);
ASSERT(out_trampoline_type != nullptr);
if (!Isolate::IsolateCreationEnabled()) {
FATAL("GetFfiCallbackMetadata called after shutdown %p",
reinterpret_cast<void*>(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", "");
tests/ffi/async_void_function_callbacks_test.dart
-17
View File
@@ -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<void> testNativeCallableUseAfterFree() async {
final lib = NativeLibrary();
final callback = NativeCallable<CallbackNativeType>.listener(simpleFunction);
final nativeFunction = callback.nativeFunction;
callback.close();
simpleFunctionResult = Completer<int>();
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();