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): add missing "in-preloading" check to RSHUTDOWN #3037

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

realFlowControl
Copy link
Member

@realFlowControl realFlowControl commented Jan 14, 2025

Description

The profiler may crash when:

  1. The profiler is enabled,
  2. Allocation profiling is enabled (on by default),
  3. The preloading feature of PHP is on,
  4. And a custom memory handler other than Datadog's is used. This could be another profiler, debugger, or by setting the environment variable USE_ZEND_ALLOC to 0.

The profiler is not supposed to collect samples during preloading for technical reasons. However, RSHUTDOWN was missing the "in-preloading" check which would lead to a situation where some profile types could collect data even though in preloading. In allocation profiling, this would lead to a situation where if a customer has a custom memory handler (or uses USE_ZEND_ALLOC=0) the allocation profiler would crash in allocation::alloc_prof_rshutdown() when trying to reset the heap:

// Safety: `unwrap()` is safe here, as `heap` is initialized in `RINIT`
let heap = unsafe { (*zend_mm_state).heap.unwrap() };

Only the allocation profile types are known to be affected; the other types avoided this issue through other guards.

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@realFlowControl realFlowControl requested a review from a team as a code owner January 14, 2025 13:48
@realFlowControl realFlowControl changed the title Add missing "in-preloading" check to RSHUTDOWN fix(profiling) add missing "in-preloading" check to RSHUTDOWN Jan 14, 2025
@github-actions github-actions bot added profiling Relates to the Continuous Profiler tracing labels Jan 14, 2025
@codecov-commenter
Copy link

codecov-commenter commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.80%. Comparing base (46ec759) to head (5fced85).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3037   +/-   ##
=========================================
  Coverage     74.80%   74.80%           
  Complexity     2781     2781           
=========================================
  Files           112      112           
  Lines         11017    11017           
=========================================
  Hits           8241     8241           
  Misses         2776     2776           
Flag Coverage Δ
tracer-php 74.80% <ø> (ø)

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


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 46ec759...5fced85. Read the comment docs.

@realFlowControl realFlowControl force-pushed the florian/fix-preloading-profiling branch from 631d4ff to 1000e1c Compare January 14, 2025 14:05
@pr-commenter
Copy link

pr-commenter bot commented Jan 14, 2025

Benchmarks [ profiler ]

Benchmark execution time: 2025-01-14 16:11:40

Comparing candidate commit 5fced85 in PR branch florian/fix-preloading-profiling with baseline commit 46ec759 in branch master.

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

@realFlowControl realFlowControl force-pushed the florian/fix-preloading-profiling branch from 1000e1c to 8edc030 Compare January 14, 2025 14:31
`RSHUTDOWN` was missing the "in-preloading" check which would lead to a
situation where some profilers would collect data even though in
preloading. In allocation profiling this would lead to a situation where
if a customer has a custom memory handler (or uses `USE_ZEND_ALLOC=0`)
the allocation profiler would crash in
`allocation::alloc_prof_rshutdown()` when trying to reset the heap.
@realFlowControl realFlowControl force-pushed the florian/fix-preloading-profiling branch from 8edc030 to 5fced85 Compare January 14, 2025 16:00
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.

We went over the code together and the change looks good to us. The side-effects are a bit hard to reason about, maybe we can think of a way to represent invariants we need in code to help communicate these better in the future.

@morrisonlevi morrisonlevi changed the title fix(profiling) add missing "in-preloading" check to RSHUTDOWN fix(prof): add missing "in-preloading" check to RSHUTDOWN Jan 14, 2025
@realFlowControl realFlowControl self-assigned this Jan 15, 2025
@realFlowControl realFlowControl merged commit 82688ab into master Jan 15, 2025
687 of 736 checks passed
@realFlowControl realFlowControl deleted the florian/fix-preloading-profiling branch January 15, 2025 09:47
@github-actions github-actions bot added this to the 1.7.0 milestone Jan 15, 2025
@realFlowControl realFlowControl modified the milestones: 1.7.0, 1.6.3 Jan 17, 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