Change the semantics of rb_postponed_job_register

Our current implementation of rb_postponed_job_register suffers from
some safety issues that can lead to interpreter crashes (see bug #1991).
Essentially, the issue is that jobs can be called with the wrong
arguments.

We made two attempts to fix this whilst keeping the promised semantics,
but:
  * The first one involved masking/unmasking when flushing jobs, which
    was believed to be too expensive
  * The second one involved a lock-free, multi-producer, single-consumer
    ringbuffer, which was too complex

The critical insight behind this third solution is that essentially the
only user of these APIs are a) internal, or b) profiling gems.

For a), none of the usages actually require variable data; they will
work just fine with the preregistration interface.

For b), generally profiling gems only call a single callback with a
single piece of data (which is actually usually just zero) for the life
of the program. The ringbuffer is complex because it needs to support
multi-word inserts of job & data (which can't be atomic); but nobody
actually even needs that functionality, really.

So, this comit:
  * Introduces a pre-registration API for jobs, with a GVL-requiring
    rb_postponed_job_prereigster, which returns a handle which can be
    used with an async-signal-safe rb_postponed_job_trigger.
  * Deprecates rb_postponed_job_register (and re-implements it on top of
    the preregister function for compatability)
  * Moves all the internal usages of postponed job register
    pre-registration

1 files changed, 125 insertions, 21 deletions

diff --git a/include/ruby/debug.h b/include/ruby/debug.h
index e173f16e25..88bd721230 100644
--- a/include/ruby/debug.h
+++ b/include/ruby/debug.h
@@ -10,6 +10,7 @@
  *             modify this file, provided that  the conditions mentioned in the
  *             file COPYING are met.  Consult the file for details.
  */
+#include "ruby/internal/attr/deprecated.h"
 #include "ruby/internal/attr/nonnull.h"
 #include "ruby/internal/attr/returns_nonnull.h"
 #include "ruby/internal/dllexport.h"
@@ -615,48 +616,151 @@ VALUE rb_tracearg_object(rb_trace_arg_t *trace_arg);
 
 /*
  * Postponed Job API
- * rb_postponed_job_register and rb_postponed_job_register_one are
- * async-signal-safe and used via SIGPROF by the "stackprof" RubyGem
+ *
+ * This API is designed to be called from contexts where it is not safe to run Ruby
+ * code (e.g. because they do not hold the GVL or because GC is in progress), and
+ * defer a callback to run in a context where it _is_ safe. The primary intended
+ * users of this API is for sampling profilers like the "stackprof" gem; these work
+ * by scheduling the periodic delivery of a SIGPROF signal, and inside the C-level
+ * signal handler, deferring a job to collect a Ruby backtrace when it is next safe
+ * to do so.
+ *
+ * Historically, this API provided two functions `rb_postponed_job_register` and
+ * `rb_postponed_job_register_one`, which claimed to be fully async-signal-safe and
+ * would call back the provided `func` and `data` at an appropriate time. However,
+ * these functions were subject to race conditions which could cause crashes when
+ * racing with Ruby's internal use of them.
+ *
+ * Therefore, this API has now been changed, and now requires that jobs scheduled
+ * from a signal handler context are pre-registered in advance into a fixed-size
+ * table. This table is quite small (it only has 32 entries on most systems)
+ * and so gems should generally only preregister one or two funcs. This process is
+ * managed by the `rb_postponed_job_preregister` and `rb_postponed_job_trigger`
+ * functions.
+ *
+ * We also provide the old `rb_postponed_job_register` and
+ * `rb_postponed_job_register_one` functions for backwards compatability, but with
+ * changed semantics; `rb_postponed_job_register` now behaves the same as
+ * `rb_postponed_job_register_once`. These changes should remain compatible with all
+ * of the observed in-the-wild usages of the postponed job APIs, which almost all
+ * use the _one API and pass `0` for data anyway.
  */
 
+
 /**
  * Type of postponed jobs.
  *
- * @param[in,out]  arg What was passed to rb_postponed_job_register().
+ * @param[in,out]  arg What was passed to `rb_postponed_job_preregister`
  */
 typedef void (*rb_postponed_job_func_t)(void *arg);
 
 /**
- * Registers a postponed job.
+ * The type of a handle returned from `rb_postponed_job_preregister` and
+ * passed to `rb_postponed_job_trigger`
+ */
+typedef unsigned int rb_postponed_job_handle_t;
+#define POSTPONED_JOB_HANDLE_INVALID ((rb_postponed_job_handle_t)UINT_MAX)
+
+/**
+ * Pre-registers a func in Ruby's postponed job preregistration table,
+ * returning an opaque handle which can be used to trigger the job later. Generally,
+ * this function will be called during the initialization routine of an extension.
+ *
+ * The returned handle can be used later to call `rb_postponed_job_trigger`. This will
+ * cause Ruby to call back into the registered `func` with `data` at a later time, in
+ * a context where the GVL is held and it is safe to perform Ruby allocations.
+ *
+ * If the given func was already pre-registered, this method will overwrite the
+ * stored data with the newly passed data, and return the same handle instance as
+ * was previously returned.
+ *
+ * If this function is called concurrently with the same `func`, then the stored data
+ * could be the value from either call (but will definitely be one of them).
+ *
+ * If this function is called to update the data concurrently with a call to
+ * `rb_postponed_job_trigger` on the same handle, it's undefined whether `func` will
+ * be called with the old data or the new data.
+ *
+ * Although the current implementation of this method is in fact async-signal-safe and
+ * has defined semantics when called concurrently on the same `func`, a future Ruby
+ * version might require that this method be called under the GVL; thus, programs which
+ * aim to be forward-compatible should call this method whilst holding the GVL.
+ *
+ * @param[in]   func        The function to be pre-registered
+ * @param[in]   data        The data to be pre-registered
+ * @retval      POSTPONED_JOB_HANDLE_INVALID    The job table is full; this registration
+ *                          did not succeed and no further registration will do so for
+ *                          the lifetime of the program.
+ * @retval      otherwise   A handle which can be passed to `rb_postponed_job_trigger`
+ */
+rb_postponed_job_handle_t rb_postponed_job_preregister(rb_postponed_job_func_t func, void *data);
+
+/**
+ * Triggers a pre-registered job registered with rb_postponed_job_preregister,
+ * scheduling it for execution the next time the Ruby VM checks for interrupts.
+ * The context in which the job is called in holds the GVL and is safe to perform
+ * Ruby allocations within (i.e. it is not during GC).
+ *
+ * This method is async-signal-safe and can be called from any thread, at any
+ * time, including in signal handlers.
+ *
+ * If this method is called multiple times, Ruby will coalesce this into only
+ * one call to the job the next time it checks for interrupts.
+ *
+ * @params[in]  h   A handle returned from rb_postponed_job_preregister
+ */
+void rb_postponed_job_trigger(rb_postponed_job_handle_t h);
+
+/**
+ * Schedules the given `func` to be called with `data` when Ruby next checks for
+ * interupts. If this function is called multiple times in between Ruby checking
+ * for interrupts, then `func` will be called only once with the `data` vlaue from
+ * the first call to this function.
  *
- * There  are situations  when running  a ruby  program is  not possible.   For
- * instance when  a program is in  a signal handler; for  another instance when
- * the GC  is busy.   On such situations  however, there might  be needs  to do
- * something.  We cannot but defer such operations until we are 100% sure it is
- * safe  to execute  them.   This  mechanism is  called  postponed jobs.   This
- * function  registers a  new one.   The registered  job would  eventually gets
- * executed.
+ * Like `rb_postponed_job_trigger`, the context in which the job is called
+ * holds the GVL and can allocate Ruby objects.
  *
- * @param[in]      flags      (Unused) reserved for future extensions.
+ * This method essentially has the same semantics as:
+ *
+ * ```
+ *   rb_postponed_job_trigger(rb_postponed_job_preregister(func, data));
+ * ```
+ *
+ * @note    Prevoius versions of Ruby promised that the (`func`, `data`) pairs would
+ *          be executed as many times as they were registered with this function; in
+ *          reality this was always subject to race conditions and this function no
+ *          longer provides this guarantee. Instead, we only promise that `func` will
+ *          be called once.
+ *
+ * @deprecated  This interface implies that arbitrarily many `func`'s can be enqueued
+ *              over the lifetime of the program, whilst in reality the registration
+ *              slots for postponed jobs are a finite resource. This is made clearer
+ *              by the `rb_postponed_job_preregister` and `rb_postponed_job_trigger`
+ *              functions, and a future version of Ruby might delete this function.
+ *
+ * @param[in]      flags      Unused and ignored.
  * @param[in]      func       Job body.
  * @param[in,out]  data       Passed as-is to `func`.
- * @retval         0          Postponed job buffer is full.  Failed.
- * @retval         otherwise  Opaque return value.
- * @post           The passed job is postponed.
+ * @retval         0          Postponed job registration table is full. Failed.
+ * @retval         1          Registration succeeded.
+ * @post           The passed job will run on the next interrupt check.
  */
+ RBIMPL_ATTR_DEPRECATED(("use rb_postponed_job_preregister and rb_postponed_job_trigger"))
 int rb_postponed_job_register(unsigned int flags, rb_postponed_job_func_t func, void *data);
 
 /**
- * Identical to rb_postponed_job_register(), except  it additionally checks for
- * duplicated registration.  In case the passed job is already in the postponed
- * job buffer this function does nothing.
+ * Identical to `rb_postponed_job_register`
+ *
+ * @deprecated  This is deprecated for the same reason as `rb_postponed_job_register`
  *
- * @param[in]      flags      (Unused) reserved for future extensions.
+ * @param[in]      flags      Unused and ignored.
  * @param[in]      func       Job body.
  * @param[in,out]  data       Passed as-is to `func`.
- * @retval         0          Postponed job buffer is full.  Failed.
- * @retval         otherwise  Opaque return value.
+ * @retval         0          Postponed job registration table is full. Failed.
+ * @retval         1          Registration succeeded.
+ * @post           The passed job will run on the next interrupt check.
  */
+ RBIMPL_ATTR_DEPRECATED(("use rb_postponed_job_preregister and rb_postponed_job_trigger"))
 int rb_postponed_job_register_one(unsigned int flags, rb_postponed_job_func_t func, void *data);
 
 /** @} */