From 0268b34b2a0f4a76d0af508cb8fcce4fd41543bf Mon Sep 17 00:00:00 2001 From: Shizuo Fujita Date: Fri, 3 Jul 2026 00:11:01 +0900 Subject: [PATCH 1/2] Fix dangling path pointer in StatWatcher under GC compaction ev_stat_init() retains the path pointer it is given for the entire lifetime of the watcher; libev dereferences it on every stat (inotify rescans, lstat, statfs, ...). StatWatcher passed RSTRING_PTR(@path), which points into a Ruby String whose buffer GC.compact may relocate, leaving libev with a dangling pointer. Keep an owned copy of the path in the watcher struct instead, and free it in a new Coolio_Watcher_free() (replacing RUBY_DEFAULT_FREE). The copy is stable memory that the GC never moves, so libev's retained pointer stays valid. Co-Authored-By: Claude Opus 4.8 --- ext/cool.io/cool.io.h | 6 ++++++ ext/cool.io/stat_watcher.c | 15 +++++++++++++-- ext/cool.io/watcher.c | 14 +++++++++++++- spec/stat_watcher_spec.rb | 28 +++++++++++++++++++++++++++- 4 files changed, 59 insertions(+), 4 deletions(-) diff --git a/ext/cool.io/cool.io.h b/ext/cool.io/cool.io.h index 4510858..10bf950 100644 --- a/ext/cool.io/cool.io.h +++ b/ext/cool.io/cool.io.h @@ -55,6 +55,12 @@ struct Coolio_Watcher int enabled; VALUE loop; + /* Stable copy of the watched path for ev_stat watchers. libev retains the + * path pointer passed to ev_stat_init() for the lifetime of the watcher, so + * it must not point into a Ruby String whose buffer GC.compact may relocate. + * NULL for watcher types that don't use it. */ + char *stat_path; + void (*dispatch_callback)(VALUE self, int revents); }; diff --git a/ext/cool.io/stat_watcher.c b/ext/cool.io/stat_watcher.c index 12aa088..f7054a5 100644 --- a/ext/cool.io/stat_watcher.c +++ b/ext/cool.io/stat_watcher.c @@ -79,7 +79,8 @@ void Init_coolio_stat_watcher() */ static VALUE Coolio_StatWatcher_initialize(int argc, VALUE *argv, VALUE self) { - VALUE path, interval; + VALUE path, interval; + long path_len; struct Coolio_Watcher *watcher_data; rb_scan_args(argc, argv, "11", &path, &interval); @@ -91,11 +92,21 @@ static VALUE Coolio_StatWatcher_initialize(int argc, VALUE *argv, VALUE self) watcher_data = Coolio_Watcher_ptr(self); + /* libev keeps the path pointer passed to ev_stat_init() for the lifetime of + * the watcher. RSTRING_PTR(path) points into a Ruby String whose buffer + * GC.compact may relocate, leaving libev with a dangling pointer. Keep an + * owned copy instead; it is released in Coolio_Watcher_free(). */ + path_len = RSTRING_LEN(path); + if(watcher_data->stat_path) + xfree(watcher_data->stat_path); + watcher_data->stat_path = xmalloc(path_len + 1); + strlcpy(watcher_data->stat_path, RSTRING_PTR(path), path_len + 1); + watcher_data->dispatch_callback = Coolio_StatWatcher_dispatch_callback; ev_stat_init( &watcher_data->event_types.ev_stat, Coolio_StatWatcher_libev_callback, - RSTRING_PTR(path), + watcher_data->stat_path, interval == Qnil ? 0 : NUM2DBL(interval) ); watcher_data->event_types.ev_stat.data = (void *)self; diff --git a/ext/cool.io/watcher.c b/ext/cool.io/watcher.c index 2eeda89..4c12787 100644 --- a/ext/cool.io/watcher.c +++ b/ext/cool.io/watcher.c @@ -14,6 +14,7 @@ static VALUE cCoolio_Watcher = Qnil; static VALUE Coolio_Watcher_allocate(VALUE klass); static void Coolio_Watcher_mark(void *data); +static void Coolio_Watcher_free(void *data); static VALUE Coolio_Watcher_initialize(VALUE self); static VALUE Coolio_Watcher_attach(VALUE self, VALUE loop); @@ -56,7 +57,7 @@ static const rb_data_type_t Coolio_Watcher_type = { "Coolio::Watcher", { Coolio_Watcher_mark, - RUBY_DEFAULT_FREE, + Coolio_Watcher_free, }, }; @@ -75,6 +76,7 @@ static VALUE Coolio_Watcher_allocate(VALUE klass) watcher_data->loop = Qnil; watcher_data->enabled = 0; + watcher_data->stat_path = NULL; return watcher; } @@ -87,6 +89,16 @@ static void Coolio_Watcher_mark(void *data) rb_gc_mark(watcher_data->loop); } +static void Coolio_Watcher_free(void *data) +{ + struct Coolio_Watcher *watcher_data = data; + + if(watcher_data->stat_path) + xfree(watcher_data->stat_path); + + xfree(watcher_data); +} + static VALUE Coolio_Watcher_initialize(VALUE self) { rb_raise(rb_eRuntimeError, "watcher base class should not be initialized directly"); diff --git a/spec/stat_watcher_spec.rb b/spec/stat_watcher_spec.rb index d8ee0ab..68ee9c0 100644 --- a/spec/stat_watcher_spec.rb +++ b/spec/stat_watcher_spec.rb @@ -18,12 +18,31 @@ def on_change(previous, current) end end -def run_with_file_change(path) +def run_with_file_change(path, compact: false) reactor = Cool.io::Loop.new sw = MyStatWatcher.new(path) sw.attach(reactor) + # libev retains the path pointer passed to ev_stat_init() for the lifetime of + # the watcher. If the watcher held RSTRING_PTR(@path) directly, a compaction + # that relocates the @path String would leave libev dereferencing a stale + # address on every stat. Force a maximal relocation here so the watcher keeps + # operating against a path buffer it owns rather than Ruby-managed memory. + if compact + if GC.respond_to?(:verify_compaction_references) + # verify_compaction_references moves every movable object to a fresh slot. + # Its keyword arguments have varied across Ruby versions, so fall back. + begin + GC.verify_compaction_references(expand_heap: true, toplevel: true) + rescue ArgumentError + GC.verify_compaction_references(expand_heap: true) + end + elsif GC.respond_to?(:compact) + GC.compact + end + end + tw = Cool.io::TimerWatcher.new(INTERVAL, true) tw.on_timer do reactor.stop if sw.accessed @@ -69,6 +88,13 @@ def delete_file(path) expect(watcher.previous.ino).to eq(watcher.current.ino) end + it "keeps firing on_change after GC compaction relocates objects" do + skip "GC compaction not available" unless GC.respond_to?(:compact) + + watcher = run_with_file_change(TEMP_FILE_PATH, compact: true) + expect(watcher.accessed).to eq(true) + end + it "should raise when the handler does not take 2 parameters" do class MyStatWatcher < Cool.io::StatWatcher remove_method :on_change From 75ae12e3a2344910ca8423426b07caa4ed5000ec Mon Sep 17 00:00:00 2001 From: Shizuo Fujita Date: Fri, 3 Jul 2026 00:25:10 +0900 Subject: [PATCH 2/2] Use StringValueCStr for the StatWatcher path RSTRING_PTR does not, by contract, guarantee a NUL terminator, and it does nothing about embedded NUL bytes: a path such as "foo\0bar" would be silently truncated to "foo" when libev treats it as a C string. Copy from StringValueCStr(path) instead, which guarantees NUL termination and raises ArgumentError on an embedded NUL, matching how the standard library handles paths (e.g. File.stat). Co-Authored-By: Claude Opus 4.8 --- ext/cool.io/stat_watcher.c | 6 ++++-- spec/stat_watcher_spec.rb | 4 ++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/ext/cool.io/stat_watcher.c b/ext/cool.io/stat_watcher.c index f7054a5..00468d9 100644 --- a/ext/cool.io/stat_watcher.c +++ b/ext/cool.io/stat_watcher.c @@ -80,6 +80,7 @@ void Init_coolio_stat_watcher() static VALUE Coolio_StatWatcher_initialize(int argc, VALUE *argv, VALUE self) { VALUE path, interval; + const char *path_str; long path_len; struct Coolio_Watcher *watcher_data; @@ -88,6 +89,7 @@ static VALUE Coolio_StatWatcher_initialize(int argc, VALUE *argv, VALUE self) interval = rb_convert_type(interval, T_FLOAT, "Float", "to_f"); path = rb_String(path); + path_str = StringValueCStr(path); rb_iv_set(self, "@path", path); watcher_data = Coolio_Watcher_ptr(self); @@ -96,11 +98,11 @@ static VALUE Coolio_StatWatcher_initialize(int argc, VALUE *argv, VALUE self) * the watcher. RSTRING_PTR(path) points into a Ruby String whose buffer * GC.compact may relocate, leaving libev with a dangling pointer. Keep an * owned copy instead; it is released in Coolio_Watcher_free(). */ - path_len = RSTRING_LEN(path); + path_len = strlen(path_str); if(watcher_data->stat_path) xfree(watcher_data->stat_path); watcher_data->stat_path = xmalloc(path_len + 1); - strlcpy(watcher_data->stat_path, RSTRING_PTR(path), path_len + 1); + memcpy(watcher_data->stat_path, path_str, path_len + 1); watcher_data->dispatch_callback = Coolio_StatWatcher_dispatch_callback; ev_stat_init( diff --git a/spec/stat_watcher_spec.rb b/spec/stat_watcher_spec.rb index 68ee9c0..0a1a7d7 100644 --- a/spec/stat_watcher_spec.rb +++ b/spec/stat_watcher_spec.rb @@ -95,6 +95,10 @@ def delete_file(path) expect(watcher.accessed).to eq(true) end + it "raises ArgumentError when the path contains a null byte" do + expect { MyStatWatcher.new("foo\0bar") }.to raise_error(ArgumentError) + end + it "should raise when the handler does not take 2 parameters" do class MyStatWatcher < Cool.io::StatWatcher remove_method :on_change