Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dart: fix race condition between garbage collection of finalizable objects and native function calls #1633

Conversation

pwrobeldev
Copy link
Contributor

----- Motivation -----
If the generated class is not marked with 'Finalizable' interface,
then, when methods are invoked on its object, the object can be
garbage collected in the middle of method execution.

For instance let's take a look at the following example from functional
tests:

'AsyncClass().asyncVoid(false)'

  1. AsyncClass object is created and its lifetime is tied with the native
    finalizer, which deletes shared_ptr (this.handle) that owns resources.
  2. Then, inside 'asyncVoid(false)' we use only 'this.handle' member to
    pass the opaque handle to the native function. For garbage collector
    after the handle is passed 'this' object is no longer used by anybody.
    Therefore, it is not needed – can be finalized.
  3. If the garbage collection kicks in before the native call, then the
    'AsyncClass' object is garbage collected, its finalizer is run and therefore,
    the resources on C++ side are deleted. It causes the segmentation fault.

We have a race condition between the garbage collection and execution
of native functions.

The consequences can be dramatic -- the finalizer registered for
the object may be executed before the native function call inside
the member methods. The finalizers delete C++ resources tied with
the object. This can lead to segmentation fault.

----- Solution -----
The 'Finalizable' interface is a viable solution proposed by Dart:FFI
documentation. It ensures, that 'this' object outlives the whole method
call -- it guarantees, that the native function call finishes before the
finalizer is run.

----- Motivation -----
If the generated class is not marked with 'Finalizable' interface,
then, when methods are invoked on its object, the object can be
garbage collected in the middle of method execution.

For instance let's take a look at the following example from functional
tests:

'AsyncClass().asyncVoid(false)'

1. AsyncClass object is created and its lifetime is tied with the native
   finalizer, which deletes shared_ptr (this.handle) that owns resources.
2. Then, inside 'asyncVoid(false)' we use only 'this.handle' member to
   pass the opaque handle to the native function. From garbage collector’s
   after the handle is passed 'this' object is no longer used by anybody.
   Therefore, it is not needed – can be finalized.
3. If the garbage collection kicks in before the native call, then the
   'AsyncClass' object is garbage collected, its finalizer is run and therefore,
   the resources on C++ side are deleted. It causes the segmentation fault.

We have a race condition between the garbage collection and execution
of native functions.

The consequences can be dramatic -- the finalizer registered for
the object may be executed before the native function call inside
the member methods. The finalizers delete C++ resources tied with
the object. This can lead to segmentation fault.

----- Solution -----
The 'Finalizable' interface is a viable solution proposed by Dart:FFI
documentation. It ensures, that 'this' object outlives the whole method
call -- it guarantees, that the native function call finishes before the
finalizer is run.

Signed-off-by: Patryk Wrobel <183546751+pwrobeldev@users.noreply.github.com>
This change adjusts the output of smoke tests for usage of 'class'
keyword for Dart.

Signed-off-by: Patryk Wrobel <183546751+pwrobeldev@users.noreply.github.com>
If the generated class for interface is not marked with 'Finalizable',
then when methods are invoked on its object, the object can be
garbage collected in the middle of execution of the method.

The consequences can be dramatic -- the finalizer registered for
the object may be executed before the native function call inside
the member methods. This can lead to segmentation fault.

The 'Finalizable' interface ensures, that the object outlives the
native function call.

Signed-off-by: Patryk Wrobel <183546751+pwrobeldev@users.noreply.github.com>
This change adjusts the output of smoke tests for usage of 'interface'
keyword for Dart.

Signed-off-by: Patryk Wrobel <183546751+pwrobeldev@users.noreply.github.com>
If the generated class for lambda is not marked with 'Finalizable',
then when 'call()' is invoked on its object, the object can be
garbage collected in the middle of execution of the method.

The consequences can be dramatic -- the finalizer registered for
the object may be executed before the native function call inside
the member methods. This can lead to segmentation fault.

The 'Finalizable' interface ensures, that the object outlives the
native function call.

Signed-off-by: Patryk Wrobel <183546751+pwrobeldev@users.noreply.github.com>
This change adjusts the output of smoke tests for usage of 'lambda'
keyword for Dart.

Signed-off-by: Patryk Wrobel <183546751+pwrobeldev@users.noreply.github.com>
If the generated class is not marked with 'Finalizable',
then when a method is invoked on its object, the object can be
garbage collected in the middle of execution of the method.

The consequences can be dramatic -- the finalizer registered for
the object may be executed before the native function call inside
the member methods. This can lead to segmentation fault.

The 'Finalizable' interface ensures, that the object outlives the
native function call.

Signed-off-by: Patryk Wrobel <183546751+pwrobeldev@users.noreply.github.com>
This change adjusts the output of smoke tests for usage of 'LazyList'
for Dart.

Signed-off-by: Patryk Wrobel <183546751+pwrobeldev@users.noreply.github.com>
Signed-off-by: Patryk Wrobel <183546751+pwrobeldev@users.noreply.github.com>
'Finalizer' marker was introduced to FFI in SDK 2.17.0.
The CI jobs had been using much older SDK: 2.12.

The latest stable Dart SDK is 3.6.0. Therefore, the upgrade
is minor.

Signed-off-by: Patryk Wrobel <183546751+pwrobeldev@users.noreply.github.com>
@pwrobeldev pwrobeldev marked this pull request as ready for review December 12, 2024 07:11
@pwrobeldev
Copy link
Contributor Author

