Skip to content

Commit

Permalink
Fix race with timer signal in stackprof_start()
Browse files Browse the repository at this point in the history
I got a SEGV core dump with the stack trace as follows:

    #0  __pthread_kill_implementation (no_tid=0, signo=14, threadid=0) at ./nptl/pthread_kill.c:50
    tmm1#1  __pthread_kill_internal (signo=14, threadid=0) at ./nptl/pthread_kill.c:78
    tmm1#2  __GI___pthread_kill (threadid=0, signo=14) at ./nptl/pthread_kill.c:89
    tmm1#3  <signal handler called> () at /lib/x86_64-linux-gnu/libc.so.6
    tmm1#4  stackprof_start (argc=<optimized out>, argv=<optimized out>, self=<optimized out>) at stackprof.c:228
    tmm1#5  vm_call_cfunc_with_frame_
    ...

Notice that `threadid=0` in the top frame -- the SEGV comes from inside
libc as it tries to dereference `threadid`.

The signal comes from stackprof's signal handler:

    if (pthread_self() != _stackprof.target_thread) {
        pthread_kill(_stackprof.target_thread, sig);
        return;
    }

During stackprof_start(), `_stackprof.target_thread` is 0.

You can recreate the stack trace in the crash with a program that does
`pthread_kill(0, SIGALRM)`:

    #include <signal.h>

    int
    main(void)
    {
       pthread_kill(0, SIGALRM);
    }

Only set `running` after target_thread is set to avoid this crash in
case the timer expires after `settimer()` but before setting
`target_thread`.

Also, since the ordering is important here, make `running`
`volatile sig_atomic_t` to prevent the compiler from doing unwanted
reordering.
  • Loading branch information
XrXr committed Dec 10, 2024
1 parent 078f365 commit bd7516d
Showing 1 changed file with 3 additions and 2 deletions.
5 changes: 3 additions & 2 deletions ext/stackprof/stackprof.c
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ typedef struct {
} sample_time_t;

static struct {
int running;
volatile sig_atomic_t running;
int raw;
int aggregate;

Expand Down Expand Up @@ -217,7 +217,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 +225,8 @@ stackprof_start(int argc, VALUE *argv, VALUE self)
_stackprof.metadata = metadata;
_stackprof.out = out;
_stackprof.target_thread = pthread_self();
// Do this last since timer set above might expires
_stackprof.running = 1;

if (raw) {
capture_timestamp(&_stackprof.last_sample_at);
Expand Down

0 comments on commit bd7516d

Please sign in to comment.