[GR-18163] Fix rb_thread_fd_select()

PullRequest: truffleruby/3542

CHANGELOG.md

@@ -11,6 +11,7 @@ Bug fixes:
 * Fix `rb_gc_register_address()`/`rb_global_variable()` to read the latest value (#2721, #2734, #2720, @eregon).
 * Synchronize concurrent writes to the same StringIO (@eregon).
 * Fix `StringIO#write(str)` when `str` is of an incompatible encoding and position < buffer size (#2770, @eregon).
+* Fix `rb_thread_fd_select()` to correctly initialize fdset copies and handle the timeout (@eregon).
 
 Compatibility:
 

lib/cext/ABI_version.txt

@@ -1 +1 @@
-2
+3

lib/cext/include/ruby/internal/intern/select.h

@@ -38,10 +38,12 @@
 # /** Does nothing (defined for compatibility). */
 # define rb_fd_resize(n, f) ((void)(f))
 #else
+#ifndef TRUFFLERUBY // Don't define an incorrect rb_fdset_t if NFDBITS is not set
 # include "ruby/internal/intern/select/posix.h"
 # /** Does nothing (defined for compatibility). */
 # define rb_fd_resize(n, f) ((void)(f))
 #endif
+#endif
 
 RBIMPL_SYMBOL_EXPORT_BEGIN()
 
@@ -79,7 +81,9 @@ struct timeval;
  * the reason for  this limitatuon in detail, you might  find this thread super
  * interesting: https://lkml.org/lkml/2004/10/6/117
  */
+#if defined(TRUFFLERUBY) && defined(NFDBITS) && defined(HAVE_RB_FD_INIT) // Don't define an incorrect rb_fdset_t if NFDBITS is not set
 int rb_thread_fd_select(int nfds, rb_fdset_t *rfds, rb_fdset_t *wfds, rb_fdset_t *efds, struct timeval *timeout);
+#endif
 
 RBIMPL_SYMBOL_EXPORT_END()
 

lib/cext/include/ruby/internal/intern/select/posix.h

@@ -35,6 +35,10 @@
 #include "ruby/internal/attr/nonnull.h"
 #include "ruby/internal/attr/pure.h"
 
+#ifdef TRUFFLERUBY
+#error "Expected defined(NFDBITS) && defined(HAVE_RB_FD_INIT)"
+#endif
+
 /**
  * The data structure which wraps the  fd_set bitmap used by `select(2)`.  This
  * allows Ruby to use FD sets larger than what has been historically allowed on

lib/cext/include/ruby/internal/intern/select/win32.h

@@ -27,6 +27,10 @@
 #include "ruby/internal/dllexport.h"
 #include "ruby/assert.h"
 
+#ifdef TRUFFLERUBY
+#error "Expected defined(NFDBITS) && defined(HAVE_RB_FD_INIT)"
+#endif
+
 /**@cond INTERNAL_MACRO */
 #define rb_fd_zero  rb_fd_zero
 #define rb_fd_clr   rb_fd_clr

spec/ruby/optional/capi/ext/io_spec.c

@@ -182,6 +182,46 @@ VALUE io_spec_rb_thread_fd_writable(VALUE self, VALUE io) {
   return Qnil;
 }
 
+VALUE io_spec_rb_thread_fd_select_read(VALUE self, VALUE io) {
+  int fd = io_spec_get_fd(io);
+
+  rb_fdset_t fds;
+  rb_fd_init(&fds);
+  rb_fd_set(fd, &fds);
+
+  int r = rb_thread_fd_select(fd + 1, &fds, NULL, NULL, NULL);
+  rb_fd_term(&fds);
+  return INT2FIX(r);
+}
+
+VALUE io_spec_rb_thread_fd_select_write(VALUE self, VALUE io) {
+  int fd = io_spec_get_fd(io);
+
+  rb_fdset_t fds;
+  rb_fd_init(&fds);
+  rb_fd_set(fd, &fds);
+
+  int r = rb_thread_fd_select(fd + 1, NULL, &fds, NULL, NULL);
+  rb_fd_term(&fds);
+  return INT2FIX(r);
+}
+
+VALUE io_spec_rb_thread_fd_select_timeout(VALUE self, VALUE io) {
+  int fd = io_spec_get_fd(io);
+
+  struct timeval timeout;
+  timeout.tv_sec = 10;
+  timeout.tv_usec = 20;
+
+  rb_fdset_t fds;
+  rb_fd_init(&fds);
+  rb_fd_set(fd, &fds);
+
+  int r = rb_thread_fd_select(fd + 1, NULL, &fds, NULL, &timeout);
+  rb_fd_term(&fds);
+  return INT2FIX(r);
+}
+
 VALUE io_spec_rb_io_binmode(VALUE self, VALUE io) {
   return rb_io_binmode(io);
 }
@@ -256,6 +296,9 @@ void Init_io_spec(void) {
   rb_define_method(cls, "rb_io_wait_writable", io_spec_rb_io_wait_writable, 1);
   rb_define_method(cls, "rb_thread_wait_fd", io_spec_rb_thread_wait_fd, 1);
   rb_define_method(cls, "rb_thread_fd_writable", io_spec_rb_thread_fd_writable, 1);
+  rb_define_method(cls, "rb_thread_fd_select_read", io_spec_rb_thread_fd_select_read, 1);
+  rb_define_method(cls, "rb_thread_fd_select_write", io_spec_rb_thread_fd_select_write, 1);
+  rb_define_method(cls, "rb_thread_fd_select_timeout", io_spec_rb_thread_fd_select_timeout, 1);
   rb_define_method(cls, "rb_wait_for_single_fd", io_spec_rb_wait_for_single_fd, 4);
   rb_define_method(cls, "rb_io_binmode", io_spec_rb_io_binmode, 1);
   rb_define_method(cls, "rb_fd_fix_cloexec", io_spec_rb_fd_fix_cloexec, 1);

spec/ruby/optional/capi/io_spec.rb

@@ -262,6 +262,21 @@
     end
   end
 
+  describe "rb_thread_fd_select" do
+    it "waits until an fd is ready for reading" do
+      @w_io.write "rb_thread_fd_select"
+      @o.rb_thread_fd_select_read(@r_io).should == 1
+    end
+
+    it "waits until an fd is ready for writing" do
+      @o.rb_thread_fd_select_write(@w_io).should == 1
+    end
+
+    it "waits until an fd is ready for writing with timeout" do
+      @o.rb_thread_fd_select_timeout(@w_io).should == 1
+    end
+  end
+
   platform_is_not :windows do
     describe "rb_io_wait_readable" do
       it "returns false if there is no error condition" do

src/main/c/cext/alloc.c

@@ -20,7 +20,7 @@ void ruby_malloc_size_overflow(size_t count, size_t elsize) {
      count, elsize);
 }
 
-size_t xmalloc2_size(const size_t count, const size_t elsize) {
+static size_t xmalloc2_size(const size_t count, const size_t elsize) {
   size_t ret;
   if (rb_mul_size_overflow(count, elsize, SSIZE_MAX, &ret)) {
     ruby_malloc_size_overflow(count, elsize);
@@ -29,35 +29,45 @@ size_t xmalloc2_size(const size_t count, const size_t elsize) {
 }
 
 void *ruby_xmalloc(size_t size) {
-  return malloc(size);
+  void* result = malloc(size);
+  if (result == NULL && size) {
+    rb_memerror();
+  }
+  return result;
 }
 
 void *ruby_xmalloc2(size_t n, size_t size) {
   size_t total_size = xmalloc2_size(n, size);
   if (total_size == 0) {
     total_size = 1;
   }
-  return malloc(total_size);
+  return ruby_xmalloc(total_size);
 }
 
 void* rb_xmalloc_mul_add(size_t x, size_t y, size_t z) {
   return ruby_xmalloc(x * y + z);
 }
 
 void *ruby_xcalloc(size_t n, size_t size) {
-  return calloc(n, size);
+  size_t total_size = xmalloc2_size(n, size);
+  void* result = calloc(1, total_size);
+  if (result == NULL && total_size) {
+    rb_memerror();
+  }
+  return result;
 }
 
 void *ruby_xrealloc(void *ptr, size_t new_size) {
-  return realloc(ptr, new_size);
+  void* result = realloc(ptr, new_size);
+  if (result == NULL && new_size) {
+    rb_memerror();
+  }
+  return result;
 }
 
 void *ruby_xrealloc2(void *ptr, size_t n, size_t size) {
-  size_t len = size * n;
-  if (n != 0 && size != len / n) {
-    rb_raise(rb_eArgError, "realloc: possible integer overflow");
-  }
-  return realloc(ptr, len);
+  size_t total_size = xmalloc2_size(n, size);
+  return ruby_xrealloc(ptr, total_size);
 }
 
 void ruby_xfree(void *address) {
@@ -68,7 +78,7 @@ void *rb_alloc_tmp_buffer(volatile VALUE *store, long len) {
   if (len == 0) {
     len = 1;
   }
-  void *ptr = malloc(len);
+  void *ptr = ruby_xmalloc(len);
   *((void**)store) = ptr;
   return ptr;
 }
@@ -78,7 +88,7 @@ void *rb_alloc_tmp_buffer_with_count(volatile VALUE *store, size_t size, size_t
 }
 
 void rb_free_tmp_buffer(volatile VALUE *store) {
-  free(*((void**)store));
+  ruby_xfree(*((void**)store));
 }
 
 void rb_mem_clear(VALUE *mem, long n) {
@@ -101,7 +111,7 @@ rb_imemo_tmpbuf_t *rb_imemo_tmpbuf_parser_heap(void *buf, rb_imemo_tmpbuf_t *old
      GC. This is not a problem for Ripper because we also mod that to free the
      heap when freeing the parser structure, but it might be a problem if other
      extensions use this function. */
-  rb_imemo_tmpbuf_t *imemo = malloc(sizeof(rb_imemo_tmpbuf_t));
+  rb_imemo_tmpbuf_t *imemo = ruby_xmalloc(sizeof(rb_imemo_tmpbuf_t));
   imemo->ptr = buf;
   imemo->next = old_heap;
   imemo->cnt = cnt;

src/main/c/cext/data.c

@@ -12,7 +12,7 @@
 // RData and RTypedData, rb_data_*, rb_typeddata_*
 
 static RUBY_DATA_FUNC rb_tr_free_function(RUBY_DATA_FUNC dfree) {
-  return (dfree == (RUBY_DATA_FUNC)RUBY_DEFAULT_FREE) ? free : dfree;
+  return (dfree == (RUBY_DATA_FUNC)RUBY_DEFAULT_FREE) ? ruby_xfree : dfree;
 }
 
 #undef rb_data_object_wrap
@@ -22,7 +22,7 @@ VALUE rb_data_object_wrap(VALUE klass, void *data, RUBY_DATA_FUNC dmark, RUBY_DA
 }
 
 VALUE rb_data_object_zalloc(VALUE klass, size_t size, RUBY_DATA_FUNC dmark, RUBY_DATA_FUNC dfree) {
-  void *data = calloc(1, size);
+  void *data = ruby_xcalloc(1, size);
   return rb_data_object_wrap(klass, data, dmark, dfree);
 }
 
@@ -34,7 +34,7 @@ VALUE rb_data_typed_object_wrap(VALUE ruby_class, void *data, const rb_data_type
 }
 
 VALUE rb_data_typed_object_zalloc(VALUE ruby_class, size_t size, const rb_data_type_t *data_type) {
-  void *data = calloc(1, size);
+  void *data = ruby_xcalloc(1, size);
   return rb_data_typed_object_wrap(ruby_class, data, data_type);
 }
 

src/main/c/cext/encoding.c

@@ -21,7 +21,7 @@ rb_encoding* rb_to_encoding(VALUE encoding) {
 }
 
 rb_encoding* rb_encoding_to_native(char* name) {
-  OnigEncodingType* native = calloc(1, sizeof(rb_encoding)); // calloc() to zero-fill
+  OnigEncodingType* native = ruby_xcalloc(1, sizeof(rb_encoding)); // ruby_xcalloc() to zero-fill
   native->name = name;
   return native;
 }

src/main/c/cext/fd.c

@@ -27,8 +27,8 @@
 
 void rb_fd_init(rb_fdset_t *fds) {
   fds->maxfd = 0;
-  fds->fdset = ALLOC(fd_set);
-  FD_ZERO(fds->fdset);
+  // TruffleRuby: use calloc() instead of ALLOC+FD_ZERO, because FD_ZERO is problematic on some platforms (GR-42420)
+  fds->fdset = RB_ZALLOC(fd_set);
 }
 
 void rb_fd_term(rb_fdset_t *fds) {
@@ -82,7 +82,7 @@ int rb_fd_isset(int n, const rb_fdset_t *fds) {
   if (n >= fds->maxfd) {
     return 0;
   }
-  return FD_ISSET(n, fds->fdset);
+  return FD_ISSET(n, fds->fdset) != 0;
 }
 
 void rb_fd_copy(rb_fdset_t *dst, const fd_set *src, int max) {
@@ -118,6 +118,8 @@ void rb_fd_init_copy(rb_fdset_t *dst, rb_fdset_t *src) {
   memcpy(dst->fdset, src->fdset, size);
 }
 
+// These functions are based on https://www.gnu.org/software/libc/manual/html_node/Calculating-Elapsed-Time.html
+// See also https://stackoverflow.com/questions/15846762/timeval-subtract-explanation
 static bool timespec_subtract(struct timespec *result, struct timespec x, struct timespec y) {
   /* Perform the carry for the later subtraction by updating y. */
   if (x.tv_nsec < y.tv_nsec) {
@@ -190,9 +192,9 @@ struct select_set {
   rb_fdset_t *rset;
   rb_fdset_t *wset;
   rb_fdset_t *eset;
-  rb_fdset_t *orig_rset;
-  rb_fdset_t *orig_wset;
-  rb_fdset_t *orig_eset;
+  rb_fdset_t orig_rset;
+  rb_fdset_t orig_wset;
+  rb_fdset_t orig_eset;
   struct timeval *timeout;
   struct timeval *orig_timeout;
 };
@@ -213,7 +215,7 @@ static bool update_timeout(struct timeval *timeout, struct timeval *orig_timeout
     timespec_subtract(&difftime, currenttime, *starttime);
     difftimeout.tv_sec = difftime.tv_sec;
     difftimeout.tv_usec = difftime.tv_nsec / 1000;
-    timeleft = timeval_subtract(timeout, *orig_timeout, difftimeout);
+    timeleft = !timeval_subtract(timeout, *orig_timeout, difftimeout);
   }
 
   return timeleft;
@@ -230,9 +232,9 @@ static void* rb_thread_fd_select_blocking(void *data) {
   int result = 0;
   bool timeleft = true;
   do {
-    restore_fds(set->rset, set->orig_rset);
-    restore_fds(set->wset, set->orig_wset);
-    restore_fds(set->eset, set->orig_eset);
+    restore_fds(set->rset, &set->orig_rset);
+    restore_fds(set->wset, &set->orig_wset);
+    restore_fds(set->eset, &set->orig_eset);
     timeleft = update_timeout(set->timeout, set->orig_timeout, &starttime);
     if (!timeleft) {
       break;
@@ -247,39 +249,48 @@ static void* rb_thread_fd_select_internal(void *sets) {
 }
 
 static void rb_thread_fd_select_set_free(struct select_set *sets) {
-  if (sets->orig_rset) {
-    rb_fd_term(sets->orig_rset);
+  if (sets->rset) {
+    rb_fd_term(&sets->orig_rset);
   }
-  if (sets->orig_wset) {
-    rb_fd_term(sets->orig_wset);
+  if (sets->wset) {
+    rb_fd_term(&sets->orig_wset);
   }
-  if (sets->orig_eset) {
-    rb_fd_term(sets->orig_eset);
+  if (sets->eset) {
+    rb_fd_term(&sets->orig_eset);
   }
 }
 
 static void fd_init_copy(rb_fdset_t *dst, int max, rb_fdset_t *src) {
   if (src) {
     rb_fd_resize(max - 1, src);
-    if (dst != src) {
-      rb_fd_init_copy(dst, src);
-    }
+    rb_fd_init_copy(dst, src);
+  } else {
+    dst->fdset = NULL;
+    dst->maxfd = 0;
   }
 }
 
 int rb_thread_fd_select(int max, rb_fdset_t *read, rb_fdset_t *write, rb_fdset_t *except, struct timeval *timeout) {
   // NOTE: MRI has more logic in here
   struct select_set set;
+  struct timeval orig_timeval;
+
   set.max = max;
   set.rset = read;
   set.wset = write;
   set.eset = except;
   set.timeout = timeout;
-  fd_init_copy(set.orig_rset, set.max, set.rset);
-  fd_init_copy(set.orig_wset, set.max, set.wset);
-  fd_init_copy(set.orig_eset, set.max, set.eset);
-  struct timeval orig_timeval = *timeout;
-  set.orig_timeout = &orig_timeval;
+
+  fd_init_copy(&set.orig_rset, max, set.rset);
+  fd_init_copy(&set.orig_wset, max, set.wset);
+  fd_init_copy(&set.orig_eset, max, set.eset);
+
+  if (timeout) {
+    orig_timeval = *timeout;
+    set.orig_timeout = &orig_timeval;
+  } else {
+    set.orig_timeout = NULL;
+  }
 
   void* result = rb_ensure(rb_thread_fd_select_internal, (VALUE)&set, rb_thread_fd_select_set_free, (VALUE)&set);
   return (int)(long)result;

src/main/c/cext/printf.c

@@ -192,7 +192,7 @@ VALUE rb_tr_get_sprintf_args(va_list args, VALUE types) {
     default:
       {
         char *err_str;
-        if (asprintf(&err_str, "unhandled rb_sprintf arg type %d", type) > 0 ) {
+        if (asprintf(&err_str, "unhandled rb_sprintf arg type %d", type) > 0) {
           rb_tr_error(err_str);
           free(err_str);
         }