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.

Sign up for GitHub

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

Jump to bottom

Fix possible crash in dd_patched_zend_call_known_function #2548

Merged
merged 4 commits into from
Mar 2, 2024
Merged

Fix possible crash in dd_patched_zend_call_known_function #2548

merged 4 commits into from
Mar 2, 2024

Conversation

bwoebi
Copy link
Collaborator

@bwoebi bwoebi commented Feb 29, 2024

Description

Fixes #2547.

Null-checks can be sometimes important.

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@bwoebi
Fix possible crash in dd_patched_zend_call_known_function
8989061 
Fixes #2547.

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
@bwoebi bwoebi requested a review from a team as a code owner February 29, 2024 11:34
@codecov-commenter
Copy link

codecov-commenter commented Feb 29, 2024 •
edited
Loading

Codecov Report

Merging #2548 (74a41f0) into master (552b0ef) will decrease coverage by 0.01%.
Report is 4 commits behind head on master.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2548      +/-   ##
============================================
- Coverage     76.55%   76.54%   -0.01%     
  Complexity      267      267              
============================================
  Files           138      138              
  Lines         17457    17458       +1     
  Branches        976      976              
============================================
  Hits          13364    13364              
- Misses         3573     3574       +1     
  Partials        520      520
Flag Coverage Δ
appsec-extension 69.13% <ø> (ø)
tracer-extension 78.68% <0.00%> (-0.01%) ⬇️
tracer-integrations 79.49% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
ext/ddtrace.c 73.01% <0.00%> (ø)
zend_abstract_interface/hook/hook.c 55.00% <0.00%> (-0.08%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 552b0ef...74a41f0. Read the comment docs.

@pr-commenter
Copy link

pr-commenter bot commented Feb 29, 2024 •
edited
Loading

Benchmarks

Benchmark execution time: 2024-03-02 00:07:00

Comparing candidate commit 5d9dea7 in PR branch bob/fix-execute-data with baseline commit 552b0ef in branch master.

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

scenario:TraceSerializationBench/benchSerializeTrace

  • 🟥 execution_time [+4.513µs; +7.187µs] or [+2.436%; +3.878%]

@PROFeNoM
Copy link
Contributor

Is this easily reproducible/Can we easily do a regression for this crash?

@bwoebi
Copy link
Collaborator Author

bwoebi commented Feb 29, 2024

Yes, it's easily reproducible. But it's a 8.1.2 specific change...

morrisonlevi
morrisonlevi approved these changes Feb 29, 2024
View reviewed changes
Copy link
Collaborator

@morrisonlevi morrisonlevi left a comment

Choose a reason for hiding this comment

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

Null pointers, man 🤦🏻‍♂️

@bwoebi bwoebi force-pushed the bob/fix-execute-data branch 5 times, most recently from 8c97770 to adc8854 Compare March 1, 2024 11:27
@bwoebi
Test extension for 8.1.2
8a8c6e6 
Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
@bwoebi
Fix possible segfault when removing hooks
c1eb746 
Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
bwoebi
bwoebi commented Mar 1, 2024
View reviewed changes
.circleci/continue_config.yml Outdated Show resolved Hide resolved
@bwoebi
Update .circleci/continue_config.yml
5d9dea7 
Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
@bwoebi bwoebi merged commit b3569d5 into master Mar 2, 2024
636 of 638 checks passed
@bwoebi bwoebi deleted the bob/fix-execute-data branch March 2, 2024 00:50
@github-actions github-actions bot added this to the 0.99.0 milestone Mar 2, 2024
bwoebi added a commit that referenced this pull request Mar 4, 2024
@bwoebi
Fix possible crash in dd_patched_zend_call_known_function (#2548)
c78f6d4 
* Fix possible crash in dd_patched_zend_call_known_function

Fixes #2547.

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>

* Test extension for 8.1.2

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>

* Fix possible segfault when removing hooks

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>

---------

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
@bwoebi bwoebi modified the milestones: 0.99.0, 0.98.1 Mar 4, 2024
@rokclimb15
Copy link

@bwoebi I wonder if this addresses #1734

@bwoebi
Copy link
Collaborator Author

bwoebi commented Mar 4, 2024 •
edited
Loading

@rokclimb15 Yes, that issue should actually be fixed :-)

And note that 0.98.1 has been released now with this fix too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@morrisonlevi morrisonlevi morrisonlevi approved these changes

@PROFeNoM PROFeNoM PROFeNoM approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.98.1
Development

Successfully merging this pull request may close these issues.

[Bug]: Segfault in dd_patched_zend_call_known_function
5 participants
@bwoebi @codecov-commenter @PROFeNoM @rokclimb15 @morrisonlevi