Skip to content
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.

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

Fix potential crash by importing upstream rb_profile_frames fix #3289

Merged
merged 1 commit into from
Dec 5, 2023

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Nov 29, 2023

What does this PR do?

This PR imports the changes from upstream
ruby/ruby#8415 into our copy of rb_profile_frames.

I'm not entirely sure if this crash could happen for ddtrace because unlike other Ruby profilers (like stackprof) we are not (yet?) sampling from the signal handler.

Motivation:

Make sure our rb_profile_frames matches upstream in terms of validation/safety.

Additional Notes:

N/A

How to test the change?

Good question -- I'll admit I don't quite know how to trigger the issue reported upstream. In any case, the change is quite small, so at most we'll report line 0 in a few cases where we previously weren't.

For Datadog employees:

  • If this PR touches code that signs or publishes builds or packages, or handles
    credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

**What does this PR do?**

This PR imports the changes from upstream
ruby/ruby#8415 into our copy of
`rb_profile_frames`.

I'm not entirely sure if this crash could happen for ddtrace because
unlike other Ruby profilers (like stackprof) we are not (yet?) sampling
from the signal handler.

**Motivation:**

Make sure our `rb_profile_frames` matches upstream in terms of
validation/safety.

**Additional Notes:**

N/A

**How to test the change?**

Good question -- I'll admit I don't quite know how to trigger the
issue reported upstream. In any case, the change is quite small, so
at most we'll report line 0 in a few cases where we previously weren't.
@ivoanjo ivoanjo requested a review from a team as a code owner November 29, 2023 15:11
@github-actions github-actions bot added the profiling Involves Datadog profiling label Nov 29, 2023
@ivoanjo ivoanjo merged commit 013cc26 into master Dec 5, 2023
219 checks passed
@ivoanjo ivoanjo deleted the ivoanjo/profilerimport-jit-fix branch December 5, 2023 09:14
@github-actions github-actions bot added this to the 1.18.0 milestone Dec 5, 2023
@TonyCTHsu TonyCTHsu mentioned this pull request Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
profiling Involves Datadog profiling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants