Skip to content

Commit 1b1df1b

Browse files
committed
[PROF-11475] Add extra safety checks when getting line number in profiler
**What does this PR do?** This PR adds a few extra checks when getting the line number during stack trace gathering in the profiler. We got one automated report of a crash in the `rb_iseq_line_no` function, and we couldn't reproduce it, so this is the first attempt at avoiding the issue -- let's see if we ever observe this crash again or not. **Motivation:** Improve stability of the profiler. **Additional Notes:** See the internal PROF-11475 ticket for more details. **How to test the change?** Our existing test coverage already validates that stack trace gathering works fine and matches the Ruby APIs. If this condition was not correct, we'd get more lines set to 0, and our unit tests would fail. (I did some quick manual testing to check this was the case -- and indeed it is).
1 parent 2f427f8 commit 1b1df1b

File tree

1 file changed

+8
-0
lines changed

1 file changed

+8
-0
lines changed

ext/datadog_profiling_native_extension/private_vm_api_access.c

+8
Original file line numberDiff line numberDiff line change
@@ -312,6 +312,7 @@ VALUE thread_name_for(VALUE thread) {
312312
// to support our custom rb_profile_frames (see below)
313313
// Modifications:
314314
// * Support int first_lineno for Ruby 3.2.0+ (https://github.com/ruby/ruby/pull/6430)
315+
// * Validate iseq and pos before calling `rb_iseq_line_no` as a safety measure (see comment below for details)
315316
//
316317
// `node_id` gets used depending on Ruby VM compilation settings (USE_ISEQ_NODE_ID being defined).
317318
// To avoid getting false "unused argument" warnings in setups where it's not used, we need to do this weird dance
@@ -358,6 +359,13 @@ calc_pos(const rb_iseq_t *iseq, const VALUE *pc, int *lineno, int *node_id)
358359
__builtin_trap();
359360
}
360361
#endif
362+
363+
// In PROF-11475 we spotted a crash when calling `rb_iseq_line_no` from this method. We couldn't reproduce or
364+
// figure out the root cause, but "just in case", we're validating that the iseq looks valid and that the
365+
// `n` used for the position is also sane, and if they don't look good, we don't calculate the line, rather
366+
// than potentially trigger any issues.
367+
if (RB_UNLIKELY(!RB_TYPE_P((VALUE) iseq, T_IMEMO) || n < 0 || n > ISEQ_BODY(iseq)->iseq_size)) return 0;
368+
361369
if (lineno) *lineno = rb_iseq_line_no(iseq, pos);
362370
#ifdef USE_ISEQ_NODE_ID
363371
if (node_id) *node_id = rb_iseq_node_id(iseq, pos);

0 commit comments

Comments
 (0)