Skip to content

Commit

Permalink
Fix a use-after-free and a shutdown crash (rubyjs#162)
Browse files Browse the repository at this point in the history 
* Fix use-after-free

This can easily glanced on valgrind:

$ cat a.rb
require 'mini_racer'

puts 'before context new'
context = MiniRacer::Context.new
puts 'after context new'
context.dispose
puts 'after dispose'

glopes ~/repos/mini_racer ((b32ca19...) %) $ valgrind --max-stackframe=8379104 /home/glopes/ruby/bin/ruby -Ilib:test:lib a.rb
==29861== Memcheck, a memory error detector
==29861== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==29861== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==29861== Command: /home/glopes/ruby/bin/ruby -Ilib:test:lib a.rb
==29861==
before context new
after context new
==29861== Invalid read of size 8
==29861==    at 0xC301450: free_context(ContextInfo*) (mini_racer_extension.cc:1247)
==29861==    by 0xC301516: rb_context_dispose(unsigned long) (mini_racer_extension.cc:1439)
==29861==    by 0x2F5B39: vm_call_cfunc_with_frame (vm_insnhelper.c:2514)
==29861==    by 0x2F5B39: vm_call_cfunc (vm_insnhelper.c:2539)
==29861==    by 0x30FF4D: vm_call_method (vm_insnhelper.c:3053)
==29861==    by 0x307CD4: vm_sendish (vm_insnhelper.c:4023)
==29861==    by 0x307CD4: vm_exec_core (insns.def:801)
==29861==    by 0x2FF7E7: rb_vm_exec (vm.c:1920)
==29861==    by 0x3004DD: invoke_iseq_block_from_c (vm.c:1116)
==29861==    by 0x3004DD: invoke_block_from_c_bh (vm.c:1134)
==29861==    by 0x3004DD: vm_yield (vm.c:1179)
==29861==    by 0x3004DD: rb_yield_0 (vm_eval.c:1227)
==29861==    by 0x3004DD: rb_yield_1 (vm_eval.c:1233)
==29861==    by 0x3004DD: rb_yield (vm_eval.c:1243)
==29861==    by 0x136F60: rb_ensure (eval.c:1129)
==29861==    by 0x2F5B39: vm_call_cfunc_with_frame (vm_insnhelper.c:2514)
==29861==    by 0x2F5B39: vm_call_cfunc (vm_insnhelper.c:2539)
==29861==    by 0x30FF4D: vm_call_method (vm_insnhelper.c:3053)
==29861==    by 0x307D8A: vm_sendish (vm_insnhelper.c:4023)
==29861==    by 0x307D8A: vm_exec_core (insns.def:782)
==29861==    by 0x2FFE8E: rb_vm_exec (vm.c:1929)
==29861==  Address 0xbd6e520 is 0 bytes inside a block of size 48 free'd
==29861==    at 0x4C30D3B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==29861==    by 0x157D09: objspace_xfree (gc.c:10071)
==29861==    by 0x157D09: ruby_sized_xfree (gc.c:10164)
==29861==    by 0x157D09: ruby_xfree (gc.c:10171)
==29861==    by 0xC3013CF: operator delete (mini_racer_extension.cc:84)
==29861==    by 0xC3013CF: release (mini_racer_extension.cc:71)
==29861==    by 0xC3013CF: free_context_raw(void*) (mini_racer_extension.cc:1221)
==29861==    by 0xC301448: free_context(ContextInfo*) (mini_racer_extension.cc:1244)
==29861==    by 0xC301516: rb_context_dispose(unsigned long) (mini_racer_extension.cc:1439)
==29861==    by 0x2F5B39: vm_call_cfunc_with_frame (vm_insnhelper.c:2514)
==29861==    by 0x2F5B39: vm_call_cfunc (vm_insnhelper.c:2539)
==29861==    by 0x30FF4D: vm_call_method (vm_insnhelper.c:3053)
==29861==    by 0x307CD4: vm_sendish (vm_insnhelper.c:4023)
==29861==    by 0x307CD4: vm_exec_core (insns.def:801)
==29861==    by 0x2FF7E7: rb_vm_exec (vm.c:1920)
==29861==    by 0x3004DD: invoke_iseq_block_from_c (vm.c:1116)
==29861==    by 0x3004DD: invoke_block_from_c_bh (vm.c:1134)
==29861==    by 0x3004DD: vm_yield (vm.c:1179)
==29861==    by 0x3004DD: rb_yield_0 (vm_eval.c:1227)
==29861==    by 0x3004DD: rb_yield_1 (vm_eval.c:1233)
==29861==    by 0x3004DD: rb_yield (vm_eval.c:1243)
==29861==    by 0x136F60: rb_ensure (eval.c:1129)
==29861==    by 0x2F5B39: vm_call_cfunc_with_frame (vm_insnhelper.c:2514)
==29861==    by 0x2F5B39: vm_call_cfunc (vm_insnhelper.c:2539)
==29861==  Block was alloc'd at
==29861==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==29861==    by 0x154BE3: objspace_xmalloc0 (gc.c:9860)
==29861==    by 0xC3041D1: operator new (mini_racer_extension.cc:80)
==29861==    by 0xC3041D1: rb_context_init_unsafe(unsigned long, unsigned long, unsigned long) (mini_racer_extension.cc:777)
==29861==    by 0x2F5B39: vm_call_cfunc_with_frame (vm_insnhelper.c:2514)
==29861==    by 0x2F5B39: vm_call_cfunc (vm_insnhelper.c:2539)
==29861==    by 0x30FF4D: vm_call_method (vm_insnhelper.c:3053)
==29861==    by 0x307CD4: vm_sendish (vm_insnhelper.c:4023)
==29861==    by 0x307CD4: vm_exec_core (insns.def:801)
==29861==    by 0x2FF7E7: rb_vm_exec (vm.c:1920)
==29861==    by 0x3037B6: vm_call0_body (vm_eval.c:136)
==29861==    by 0x303F7E: rb_vm_call0 (vm_eval.c:52)
==29861==    by 0x3042CC: rb_vm_call_kw (vm_eval.c:268)
==29861==    by 0x3048CF: rb_call0 (vm_eval.c:392)
==29861==    by 0x304D73: rb_call (vm_eval.c:718)
==29861==    by 0x304D73: rb_funcallv_kw (vm_eval.c:965)
==29861==
after dispose

* Fix shutdown crashes, make cleanup threads detached

Can be reproduced just by running the tests:

while /home/glopes/ruby/bin/ruby -Ilib:test:lib -I/home/glopes/ruby/lib/ruby/gems/2.7.0/gems/rake-10.5.0/lib /home/glopes/ruby/lib/ruby/gems/2.7.0/gems/rake-10.5.0/lib/rake/rake_test_loader.rb test/mini_racer_test.rb; do echo not yet; done

...

You have skipped tests. Run with --verbose for details.
Segmentation fault (core dumped)
glopes ~/repos/mini_racer ((b32ca19...) %) $ coredumpctl gdb
           PID: 29454 (ruby)
           UID: 1000 (glopes)
           GID: 1000 (glopes)
        Signal: 11 (SEGV)
     Timestamp: Tue 2020-05-05 15:24:40 WEST (11s ago)
  Command Line: /home/glopes/ruby/bin/ruby -Ilib:test:lib -I/home/glopes/ruby/lib/ruby/gems/2.7.0/gems/rake-10.5.0/lib /home/glopes/ruby/lib/ruby/gems/2.7.0/gems/rake-10.5.0/lib/rake/rake_test_loader.rb test/mini_racer_test.rb
    Executable: /home/glopes/ruby/bin/ruby
 Control Group: /user.slice/user-1000.slice/session-c2.scope
          Unit: session-c2.scope
         Slice: user-1000.slice
       Session: c2
     Owner UID: 1000 (glopes)
       Boot ID: 64a5c179f6764f5f95498c75fd17be8e
    Machine ID: 82e2fca72c474b2591af0e380dc4cead
      Hostname: aorus
       Storage: /var/lib/systemd/coredump/core.ruby.1000.64a5c179f6764f5f95498c75fd17be8e.29454.1588688680000000.lz4
       Message: Process 29454 (ruby) of user 1000 dumped core.

                Stack trace of thread 29491:
                #0  0x0000563082fe9e60 n/a (n/a)
                #1  0x00007fc11080ae2c n/a (/home/glopes/repos/mini_racer/lib/mini_racer_extension.so)
                #2  0x00007fc110794b03 n/a (/home/glopes/repos/mini_racer/lib/mini_racer_extension.so)
                #3  0x00007fc11082829e n/a (/home/glopes/repos/mini_racer/lib/mini_racer_extension.so)
                #4  0x00007fc110827f76 n/a (/home/glopes/repos/mini_racer/lib/mini_racer_extension.so)
                #5  0x00007fc1105e3560 n/a (/home/glopes/repos/mini_racer/lib/mini_racer_extension.so)

GNU gdb (Ubuntu 8.1-0ubuntu3.2) 8.1.0.20180409-git
Copyright (C) 2018 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
<http://www.gnu.org/software/gdb/documentation/>.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from /home/glopes/ruby/bin/ruby...done.
[New LWP 29491]
[New LWP 29455]
[New LWP 29456]
[New LWP 29454]
[New LWP 29468]
[New LWP 29469]
[New LWP 29465]
[New LWP 29467]
[New LWP 29471]
[New LWP 29470]
[New LWP 29472]
[New LWP 29474]
[New LWP 29473]
[New LWP 29475]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Core was generated by `/home/glopes/ruby/bin/ruby -Ilib:test:lib -I/home/glopes/ruby/lib/ruby/gems/2.7'.
Program terminated with signal SIGSEGV, Segmentation fault.
[Current thread is 1 (Thread 0x7fc0b0ff9700 (LWP 29491))]
 #0  0x0000563082fe9e60 in ?? ()
 #1  0x00007fc110b9ea6c in v8::internal::VirtualMemory::Free() () from /home/glopes/repos/mini_racer/lib/mini_racer_extension.so
 #2  0x00007fc11080ae2c in v8::internal::StoreBuffer::TearDown() () from /home/glopes/repos/mini_racer/lib/mini_racer_extension.so
 #3  0x00007fc110794b03 in v8::internal::Heap::TearDown() () from /home/glopes/repos/mini_racer/lib/mini_racer_extension.so
 #4  0x00007fc11082829e in v8::internal::Isolate::Deinit() () from /home/glopes/repos/mini_racer/lib/mini_racer_extension.so
 #5  0x00007fc110827f76 in v8::internal::Isolate::Delete(v8::internal::Isolate*) () from /home/glopes/repos/mini_racer/lib/mini_racer_extension.so
 #6  0x00007fc1105e3560 in free_isolate (isolate_info=isolate_info@entry=0x563083e7db70) at ../../../../ext/mini_racer_extension/mini_racer_extension.cc:1194
 #7  0x00007fc1105e03c8 in IsolateInfo::~IsolateInfo (this=0x563083e7db70, __in_chrg=<optimized out>)
    at ../../../../ext/mini_racer_extension/mini_racer_extension.cc:52
 #8  IsolateInfo::release (this=0x563083e7db70) at ../../../../ext/mini_racer_extension/mini_racer_extension.cc:71
 #9  free_context_raw (arg=0x5630832a7d90) at ../../../../ext/mini_racer_extension/mini_racer_extension.cc:1221
 #10 0x00007fc116c5f6db in start_thread (arg=0x7fc0b0ff9700) at pthread_create.c:463
 #11 0x00007fc115d2588f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
  • Loading branch information
@cataphract
cataphract authored May 8, 2020
1 parent b32ca19 commit 2b1ee80
Showing 1 changed file with 45 additions and 13 deletions.
58 changes: 45 additions & 13 deletions ext/mini_racer_extension/mini_racer_extension.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,10 @@ static VALUE rb_cDateTime = Qnil;
static std::unique_ptr<Platform> current_platform = NULL;
static std::mutex platform_lock;

static pthread_attr_t *thread_attr_p;
static pthread_rwlock_t exit_lock = PTHREAD_RWLOCK_INITIALIZER;
static bool ruby_exiting; // guarded by exit_lock

static VALUE rb_platform_set_flag_as_str(VALUE _klass, VALUE flag_as_str) {
bool platform_already_initialized = false;

Expand Down Expand Up @@ -1205,7 +1209,7 @@ void free_isolate(IsolateInfo* isolate_info) {
delete isolate_info->allocator;
}

static void *free_context_raw(void* arg) {
static void free_context_raw(void *arg) {
ContextInfo* context_info = (ContextInfo*)arg;
IsolateInfo* isolate_info = context_info->isolate_info;
Persistent<Context>* context = context_info->context;
Expand All @@ -1222,6 +1226,20 @@ static void *free_context_raw(void* arg) {
}

xfree(context_info);
}

static void *free_context_thr(void* arg) {
if (pthread_rwlock_tryrdlock(&exit_lock) != 0) {
return NULL;
}
if (ruby_exiting) {
return NULL;
}

free_context_raw(arg);

pthread_rwlock_unlock(&exit_lock);

return NULL;
}

Expand All @@ -1235,22 +1253,17 @@ static void free_context(ContextInfo* context_info) {
context_info_copy->context = context_info->context;

if (isolate_info && isolate_info->refs() > 1) {
pthread_t free_context_thread;
if (pthread_create(&free_context_thread, NULL, free_context_raw, (void*)context_info_copy)) {
fprintf(stderr, "WARNING failed to release memory in MiniRacer, thread to release could not be created, process will leak memory\n");
}

pthread_t free_context_thread;
if (pthread_create(&free_context_thread, thread_attr_p,
free_context_thr, (void*)context_info_copy)) {
fprintf(stderr, "WARNING failed to release memory in MiniRacer, thread to release could not be created, process will leak memory\n");
}
} else {
free_context_raw(context_info_copy);
}

if (context_info->context && isolate_info && isolate_info->isolate) {
context_info->context = NULL;
}

if (isolate_info) {
context_info->isolate_info = NULL;
}
context_info->context = NULL;
context_info->isolate_info = NULL;
}

static void deallocate_isolate(void* data) {
Expand Down Expand Up @@ -1582,6 +1595,16 @@ static VALUE rb_context_create_isolate_value(VALUE self) {
return Data_Wrap_Struct(rb_cIsolate, NULL, &deallocate_isolate, isolate_info);
}

static void set_ruby_exiting(VALUE value) {
(void)value;

int res = pthread_rwlock_wrlock(&exit_lock);
ruby_exiting = true;
if (res == 0) {
pthread_rwlock_unlock(&exit_lock);
}
}

extern "C" {

void Init_mini_racer_extension ( void )
Expand Down Expand Up @@ -1635,5 +1658,14 @@ extern "C" {
rb_define_private_method(rb_cIsolate, "init_with_snapshot",(VALUE(*)(...))&rb_isolate_init_with_snapshot, 1);

rb_define_singleton_method(rb_cPlatform, "set_flag_as_str!", (VALUE(*)(...))&rb_platform_set_flag_as_str, 1);

rb_set_end_proc(set_ruby_exiting, Qnil);

static pthread_attr_t attr;
if (pthread_attr_init(&attr) == 0) {
if (pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED) == 0) {
thread_attr_p = &attr;
}
}
}
}

0 comments on commit 2b1ee80

Please sign in to comment.