Closed voxik closed 1 year ago
I have reduced the reproducer:
$ cat fiddle_fork.rb
require 'fiddle/import'
module Fiddle
module LIBC
extend Importer
dlload "libc.so.6", "libm.so.6"
CallCallback = bind("void call_callback(void*, void*)"){ | ptr1, ptr2| }
end
end
error, pid, status = IO.pipe do |r, w|
pid = fork {}
w.close
[r.read, *Process.wait2(pid)]
end
$ RUBYLIB=.:lib:.ext/common:.ext/x86_64-linux:tool/lib LD_LIBRARY_PATH=. ./ruby fiddle_fork.rb
Aborted (core dumped)
I think this might be similar to ffi/ffi/issues/621
I have also reported this issue in Fedora:
I tested this on Ubuntu (with clang and gcc) and macOS with clang using libffi version 3.4.2 but I'm not able to reproduce this. Since the problem started happening after upgrading libffi, could it be a problem with libffi rather than a problem with Fiddle?
We looked at this issue closer, and it seems to be composed of SELinux, FFI Closure, and Forking the Ruby process.
From the linked bug: "In modern libffi 3.4.2 we use memfd_create quite eagerly for SELinux protected systems and such closures are shared between the parent and child process." and "Closures may be inherited by the child, and so the parent and child must coordinate the usage.".
Which would be consistent with our findings.
See the following minimal reproducer that triggers the bug with Fedora 36 and later:
require 'fiddle/closure'
require 'fiddle/struct'
Fiddle::Closure.new(Fiddle::TYPE_VOID, [])
fork { }
GC.start
This describes all required to trigger the bug on the Ruby side. Note that SELinux plays a role in this as well since libffi takes a different approach to allocate the closure on SELinux systems.
The reproducer demonstrates the following: First, we allocate closure, then we fork the process. The forked process inherits the shared FFI memory. Then that process ends, I think that Ruby frees what was present in that child process which corrupts the memory that is still present in the parent, then we start GC in the parent (to make sure GC runs, in bigger programs this would happen automatically in the background), which results in a SIGABRT with the backtrace noted in the first comment.
The failure we observe in the Ruby test suite follows a very similar pattern to the reproducer when it fails for us in the build. First, some Fiddle tests that allocate Closures run, then some other test forks, and then the GC needs to be triggered. Some tests trigger them manually (e.g. TestHash#test_replace_bug15358), and some are very likely to trigger the GC simply by their memory usage (e.g. TestTrick2015#test_kinaba).
We can see the problem is that this memory usage is not coordinated properly between parent and child process. On Fedora side we currently disable the closure tests (ruby.spec) to prevent this kind of failure.
The possible mitigations for this are:
To the last point, we also found that if the Fiddle::Closure is allocated in the child, it is not a problem for overall status of the test suite, therefore this approach could be used for tests using closure to ensure that the tests can run even with SELinux and whatnot. Additionally, with this approach, one more test would be created with the mentioned reproducer to test the integration of libffi on the current system.
Using the previous reproducer:
require 'fiddle/closure'
require 'fiddle/struct'
fork do
Fiddle::Closure.new(Fiddle::TYPE_VOID, [])
end
fork { }
GC.start
This does not crash the process, unlike the previous reproducer.
I noticed that Ruby tests include assert_separately
, which would help with this. I am unsure if it is usable here as it seems unique to the Ruby test suite.
@jackorp Thanks for your detailed description! Could you try 1343ac7a9567665affc0e9f4bc9fd74cfd97a5fd ?
Thx for the patch. I leave the testing to @jackorp thx :wink:
Nevertheless, this is fixing the specific use case in the test suite, while this is generic issue. Therefore I wonder if there is a way to e.g.:
I am not ffi expert, so apologies if I am completely off.
Thanks for the patch!
It improved the situation somewhat but not complemetely. After uncomentting and re-enabling the tests for Ruby and applying the patch I observe new issue in the build logs:
1) Failure:
Fiddle::TestClosure#test_conversion_unsigned_char [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:14]:
<0> expected but was
<1>.
2) Failure:
Fiddle::TestClosure#test_conversion_unsigned_long [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:14]:
<0> expected but was
<1>.
3) Failure:
Fiddle::TestClosure#test_const_string [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:14]:
<0> expected but was
<1>.
4) Failure:
Fiddle::TestClosure#test_conversion_long [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:14]:
<0> expected but was
<1>.
5) Failure:
Fiddle::TestClosure#test_conversion_char [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:14]:
<0> expected but was
<1>.
6) Failure:
Fiddle::TestClosure#test_conversion_unsigned_long_long [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:14]:
<0> expected but was
<1>.
7) Failure:
Fiddle::TestClosure#test_conversion_int [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:14]:
<0> expected but was
<1>.
8) Failure:
Fiddle::TestClosure#test_conversion_long_long [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:14]:
<0> expected but was
<1>.
9) Failure:
Fiddle::TestClosure#test_conversion_unsigned_int [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:14]:
<0> expected but was
<1>.
10) Failure:
Fiddle::TestClosure#test_conversion_short [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:14]:
<0> expected but was
<1>.
11) Failure:
Fiddle::TestClosure#test_block_caller [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:14]:
<0> expected but was
<1>.
12) Failure:
Fiddle::TestClosure#test_conversion_unsigned_short [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:14]:
<0> expected but was
<1>.
The Ruby test suite failed. Although without a segfault, that is an improvement.
Interestingly there seems to be some other issue with this, as I have another build log:
1) Failure:
Fiddle::TestFunc#test_qsort1 [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_func.rb:87]:
<0> expected but was
<1>.
Here, only a single test failed. Some builds do pass, but there are more of those that do not pass.
I think sometimes there is a (uncollectable maybe?) reference to Fiddle::Closure introduced in the tests. I'll try some local testing and poking around to find what that reference is.
Maybe asserting that there is either 0 or 1 reference in the ObjectSpace would be the way to go, but it requires lengthier testing on my side to confirm that this approach would not lead to a segfault due to the "random" nature of GC.
Thanks for confirming the patch.
It seems that we need a feature that frees a Fiddle::Closure
explicitly without GC. e.g.:
closure = Fiddle::Closure.new(...)
closure.free # Free the closure immediately.
Fiddle::Closure.allocate(...) do |closure|
# ...
end # Free the allocated closure when the block is exited.
But I don't have a good name for the feature for now... I consider the name in RubyKaigi 2022... (I give a talk today. I consider the name after my talk is finished.)
@jackorp Could you try https://github.com/ruby/fiddle/commit/a0ccc6bb1b989a32bd028ccc06445975c20e9446 ?
Hmm, not better. I also applied the previous commit with GC.start
to ensure application of the a0ccc6b
without modifying the patch too much. I am seeing similar failures as before.
However I have to note, in order to run the whole Ruby test suite together with the patches, I patch the fiddle shipped with Ruby 3.1.2. I may be missing a commit that makes all this works together.
If my patch-only approach fails, I'll try importing fiddle from the newest master branch commit as a gem and see if that improves the situation.
The work is happening on my downstream Fedora Ruby fork branch: https://src.fedoraproject.org/fork/jackorp/rpms/ruby/commits/fiddle_closure_test
Failed build: https://koji.fedoraproject.org/koji/taskinfo?taskID=91998265 relevant test failures, note failure number 14, which is new:
1) Failure:
Fiddle::TestFunc#test_qsort1 [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_func.rb:87]:
<0> expected but was
<1>.
2) Failure:
Fiddle::TestClosure#test_conversion_unsigned_long_long [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:14]:
<0> expected but was
<1>.
3) Failure:
Fiddle::TestClosure#test_const_string [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:14]:
<0> expected but was
<1>.
4) Failure:
Fiddle::TestClosure#test_conversion_unsigned_char [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:14]:
<0> expected but was
<1>.
5) Failure:
Fiddle::TestClosure#test_conversion_unsigned_long [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:14]:
<0> expected but was
<1>.
6) Failure:
Fiddle::TestClosure#test_conversion_long [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:14]:
<0> expected but was
<1>.
7) Failure:
Fiddle::TestClosure#test_conversion_char [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:14]:
<0> expected but was
<1>.
8) Failure:
Fiddle::TestClosure#test_conversion_unsigned_short [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:14]:
<0> expected but was
<1>.
9) Failure:
Fiddle::TestClosure#test_conversion_int [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:14]:
<0> expected but was
<1>.
10) Failure:
Fiddle::TestClosure#test_block_caller [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:14]:
<0> expected but was
<1>.
11) Failure:
Fiddle::TestClosure#test_conversion_short [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:14]:
<0> expected but was
<1>.
12) Failure:
Fiddle::TestClosure#test_conversion_unsigned_int [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:14]:
<0> expected but was
<1>.
13) Failure:
Fiddle::TestClosure#test_conversion_long_long [/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:14]:
<0> expected but was
<1>.
14) Error:
Fiddle::TestClosure#test_free:
ArgumentError: wrong number of arguments (given 0, expected 1+)
/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:83:in `block in test_free'
/builddir/build/BUILD/ruby-3.1.2/.ext/common/fiddle/closure.rb:20:in `create'
/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:82:in `test_free'
/builddir/build/BUILD/ruby-3.1.2/tool/test/runner.rb:23:in `<top (required)>'
./test/runner.rb:14:in `require_relative'
./test/runner.rb:14:in `<main>'
Sorry... I missed some tests to be switched to new API...
Could you also apply the following patches?
Sorry. One more commit: https://github.com/ruby/fiddle/commit/2530496602f2bef6ef080ad3f67ae71dd601843d
Thanks! The state is Much better now.
However this error from the logs is puzzling:
1) Error:
Fiddle::TestClosure#test_free:
NoMethodError: undefined method `assert_false' for #<Fiddle::TestClosure:0x00007f5f73a35fa0 @__name__=:test_free, @__io__=nil, @passed=false, @_assertions=0, @options={:job_status=>nil, :retry=>true, :hide_skip=>true, :repeat_count=>nil, :excludes=>["./test/excludes"], :ruby=>["./miniruby", "-I./lib", "-I.", "-I.ext/common", "./tool/runruby.rb", "--extout=.ext", "--", "--disable-gems"], :filter=>/\A(?=.*)(?!.*(?-mix:(?-mix:memory_leak)|(?-mix:TestAddressResolve#test_socket_getnameinfo_domain_blocking)))/, :verbose=>true, :seed=>10492}, @__gc_disabled__=false, @tracepoint_captured_singlethread=true, @tracepoint_captured_stat=[[#<RubyVM:0x00007f5f7deb3af0>, 0, 0]], @libc=#<Fiddle::Handle:0x00007f5f73a3a2d0>, @libm=#<Fiddle::Handle:0x00007f5f73a3a2a8>>
Did you mean? assert_file
assert_raise
assert_same
assert_raises
assert_all?
assert_false(closure.freed?)
^^^^^^^^^^^^
/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:86:in `block in test_free'
/builddir/build/BUILD/ruby-3.1.2/.ext/common/fiddle/closure.rb:20:in `create'
/builddir/build/BUILD/ruby-3.1.2/test/fiddle/test_closure.rb:85:in `test_free'
I don't understand how it does not know about assert_false... I assume Ruby only has the test-unit available in the tool/lib/test/unit during the tests phase. That one does not have assert_false
, however it has refute
, which seems to fulfill similar role and is also available in the upstream test-unit.
Alternatively, consider something like assert(closure.freed? == false)
.
This seems like the single error so far. I'll try out more builds in parallel later, just to be more sure.
For now I am using the following patch for e1221297
:
From e1221297fb0177d98c8670e5490c8131227621d5 Mon Sep 17 00:00:00 2001
From: Sutou Kouhei <kou@clear-code.com>
Date: Thu, 15 Sep 2022 06:40:31 +0900
Subject: [PATCH 2/6] test: don't use power-assert
It seems that we can't use it in ruby/ruby.
---
test/fiddle/test_closure.rb | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/test/fiddle/test_closure.rb b/test/fiddle/test_closure.rb
index 1726db7..13dfa5b 100644
--- a/test/fiddle/test_closure.rb
+++ b/test/fiddle/test_closure.rb
@@ -83,13 +83,9 @@ module Fiddle
def test_free
Closure.create(:int, [:void]) do |closure|
- assert do
- not closure.freed?
- end
+ assert_equal(closure.freed?, false)
closure.free
- assert do
- closure.freed?
- end
+ assert_equal(closure.freed?, true)
closure.free
end
end
--
2.37.3
Note I changed the assert_false
to assert_equal(closure.freed?, false)
.
So far 4 arch specific builds passed with the changes: https://koji.fedoraproject.org/koji/taskinfo?taskID=92035826 I'll run more builds to confirm the results, but this seems successful so far.
Ran 5 builds with 4 different arches each, only problem was unrelated failure on aarch64 on a few of those. Otherwise, nothing fiddle related!
Thanks!
Please use https://github.com/ruby/fiddle/commit/ced671e43bb72f3714cd6e2ee2d3f4ca92f05353 for assert_false
.
Hi, libffi-3.4.2 recently landed in Fedora and since that time, I observe strange failures in Ruby test suite:
Executing the test on itself, they passes just fine, but testing e.g.
test/ruby/test_autoload.rb -v -n '/TestAutoload#test_autoload_fork/'
together withtest/fiddle/test_import.rb
makes the reproducer smaller.Trying to reduce the issue even further, I have reduced the test/ruby/test_import.rb into the following shape:
And use just miniruby: