Skip to content

[NO-TICKET] Profiling: Verify iseq before calling rb_iseq_path#6014

Merged
ivoanjo merged 1 commit into
masterfrom
ivoanjo/check-before-rb_iseq_path
Jul 9, 2026
Merged

[NO-TICKET] Profiling: Verify iseq before calling rb_iseq_path#6014
ivoanjo merged 1 commit into
masterfrom
ivoanjo/check-before-rb_iseq_path

Conversation

@ivoanjo

@ivoanjo ivoanjo commented Jul 9, 2026

Copy link
Copy Markdown
Member

What does this PR do?

This PR adds an extra check that the pathobj of an iseq is non-null before we call rb_iseq_path on it.

Motivation:

We saw a null-pointer-dereference crash from a customer inside rb_iseq_path. Our current suspicion is that the pathobj itself might be NULL, and since it's cheap/easy to check, we're adding this extra safety check as:

  • Best case it fixes the issue
  • Worst case the issue still exists, and we learn that it's not the pathobj being NULL

Change log entry

Yes. Profiling: Add workaround for crash when collecting frame information

Additional Notes:

We've seen the following crash from a customer:

(see
https://app.datadoghq.com/logs?query=%40error.source_type%3ACrashtracking%20source%3Aruby%20%40error.stack.frames.function%3Arb_iseq_path&from_ts=1780997944044&to_ts=1783589944044)

SIGSEGV (SEGV_MAPERR) at address 0x0000000000000000. Address is
below the null-page threshold (0x0000000000001000), suggesting a
direct null dereference on a null pointer. Crash in
/usr/local/lib/libruby.so.3.4.8.

with the stack trace showing:

    0 rb_iseq_path (/usr/src/ruby/iseq.c:1384)
    1 sample_thread (datadog_profiling_native_extension/collectors_stack.c:306)

Looking at our code:

if (buffer->stack_buffer[i].is_ruby_frame) {
      VALUE name = rb_iseq_base_label(buffer->stack_buffer[i].as.ruby_frame.iseq);
      VALUE filename = rb_iseq_path(buffer->stack_buffer[i].as.ruby_frame.iseq); <--- But clearly we're talking about this line

      name_slice = NIL_P(name) ? DDOG_CHARSLICE_C("") : char_slice_from_ruby_string(name); <-- Line 306 is here

Two relevant details here are: we call rb_iseq_base_label before rb_iseq_path so probably it's not the iseq nor its body that are NULL:

rb_iseq_base_label(const rb_iseq_t *iseq)
{
    return ISEQ_BODY(iseq)->location.base_label;
}

VALUE
rb_iseq_path(const rb_iseq_t *iseq)
{
    return pathobj_path(ISEQ_BODY(iseq)->location.pathobj);
}

...and thus we suspect it's either the pathobj that's NULL (0x0) or something that the pathobj references.

It's not clear from looking at the Ruby 3.4.8 sources in what situation an iseq that's live on the stack might ever have a NULL.

We've seen around ~100 crashes referencing rb_iseq_path from the same customer in the last month; + 1 crash from a different customer that's slightly similar:

    0  rb_iseq_path (/usr/src/ruby/iseq.c:1384)
    1  control_frame_dump (src/ruby/vm_dump.c:133)
    2  rb_vm_bugreport (src/ruby/vm_dump.c:1134)
    3  rb_bug (usr/src/ruby/error.c:1115)
    4  read_barrier_signal (gc/default/default.c:3442)
    5  __kernel_rt_sigreturn
    6
    7
    8
    9
    10
    11
    12 grpc_1.0
    13 rb_gc_obj_free (/usr/src/ruby/gc.c:1364)
    14 gc_compact_plane (gc/default/default.c:5603)
    15 gc_sweep (gc/default/default.c:4144)
    16 gc_start (gc/default/default.c:6480)
    17 rb_gc_prepare_heap (/usr/src/ruby/gc.c:3213)
    18 proc_warmup (src/ruby/process.c:8861)

Here, Ruby itself is crashing during a crash when trying to access rb_iseq_path. One customer was on Ruby 3.4.8 and the other on 3.4.9 so that's interesting but with such a small sample size doesn't say a lot.

How to test the change?

Adding unit test coverage for this one would be... really awkward; this change is small enough that we'll have to validate it as-is.

**What does this PR do?**

This PR adds an extra check that the `pathobj` of an `iseq` is non-null
before we call `rb_iseq_path` on it.

**Motivation:**

We saw a null-pointer-dereference crash from a customer inside
`rb_iseq_path`. Our current suspicion is that the `pathobj` itself
might be NULL, and since it's cheap/easy to check, we're adding this
extra safety check as:

* Best case it fixes the issue
* Worst case the issue still exists, and we learn that it's not the
  `pathobj` being NULL

**Additional Notes:**

We've seen the following crash from a customer:

(see
<https://app.datadoghq.com/logs?query=%40error.source_type%3ACrashtracking%20source%3Aruby%20%40error.stack.frames.function%3Arb_iseq_path&from_ts=1780997944044&to_ts=1783589944044>)

> SIGSEGV (SEGV_MAPERR) at address 0x0000000000000000. Address is
> below the null-page threshold (0x0000000000001000), suggesting a
> direct null dereference on a null pointer. Crash in
> /usr/local/lib/libruby.so.3.4.8.

with the stack trace showing:

```
0 rb_iseq_path (/usr/src/ruby/iseq.c:1384)
1 sample_thread (datadog_profiling_native_extension/collectors_stack.c:306)
```

Looking at our code:

```c
if (buffer->stack_buffer[i].is_ruby_frame) {
      VALUE name = rb_iseq_base_label(buffer->stack_buffer[i].as.ruby_frame.iseq);
      VALUE filename = rb_iseq_path(buffer->stack_buffer[i].as.ruby_frame.iseq); <--- But clearly we're talking about this line

      name_slice = NIL_P(name) ? DDOG_CHARSLICE_C("") : char_slice_from_ruby_string(name); <-- Line 306 is here
```

Two relevant details here are: we call `rb_iseq_base_label` before
`rb_iseq_path` so probably it's not the `iseq` nor its `body` that are
NULL:

```c
rb_iseq_base_label(const rb_iseq_t *iseq)
{
    return ISEQ_BODY(iseq)->location.base_label;
}

VALUE
rb_iseq_path(const rb_iseq_t *iseq)
{
    return pathobj_path(ISEQ_BODY(iseq)->location.pathobj);
}
```

...and thus we suspect it's either the `pathobj` that's NULL (0x0)
or something that the `pathobj` references.

It's not clear from looking at the Ruby 3.4.8 sources in what
situation an iseq that's live on the stack might ever have a NULL.

We've seen around ~100 crashes referencing `rb_iseq_path` from the
same customer in the last month; + 1 crash from a different customer
that's slightly similar:

```
0  rb_iseq_path (/usr/src/ruby/iseq.c:1384)
1  control_frame_dump (src/ruby/vm_dump.c:133)
2  rb_vm_bugreport (src/ruby/vm_dump.c:1134)
3  rb_bug (usr/src/ruby/error.c:1115)
4  read_barrier_signal (gc/default/default.c:3442)
5  __kernel_rt_sigreturn
6
7
8
9
10
11
12 grpc_1.0
13 rb_gc_obj_free (/usr/src/ruby/gc.c:1364)
14 gc_compact_plane (gc/default/default.c:5603)
15 gc_sweep (gc/default/default.c:4144)
16 gc_start (gc/default/default.c:6480)
17 rb_gc_prepare_heap (/usr/src/ruby/gc.c:3213)
18 proc_warmup (src/ruby/process.c:8861)
```

Here, Ruby itself is crashing during a crash when trying to access
`rb_iseq_path`. One customer was on Ruby 3.4.8 and the other on 3.4.9
so that's interesting but with such a small sample size doesn't say
a lot.

**How to test the change?**

Adding unit test coverage for this one would be... really awkward;
this change is small enough that we'll have to validate it as-is.
@ivoanjo
ivoanjo requested review from a team as code owners July 9, 2026 09:44
@dd-octo-sts dd-octo-sts Bot added the profiling Involves Datadog profiling label Jul 9, 2026
@ivoanjo
ivoanjo enabled auto-merge July 9, 2026 10:03
@datadog-prod-us1-6

datadog-prod-us1-6 Bot commented Jul 9, 2026

Copy link
Copy Markdown

Tests

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 90.04% (+0.01%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: d54994d | Docs | Datadog PR Page | Give us feedback!

@pr-commenter

pr-commenter Bot commented Jul 9, 2026

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-07-09 10:08:32

Comparing candidate commit d54994d in PR branch ivoanjo/check-before-rb_iseq_path with baseline commit 4a3a8ed in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 45 metrics, 1 unstable metrics.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

  • 🟩 = significantly better candidate vs. baseline
  • 🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

@ivoanjo
ivoanjo merged commit cf99651 into master Jul 9, 2026
592 checks passed
@ivoanjo
ivoanjo deleted the ivoanjo/check-before-rb_iseq_path branch July 9, 2026 10:13
@dd-octo-sts dd-octo-sts Bot added this to the 2.38.0 milestone Jul 9, 2026
@ivoanjo ivoanjo added the identified-by:crashtracking Used to track internal stats for crashes! label Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

identified-by:crashtracking Used to track internal stats for crashes! profiling Involves Datadog profiling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants