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 misbehaviour with 256+ hooks on a same function #1809

Merged
merged 1 commit into from
Nov 22, 2022

Conversation

bwoebi
Copy link
Collaborator

@bwoebi bwoebi commented Nov 21, 2022

Description

Might also crash sometimes.

Readiness checklist

  • (only for Members) Changelog has been added to the release document.
  • Tests added for this feature/bug.

Reviewer checklist

  • Appropriate labels assigned.
  • Milestone is set.
  • Changelog has been added to the release document. For community contributors the reviewer is in charge of this task.

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
@bwoebi bwoebi added this to the 0.82.0 milestone Nov 21, 2022
@bwoebi bwoebi requested a review from a team as a code owner November 21, 2022 12:24
@@ -0,0 +1,26 @@
--TEST--
The tracer should not crash when many hooks are installed
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, did this test failed consistently before fixing?

Copy link
Collaborator Author

@bwoebi bwoebi Nov 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. (it did not crash, but create two spans)

@@ -184,12 +184,13 @@ void ddtrace_open_span(ddtrace_span_data *span) {
ddtrace_set_global_span_properties(span);
}

// += 2 increment to avoid zval type ever being 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks like magic :)

Copy link
Collaborator Author

@bwoebi bwoebi Nov 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I don't like it very much, but it works. But that's why there's a comment.

@morrisonlevi morrisonlevi added 🐛 bug Something isn't working c-extension Apply this label to issues and prs related to the C-extension labels Nov 22, 2022
@bwoebi bwoebi merged commit 3eabab1 into master Nov 22, 2022
@bwoebi bwoebi deleted the bob/fix-type-undef-overflow branch November 22, 2022 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working c-extension Apply this label to issues and prs related to the C-extension
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants