* segfault in ffi_data_to_code_pointer
@ 2019-06-26 20:55 DJ Delorie
2019-06-30 11:46 ` Anthony Green
2019-07-04 12:35 ` Florian Weimer
0 siblings, 2 replies; 9+ messages in thread
From: DJ Delorie @ 2019-06-26 20:55 UTC (permalink / raw)
To: libffi-discuss; +Cc: Florian Weimer
In src/closures.c, ffi_data_to_code_pointer() calls segment_holding()
to get a pointer to the code segment for a data segment. It doesn't
check for a NULL return, and I've got a test case where I run Ruby's
test suite (on a non-selinux aarch64 machine, if that matters) and
segment_holding() returns NULL and much hilarity ensues.
The following patch fixes the segfault, but I don't know if
segment_holding() returning NULL is an expected case, or a symptom of
problems elsewhere?
> diff -rup a/src/closures.c b/src/closures.c
> --- a/src/closures.c 2019-06-25 21:21:06.738743440 -0400
> +++ b/src/closures.c 2019-06-25 21:22:00.769716129 -0400
> @@ -621,7 +621,10 @@ void *
> ffi_data_to_code_pointer (void *data)
> {
> msegmentptr seg = segment_holding (gm, data);
> - return add_segment_exec_offset (data, seg);
> + if (seg)
> + return add_segment_exec_offset (data, seg);
> + else
> + return data;
> }
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: segfault in ffi_data_to_code_pointer 2019-06-26 20:55 segfault in ffi_data_to_code_pointer DJ Delorie @ 2019-06-30 11:46 ` Anthony Green 2019-07-02 23:47 ` DJ Delorie 2019-07-04 12:35 ` Florian Weimer 1 sibling, 1 reply; 9+ messages in thread From: Anthony Green @ 2019-06-30 11:46 UTC (permalink / raw) To: DJ Delorie; +Cc: libffi-discuss, Florian Weimer Hi DJ, DJ Delorie <dj@redhat.com> writes: > In src/closures.c, ffi_data_to_code_pointer() calls segment_holding() > to get a pointer to the code segment for a data segment. It doesn't > check for a NULL return, and I've got a test case where I run Ruby's > test suite (on a non-selinux aarch64 machine, if that matters) and > segment_holding() returns NULL and much hilarity ensues. This suggests that the ffi_closure object used to invoke ffi_data_to_code_pointer wasn't allocated by ffi_closure_alloc(). Is that something you can check? Thanks, AG > > The following patch fixes the segfault, but I don't know if > segment_holding() returning NULL is an expected case, or a symptom of > problems elsewhere? > >> diff -rup a/src/closures.c b/src/closures.c >> --- a/src/closures.c 2019-06-25 21:21:06.738743440 -0400 >> +++ b/src/closures.c 2019-06-25 21:22:00.769716129 -0400 >> @@ -621,7 +621,10 @@ void * >> ffi_data_to_code_pointer (void *data) >> { >> msegmentptr seg = segment_holding (gm, data); >> - return add_segment_exec_offset (data, seg); >> + if (seg) >> + return add_segment_exec_offset (data, seg); >> + else >> + return data; >> } -- Anthony Green <green@redhat.com> Senior Principal Solutions Architect, Financial Services +1 647 477-3809 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: segfault in ffi_data_to_code_pointer 2019-06-30 11:46 ` Anthony Green @ 2019-07-02 23:47 ` DJ Delorie 2019-07-03 22:28 ` DJ Delorie 0 siblings, 1 reply; 9+ messages in thread From: DJ Delorie @ 2019-07-02 23:47 UTC (permalink / raw) To: Anthony Green; +Cc: libffi-discuss, fweimer > DJ Delorie <dj@redhat.com> writes: >> In src/closures.c, ffi_data_to_code_pointer() calls segment_holding() >> to get a pointer to the code segment for a data segment. It doesn't >> check for a NULL return, and I've got a test case where I run Ruby's >> test suite (on a non-selinux aarch64 machine, if that matters) and >> segment_holding() returns NULL and much hilarity ensues. Anthony Green <green@redhat.com> writes: > This suggests that the ffi_closure object used to invoke > ffi_data_to_code_pointer wasn't allocated by ffi_closure_alloc(). Is > that something you can check? It seems to be so: [root ruby-2.6.3]# ./miniruby ./tool/runruby.rb -Itest/lib -r 'test/unit' "./test/fiddle/test_import.rb" < 0000ffffaa9b0000 < 0000ffffaa900000 Run options: The '<' lines indicate calls to ffi_data_to_code_pointer that had a NULL segment, and the non-existing '>' lines indicated non-existing calls to ffi_closure_allocate. However, a simple NULL check may be the right fix anyway: * it fixes the ruby build/test problem * it does not re-introduce the illegal instruction bug Which means that the non-code-seg'd closure isn't the one that triggers the illegal instruction... but I don't know why still ;-) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: segfault in ffi_data_to_code_pointer 2019-07-02 23:47 ` DJ Delorie @ 2019-07-03 22:28 ` DJ Delorie 2019-07-03 22:47 ` Anthony Green 0 siblings, 1 reply; 9+ messages in thread From: DJ Delorie @ 2019-07-03 22:28 UTC (permalink / raw) To: green, libffi-discuss, fweimer Further debugging has disclosed that the ruby code calls ffi_prep_closure() passing a pointer returned from mmap() - not one from ffi_closure_alloc(). Is that allowed? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: segfault in ffi_data_to_code_pointer 2019-07-03 22:28 ` DJ Delorie @ 2019-07-03 22:47 ` Anthony Green 2019-07-03 22:54 ` DJ Delorie 0 siblings, 1 reply; 9+ messages in thread From: Anthony Green @ 2019-07-03 22:47 UTC (permalink / raw) To: DJ Delorie; +Cc: green, libffi-discuss, fweimer DJ Delorie <dj@redhat.com> writes: > Further debugging has disclosed that the ruby code calls > ffi_prep_closure() passing a pointer returned from mmap() - not one from > ffi_closure_alloc(). > > Is that allowed? No, they should be using ffi_closure_alloc(). Can you point me at the ruby code that does this? Thanks, AG ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: segfault in ffi_data_to_code_pointer 2019-07-03 22:47 ` Anthony Green @ 2019-07-03 22:54 ` DJ Delorie 2019-07-03 23:14 ` Anthony Green 0 siblings, 1 reply; 9+ messages in thread From: DJ Delorie @ 2019-07-03 22:54 UTC (permalink / raw) To: Anthony Green; +Cc: libffi-discuss, fweimer Anthony Green <green@redhat.com> writes: > No, they should be using ffi_closure_alloc(). > > Can you point me at the ruby code that does this? ruby 2.6.3 ext/fiddle/closure.c In the failing case, USE_FFI_CLOSURE_ALLOC is not set static VALUE allocate(VALUE klass) { fiddle_closure * closure; VALUE i = TypedData_Make_Struct(klass, fiddle_closure, &closure_data_type, closure); fprintf (stderr, "DJ: allocate\n"); #if USE_FFI_CLOSURE_ALLOC closure->pcl = ffi_closure_alloc(sizeof(ffi_closure), &closure->code); #else closure->pcl = mmap(NULL, sizeof(ffi_closure), PROT_READ | PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0); #endif return i; } initialize() { . . . #if USE_FFI_CLOSURE_ALLOC result = ffi_prep_closure_loc(pcl, cif, callback, (void *)self, cl->code); #else result = ffi_prep_closure(pcl, cif, callback, (void *)self); cl->code = (void *)pcl; i = mprotect(pcl, sizeof(*pcl), PROT_READ | PROT_EXEC); if (i) { rb_sys_fail("mprotect"); } #endif . . . } ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: segfault in ffi_data_to_code_pointer 2019-07-03 22:54 ` DJ Delorie @ 2019-07-03 23:14 ` Anthony Green 2019-07-04 0:19 ` DJ Delorie 0 siblings, 1 reply; 9+ messages in thread From: Anthony Green @ 2019-07-03 23:14 UTC (permalink / raw) To: DJ Delorie; +Cc: Anthony Green, libffi-discuss, fweimer DJ Delorie <dj@redhat.com> writes: > Anthony Green <green@redhat.com> writes: >> No, they should be using ffi_closure_alloc(). >> >> Can you point me at the ruby code that does this? > > ruby 2.6.3 > ext/fiddle/closure.c Thanks. This looks right to me. Perhaps they wrote this before ffi_closure_alloc() existed? I don't remember when every bit was introduced... Looks like you need to follow up with the ruby people. Thanks DJ, AG > In the failing case, USE_FFI_CLOSURE_ALLOC is not set > > static VALUE > allocate(VALUE klass) > { > fiddle_closure * closure; > > VALUE i = TypedData_Make_Struct(klass, fiddle_closure, > &closure_data_type, closure); > > fprintf (stderr, "DJ: allocate\n"); > #if USE_FFI_CLOSURE_ALLOC > closure->pcl = ffi_closure_alloc(sizeof(ffi_closure), &closure->code); > #else > closure->pcl = mmap(NULL, sizeof(ffi_closure), PROT_READ | PROT_WRITE, > MAP_ANON | MAP_PRIVATE, -1, 0); > #endif > > return i; > } > > initialize() > { > . . . > #if USE_FFI_CLOSURE_ALLOC > result = ffi_prep_closure_loc(pcl, cif, callback, > (void *)self, cl->code); > #else > result = ffi_prep_closure(pcl, cif, callback, (void *)self); > cl->code = (void *)pcl; > i = mprotect(pcl, sizeof(*pcl), PROT_READ | PROT_EXEC); > if (i) { > rb_sys_fail("mprotect"); > } > #endif > . . . > } ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: segfault in ffi_data_to_code_pointer 2019-07-03 23:14 ` Anthony Green @ 2019-07-04 0:19 ` DJ Delorie 0 siblings, 0 replies; 9+ messages in thread From: DJ Delorie @ 2019-07-04 0:19 UTC (permalink / raw) To: Anthony Green; +Cc: libffi-discuss, fweimer Anthony Green <green@redhat.com> writes: > Thanks. This looks right to me. By "right" do you mean "wrong" ? > Perhaps they wrote this before ffi_closure_alloc() existed? I don't > remember when every bit was introduced... Looks like you need to > follow up with the ruby people. So... no need/desire/reason to change anything in libffi then? ;-) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: segfault in ffi_data_to_code_pointer 2019-06-26 20:55 segfault in ffi_data_to_code_pointer DJ Delorie 2019-06-30 11:46 ` Anthony Green @ 2019-07-04 12:35 ` Florian Weimer 1 sibling, 0 replies; 9+ messages in thread From: Florian Weimer @ 2019-07-04 12:35 UTC (permalink / raw) To: DJ Delorie; +Cc: libffi-discuss * DJ Delorie: > In src/closures.c, ffi_data_to_code_pointer() calls segment_holding() > to get a pointer to the code segment for a data segment. It doesn't > check for a NULL return, and I've got a test case where I run Ruby's > test suite (on a non-selinux aarch64 machine, if that matters) and > segment_holding() returns NULL and much hilarity ensues. > > The following patch fixes the segfault, but I don't know if > segment_holding() returning NULL is an expected case, or a symptom of > problems elsewhere? > >> diff -rup a/src/closures.c b/src/closures.c >> --- a/src/closures.c 2019-06-25 21:21:06.738743440 -0400 >> +++ b/src/closures.c 2019-06-25 21:22:00.769716129 -0400 >> @@ -621,7 +621,10 @@ void * >> ffi_data_to_code_pointer (void *data) >> { >> msegmentptr seg = segment_holding (gm, data); >> - return add_segment_exec_offset (data, seg); >> + if (seg) >> + return add_segment_exec_offset (data, seg); >> + else >> + return data; >> } I think you also need to fix the aarch64 code to avoid a null pointer dereference, like below. Also submitted as <https://github.com/libffi/libffi/pull/499>. I have verified that this fixes the Ruby build failure. Thanks, Florian diff --git a/src/aarch64/ffi.c b/src/aarch64/ffi.c index 6f6aac4..69e04d6 100644 --- a/src/aarch64/ffi.c +++ b/src/aarch64/ffi.c @@ -777,7 +777,8 @@ ffi_prep_closure_loc (ffi_closure *closure, /* Also flush the cache for code mapping. */ unsigned char *tramp_code = ffi_data_to_code_pointer (tramp); - ffi_clear_cache (tramp_code, tramp_code + FFI_TRAMPOLINE_SIZE); + if (tramp_code != NULL) + ffi_clear_cache (tramp_code, tramp_code + FFI_TRAMPOLINE_SIZE); #endif closure->cif = cif; diff --git a/src/closures.c b/src/closures.c index 4d7f945..18d3913 100644 --- a/src/closures.c +++ b/src/closures.c @@ -925,6 +925,8 @@ void * ffi_data_to_code_pointer (void *data) { msegmentptr seg = segment_holding (gm, data); + if (seg == NULL) + return NULL; return add_segment_exec_offset (data, seg); } ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-07-04 12:35 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-26 20:55 segfault in ffi_data_to_code_pointer DJ Delorie 2019-06-30 11:46 ` Anthony Green 2019-07-02 23:47 ` DJ Delorie 2019-07-03 22:28 ` DJ Delorie 2019-07-03 22:47 ` Anthony Green 2019-07-03 22:54 ` DJ Delorie 2019-07-03 23:14 ` Anthony Green 2019-07-04 0:19 ` DJ Delorie 2019-07-04 12:35 ` Florian Weimer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).