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(prof): avoid dlclose if threads did not join #3075

Merged
merged 3 commits into from
Feb 10, 2025

Conversation

morrisonlevi
Copy link
Collaborator

@morrisonlevi morrisonlevi commented Feb 6, 2025

Description

Null out the extension's handle if the ddprof_upload or ddprof_time threads timeout instead of joining. If the PHP engine dlclose's the handle and the shared object is unloaded while another thread is running, we've hit undefined behavior. This also probably results in a crash (on platforms that unload instead of no-op).

This may be the source of a crash when php-fpm does a log rotate. When doing the rotate, php-fpm shuts down all workers. If a worker is slow to process an upload and the timeout is hit, then we could hit this issue.

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

If the PHP engine dlclose's the handle and the shared object is
unloaded while another thread is running, we've hit undefined behavior.
This also probably results in a crash (on platforms that unload instead
of no-op).

This may be the source of a crash when php-fpm does a log rotate. When
doing the rotate, php-fpm shuts down all workers. If a worker is slow to
process an upload and the timeout is hit, then we could hit this issue.
@morrisonlevi morrisonlevi added cat:app-crash profiling Relates to the Continuous Profiler labels Feb 6, 2025
@codecov-commenter
Copy link

codecov-commenter commented Feb 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.74%. Comparing base (9cc3c1f) to head (0c5b86e).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3075      +/-   ##
============================================
+ Coverage     72.95%   74.74%   +1.79%     
  Complexity     2790     2790              
============================================
  Files           139      112      -27     
  Lines         15280    11042    -4238     
  Branches       1047        0    -1047     
============================================
- Hits          11147     8253    -2894     
+ Misses         3580     2789     -791     
+ Partials        553        0     -553     
Flag Coverage Δ
appsec-extension ?
tracer-php 74.74% <ø> (ø)

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

see 27 files with indirect coverage changes


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 9cc3c1f...0c5b86e. Read the comment docs.

@pr-commenter
Copy link

pr-commenter bot commented Feb 6, 2025

Benchmarks [ profiler ]

Benchmark execution time: 2025-02-10 05:57:08

Comparing candidate commit 0c5b86e in PR branch levi/fix-dlclose with baseline commit 9cc3c1f in branch master.

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

@morrisonlevi morrisonlevi marked this pull request as ready for review February 10, 2025 05:39
@morrisonlevi morrisonlevi requested review from a team as code owners February 10, 2025 05:39
Copy link
Member

@realFlowControl realFlowControl left a comment

Choose a reason for hiding this comment

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

Thanks @morrisonlevi ❤️

@realFlowControl realFlowControl merged commit c199419 into master Feb 10, 2025
488 of 511 checks passed
@realFlowControl realFlowControl deleted the levi/fix-dlclose branch February 10, 2025 07:08
@github-actions github-actions bot added this to the 1.7.0 milestone Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:app-crash profiling Relates to the Continuous Profiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants