Bug 1727832

(In reply to Florian Weimer from comment #0)
> Building with -DUSE_FFI_CLOSURE_ALLOC=1 should allow this code to use the
> internal libffi workarounds for such configurations.

What I don't understand is why we should diverge from upstream? Why upstream should not always use ffi_closure_free over mmap when it is preferred on SELinux restricted machines?

(In reply to Vít Ondruch from comment #1)
> (In reply to Florian Weimer from comment #0)
> > Building with -DUSE_FFI_CLOSURE_ALLOC=1 should allow this code to use the
> > internal libffi workarounds for such configurations.
> 
> What I don't understand is why we should diverge from upstream? Why upstream
> should not always use ffi_closure_free over mmap when it is preferred on
> SELinux restricted machines?

I don't know if upstream intends to keep support for previous libffi releases.  I cannot read Japanese: https://bugs.ruby-lang.org/issues/3371#note-4

There is check for libffi version:

https://github.com/ruby/ruby/commit/a0f7f2921518443a60c807d272f0345aed61dc21#diff-e73ea8b979a6ac11e388775b2e0f5e42R16

So does it depend on libffi version? I.e. the libffi < 3.0.5 does not support ffi_closure_free, but once it is available in libffi 3.0.5+, it should be used everywhere?

I can read Japanese.

> https://bugs.ruby-lang.org/issues/3371#note-4
> http://d.hatena.ne.jp/moriyoshi/20080322/1206220188

The mentioned Japanese blog said
When running libffi's testing code, it did not work due to SEGV. So, the blog's person fixed it.

> https://bugs.ruby-lang.org/issues/3371#note-6
> r28300 の変更で Abort はしなくなりましたが、ext/fiddle/closure.c:dealloc だけではなく ext/fiddle/closure.c:allocate も変更する必要があるのではないでしょうか?

And following above comment means
Abort was gone by r28300, do we need to change (fix) not only ext/fiddle/closure.c:dealloc but also ext/fiddle/closure.c:allocate ?

(In reply to Vít Ondruch from comment #3)
> There is check for libffi version:
> 
> https://github.com/ruby/ruby/commit/
> a0f7f2921518443a60c807d272f0345aed61dc21#diff-
> e73ea8b979a6ac11e388775b2e0f5e42R16
> 
> So does it depend on libffi version? I.e. the libffi < 3.0.5 does not
> support ffi_closure_free, but once it is available in libffi 3.0.5+, it
> should be used everywhere?

Our code has this:

#if defined(USE_FFI_CLOSURE_ALLOC)
#elif defined(__OpenBSD__) || defined(__APPLE__) || defined(__linux__)
# define USE_FFI_CLOSURE_ALLOC 0

So there has been a more recent change that disables the libffi allocator on Linux.

(In reply to Florian Weimer from comment #5)
> Our code has this:
> 
> #if defined(USE_FFI_CLOSURE_ALLOC)
> #elif defined(__OpenBSD__) || defined(__APPLE__) || defined(__linux__)
> # define USE_FFI_CLOSURE_ALLOC 0
> 
> So there has been a more recent change that disables the libffi allocator on
> Linux.

Indeed, 10 hours more recent change:

https://github.com/ruby/ruby/commit/cddd37aae2c23ce30ddaa4580d065e6d5c8c6406

referring to the same ticket. But it has been disabled almost since introduction of Fiddle into Ruby:

https://github.com/ruby/ruby/commit/7eec027f0600ea84b9205f1bf35afd54592ca3f8

with related discussion again partially in Japanese:

http://blade.nagaokaut.ac.jp/cgi-bin/vframe.rb/ruby/ruby-dev/41270?40902-41501+split-mode-vertical

Anyway, I believe that the logic determining usage of USE_FFI_CLOSURE_ALLOC is just a big hammer, which just hides some problems instead using proper solution and not reflection recent development. @Florian, from libffi POV, is there any reason why ffi_closure_free should not be used on some platform?

BTW there was apparently also lack of Ruby tooling to allow detection of libffi version via package config:

https://github.com/ruby/ruby/commit/c607493e48c9f3dbf957a4bb5f0be2e8aa288442

(In reply to Vít Ondruch from comment #6)
> Anyway, I believe that the logic determining usage of USE_FFI_CLOSURE_ALLOC
> is just a big hammer, which just hides some problems instead using proper
> solution and not reflection recent development. @Florian, from libffi POV,
> is there any reason why ffi_closure_free should not be used on some platform?

I'm not a libffi expert, sorry.  I expect that ffi_closure_free works, at least on all the architectures we care about.  The libffi bug mentioned in the Ruby bug tracker appears to have been fixed a long time ago.  No other project that uses libffi still has a similar workaround (instead they encounter bugs in the libffi SELinux support …).

Let's see what upstream thinks about changing the defaults and using ffi_closure_free.

Vit is sending the pull-request, using the logic of USE_FFI_CLOSURE_ALLOC 1 and deleting below logic that has a problem.
It seems it will be merged.

https://github.com/ruby/fiddle/pull/20

> stream-ruby-2.6-rhel-8.1.0 currently has this code in ext/fiddle/closure.c:
> 
>     closure->pcl = mmap(NULL, sizeof(ffi_closure), PROT_READ | PROT_WRITE,
>         MAP_ANON | MAP_PRIVATE, -1, 0);
> …
>     i = mprotect(pcl, sizeof(*pcl), PROT_READ | PROT_EXEC);

Hi Jan,
Please add qa_ack+ ?

The way to verify this issue is only that we can build ruby on aarch64.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHEA-2019:3447