The problem was reproducible in functional tests. On average 1 in ~15 runs failed.

Please find a few stack traces from functional tests, which show the invalid behavior.

Async_test.dart:

5   libfunctional.dylib           	       0x1054a9ea0 std::__1::__shared_count::__add_shared[abi:ue170006]() + 28
6   libfunctional.dylib           	       0x1054a9e78 std::__1::__shared_weak_count::__add_shared[abi:ue170006]() + 24
7   libfunctional.dylib           	       0x1059ef35c std::__1::shared_ptr<test::AsyncClass>::shared_ptr[abi:ue170006](std::__1::shared_ptr<test::AsyncClass> const&) + 92
8   libfunctional.dylib           	       0x105625b38 std::__1::shared_ptr<test::AsyncClass>::shared_ptr[abi:ue170006](std::__1::shared_ptr<test::AsyncClass> const&) + 36
9   libfunctional.dylib           	       0x105624cd0 lorem_ipsum::test::ffi::Conversion<std::__1::shared_ptr<test::AsyncClass>, void>::toCpp(void*) + 56
10  libfunctional.dylib           	       0x105624ba8 functional_test_AsyncClass_asyncVoid__asyncVoid__resultLambda_Boolean + 72

Inheritance_test.dart:

5   libfunctional.dylib           	       0x107227c64 std::__1::__shared_count::__add_shared[abi:ue170006]() + 28
6   libfunctional.dylib           	       0x107227c3c std::__1::__shared_weak_count::__add_shared[abi:ue170006]() + 24
7   libfunctional.dylib           	       0x10722d99c std::__1::shared_ptr<test::SimpleInstantiableTwo>::shared_ptr[abi:ue170006](std::__1::shared_ptr<test::SimpleInstantiableTwo> const&) + 92
8   libfunctional.dylib           	       0x10722b0e0 std::__1::shared_ptr<test::SimpleInstantiableTwo>::shared_ptr[abi:ue170006](std::__1::shared_ptr<test::SimpleInstantiableTwo> const&) + 36
9   libfunctional.dylib           	       0x107445de0 functional_test_SimpleInstantiableTwo_copy_handle + 108

ExternalTypes_test.dart:

5   libfunctional.dylib           	       0x106774670 std::__1::__shared_count::__add_shared[abi:ue170006]() + 28
6   libfunctional.dylib           	       0x106774648 std::__1::__shared_weak_count::__add_shared[abi:ue170006]() + 24
7   libfunctional.dylib           	       0x106dc5a58 std::__1::shared_ptr<test::UseMyClass>::shared_ptr[abi:ue170006](std::__1::shared_ptr<test::UseMyClass> const&) + 92
8   libfunctional.dylib           	       0x1069bfecc std::__1::shared_ptr<test::UseMyClass>::shared_ptr[abi:ue170006](std::__1::shared_ptr<test::UseMyClass> const&) + 36
9   libfunctional.dylib           	       0x1069bfc84 lorem_ipsum::test::ffi::Conversion<std::__1::shared_ptr<test::UseMyClass>, void>::toCpp(void*) + 56
10  libfunctional.dylib           	       0x1069bfb78 functional_test_UseMyClass_callBar__MyClass + 48

In all mentioned cases accessing the converted opaque handle caused segmentation faults. The memory was freed because of the race condition.

Please find some debug logs from one of failed cases:

1: 00:00 +0: Async: Async void
1: 
1: [DART] create(): handle: = 0x600003c18250
1: [DART] _asyncVoid__async(): _handle: = 0x600003c18250
1: 
1: 
1: [C++] Finalizing handle: 0x600003c18250
1: [C++] call function -- handle: 0x600003c18250
1: 
1: ===== CRASH =====
1: si_signo=Segmentation fault: 11(11), si_code=SEGV_ACCERR(2), si_addr=0x803

The above is an evidence of invalid behavior.

@pwrobeldev
Copy link
Contributor Author

In order to fix the problem the Finalizable marker interface can be used. We are hitting the exact problem, which is solved by Finalizable and described in the documentation.

According to the documentation:

Methods on a class implementing Finalizable keep the this object alive for the duration of the method execution. The this value is treated like a local variable.

For example, this is kept alive during the execution of someNativeCall in myFunction:

class MyFinalizable implements Finalizable {
  final Pointer nativeResource;

  MyFinalizable(this.nativeResource);

  void myFunction() {
    someNativeCall(nativeResource);
  }
}

void someNativeCall(Pointer nativeResource) {
  // ..
}

Reference:

@pwrobeldev
Copy link
Contributor Author

A few notes for reviewers:

  • the functional tests were run 100+ times with the fix and they passed each and every time; without the fix the failure was visible once in ~15 runs -- therefore, I assume that the race is fixed
  • Dart SDK version used on CI was upgraded from 2.12 to 2.18, because Finalizable has been available since 2.17 -- it is a minor upgrade -- the last stable version is 3+
  • The actual changes in mustache templates are tiny -- the remaining changes are smoke tests, which are generated

The PR is intended to be reviewed commit-by-commit. The non-interesting changes related to the output of smoke tests are present in separate commits.

@pwrobeldev pwrobeldev merged commit 2167a72 into master Dec 12, 2024
15 of 17 checks passed
@pwrobeldev pwrobeldev deleted the pwrobeldev/fix-race-condition-between-gc-plus-finalizer-and-native-call branch December 12, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants