Skip to content

fix crashes with not-yet fully-initialized stackprof global state #226

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tenderlove merged 2 commits into from
Dec 18, 2024
Merged

fix crashes with not-yet fully-initialized stackprof global state #226

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
08b5127
don't set `running` until all relevant state is initialized
froydnj Feb 5, 2024
4e504d3
be more diligent about atomic operations
froydnj Feb 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 41 additions & 11 deletions ext/stackprof/stackprof.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,19 @@ typedef struct {
int64_t delta_usec;
} sample_time_t;

/* We need to ensure that various memory operations are visible across
* threads. Ruby doesn't offer a portable way to do this sort of detection
* across all the Ruby versions we support, so we use something that casts a
* wide net (Clang, along with ICC, defines __GNUC__). */
#if defined(__GNUC__) && defined(__ATOMIC_SEQ_CST)
#define STACKPROF_HAVE_ATOMICS 1
#else
#define STACKPROF_HAVE_ATOMICS 0
#endif

static struct {
/* Access this field with the `STACKPROF_RUNNING` macro, below, since we
* can't properly express that this field has an atomic type. */
int running;
int raw;
int aggregate;
Expand Down Expand Up @@ -133,6 +145,12 @@ static struct {
pthread_t target_thread;
} _stackprof;

#if STACKPROF_HAVE_ATOMICS
#define STACKPROF_RUNNING() __atomic_load_n(&_stackprof.running, __ATOMIC_ACQUIRE)
#else
#define STACKPROF_RUNNING() _stackprof.running
#endif

static VALUE sym_object, sym_wall, sym_cpu, sym_custom, sym_name, sym_file, sym_line;
static VALUE sym_samples, sym_total_samples, sym_missed_samples, sym_edges, sym_lines;
static VALUE sym_version, sym_mode, sym_interval, sym_raw, sym_raw_lines, sym_metadata, sym_frames, sym_ignore_gc, sym_out;
Expand All @@ -154,7 +172,7 @@ stackprof_start(int argc, VALUE *argv, VALUE self)
int raw = 0, aggregate = 1;
VALUE metadata_val;

if (_stackprof.running)
if (STACKPROF_RUNNING())
return Qfalse;

rb_scan_args(argc, argv, "0:", &opts);
Expand Down Expand Up @@ -217,7 +235,6 @@ stackprof_start(int argc, VALUE *argv, VALUE self)
rb_raise(rb_eArgError, "unknown profiler mode");
}

_stackprof.running = 1;
_stackprof.raw = raw;
_stackprof.aggregate = aggregate;
_stackprof.mode = mode;
Expand All @@ -226,6 +243,13 @@ stackprof_start(int argc, VALUE *argv, VALUE self)
_stackprof.metadata = metadata;
_stackprof.out = out;
_stackprof.target_thread = pthread_self();
/* We need to ensure previous initialization stores are visible across
* threads. */
#if STACKPROF_HAVE_ATOMICS
__atomic_store_n(&_stackprof.running, 1, __ATOMIC_SEQ_CST);
#else
_stackprof.running = 1;
#endif

if (raw) {
capture_timestamp(&_stackprof.last_sample_at);
Expand All @@ -240,9 +264,15 @@ stackprof_stop(VALUE self)
struct sigaction sa;
struct itimerval timer;

#if STACKPROF_HAVE_ATOMICS
int was_running = __atomic_exchange_n(&_stackprof.running, 0, __ATOMIC_SEQ_CST);
if (!was_running)
return Qfalse;
#else
if (!_stackprof.running)
return Qfalse;
_stackprof.running = 0;
#endif

if (_stackprof.mode == sym_object) {
rb_tracepoint_disable(objtracer);
Expand Down Expand Up @@ -351,7 +381,7 @@ stackprof_results(int argc, VALUE *argv, VALUE self)
{
VALUE results, frames;

if (!_stackprof.frames || _stackprof.running)
if (!_stackprof.frames || STACKPROF_RUNNING())
return Qnil;

results = rb_hash_new();
Expand Down Expand Up @@ -455,7 +485,7 @@ stackprof_run(int argc, VALUE *argv, VALUE self)
static VALUE
stackprof_running_p(VALUE self)
{
return _stackprof.running ? Qtrue : Qfalse;
return STACKPROF_RUNNING() ? Qtrue : Qfalse;
}

static inline frame_data_t *
Expand Down Expand Up @@ -719,23 +749,23 @@ stackprof_sample_and_record(void)
static void
stackprof_job_record_gc(void *data)
{
if (!_stackprof.running) return;
if (!STACKPROF_RUNNING()) return;

stackprof_record_gc_samples();
}

static void
stackprof_job_sample_and_record(void *data)
{
if (!_stackprof.running) return;
if (!STACKPROF_RUNNING()) return;

stackprof_sample_and_record();
}

static void
stackprof_job_record_buffer(void *data)
{
if (!_stackprof.running) return;
if (!STACKPROF_RUNNING()) return;

stackprof_record_buffer();
}
Expand All @@ -747,7 +777,7 @@ stackprof_signal_handler(int sig, siginfo_t *sinfo, void *ucontext)

_stackprof.overall_signals++;

if (!_stackprof.running) return;
if (!STACKPROF_RUNNING()) return;

// There's a possibility that the signal handler is invoked *after* the Ruby
// VM has been shut down (e.g. after ruby_cleanup(0)). In this case, things
Expand Down Expand Up @@ -810,7 +840,7 @@ stackprof_newobj_handler(VALUE tpval, void *data)
static VALUE
stackprof_sample(VALUE self)
{
if (!_stackprof.running)
if (!STACKPROF_RUNNING())
return Qfalse;

_stackprof.overall_signals++;
Expand Down Expand Up @@ -854,7 +884,7 @@ static void
stackprof_atfork_prepare(void)
{
struct itimerval timer;
if (_stackprof.running) {
if (STACKPROF_RUNNING()) {
if (_stackprof.mode == sym_wall || _stackprof.mode == sym_cpu) {
memset(&timer, 0, sizeof(timer));
setitimer(_stackprof.mode == sym_wall ? ITIMER_REAL : ITIMER_PROF, &timer, 0);
Expand All @@ -866,7 +896,7 @@ static void
stackprof_atfork_parent(void)
{
struct itimerval timer;
if (_stackprof.running) {
if (STACKPROF_RUNNING()) {
if (_stackprof.mode == sym_wall || _stackprof.mode == sym_cpu) {
timer.it_interval.tv_sec = 0;
timer.it_interval.tv_usec = NUM2LONG(_stackprof.interval);
Expand Down