Skip to content

fix(prof): validate opline before access#3319

Merged
realFlowControl merged 8 commits into
masterfrom
florian/check-valid-opline
Jul 11, 2025
Merged

fix(prof): validate opline before access#3319
realFlowControl merged 8 commits into
masterfrom
florian/check-valid-opline

Conversation

@realFlowControl

@realFlowControl realFlowControl commented Jun 27, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Description

When collecting the stack trace outside of a vm interrupt, the execute_data.opline might be a dangling pointer. So far we had some mitigations in place for known cases, but there are some cases where this may still happen. Granted this is extremely rare, but annoying.

The root cause is that the execute_data.opline pointer has a different lifetime than the execute_data.func.op_array it points into.

This PR:

  • adds an out-of-bounds check for accessing the opline
  • removes all our mitigations
  • runs the allocation_generator_01.phpt test only if PHP > 8.0.10 is used, as otherwise it crashes in PHP

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@github-actions github-actions Bot added profiling Relates to the Continuous Profiler tracing labels Jun 27, 2025
@pr-commenter

pr-commenter Bot commented Jun 27, 2025 •
edited
Loading

Copy link
Copy Markdown

Benchmarks [ profiler ]

Benchmark execution time: 2025-07-10 21:47:21

Comparing candidate commit 163d199 in PR branch florian/check-valid-opline with baseline commit bd558a9 in branch master.

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

scenario:walk_stack/1

  • 🟥 wall_time [+311.336ns; +334.943ns] or [+2.467%; +2.654%]

@realFlowControl
realFlowControl force-pushed the florian/check-valid-opline branch from 44ceea8 to 2bc5d1e Compare June 27, 2025 12:12
@realFlowControl
realFlowControl marked this pull request as ready for review June 27, 2025 12:15
@realFlowControl
realFlowControl requested a review from a team as a code owner June 27, 2025 12:15
@realFlowControl
realFlowControl force-pushed the florian/check-valid-opline branch 3 times, most recently from 1439293 to 84e3204 Compare June 27, 2025 13:14
morrisonlevi
morrisonlevi reviewed Jun 27, 2025
View reviewed changes
Comment thread profiling/src/profiling/stack_walking.rs Outdated
morrisonlevi
morrisonlevi reviewed Jun 27, 2025
View reviewed changes
Comment thread profiling/src/profiling/stack_walking.rs Outdated
@realFlowControl
realFlowControl force-pushed the florian/check-valid-opline branch 3 times, most recently from d82f513 to d802cc1 Compare July 1, 2025 17:07
@codecov-commenter

codecov-commenter commented Jul 1, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.83%. Comparing base (bd558a9) to head (163d199).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3319   +/-   ##
=======================================
  Coverage   61.83%   61.83%           
=======================================
  Files         140      140           
  Lines       12318    12318           
  Branches     1617     1617           
=======================================
  Hits         7617     7617           
  Misses       3989     3989           
  Partials      712      712

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 bd558a9...163d199. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@realFlowControl
realFlowControl force-pushed the florian/check-valid-opline branch 2 times, most recently from 8c94147 to 331167f Compare July 1, 2025 19:29
@realFlowControl realFlowControl mentioned this pull request Jul 2, 2025
Open
@realFlowControl
realFlowControl force-pushed the florian/check-valid-opline branch from 331167f to 692a304 Compare July 2, 2025 14:50
@realFlowControl
realFlowControl force-pushed the florian/check-valid-opline branch from 777f9aa to ad16bf1 Compare July 8, 2025 21:59
morrisonlevi
morrisonlevi reviewed Jul 10, 2025
View reviewed changes
Comment thread profiling/tests/phpt/allocation_generator_01.phpt
@realFlowControl
realFlowControl enabled auto-merge (squash) July 11, 2025 00:27
@pr-commenter

pr-commenter Bot commented Jul 11, 2025

Copy link
Copy Markdown

Benchmarks [ tracer ]

Benchmark execution time: 2025-07-11 00:40:21

Comparing candidate commit 163d199 in PR branch florian/check-valid-opline with baseline commit bd558a9 in branch master.

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

scenario:PDOBench/benchPDOBaseline-opcache

  • 🟩 execution_time [-5.900µs; -5.173µs] or [-8.847%; -7.757%]

scenario:PDOBench/benchPDOOverheadWithDBM-opcache

  • 🟥 execution_time [+8.504µs; +15.216µs] or [+2.322%; +4.154%]

@realFlowControl
realFlowControl merged commit c748dcc into master Jul 11, 2025
1850 of 1883 checks passed
@realFlowControl
realFlowControl deleted the florian/check-valid-opline branch July 11, 2025 00:43
@github-actions github-actions Bot added this to the 1.11.0 milestone Jul 11, 2025
@morrisonlevi morrisonlevi changed the title fix(profiling): validate opline before access fix(prof): validate opline before access Jul 11, 2025
This was referenced Jul 15, 2025
Closed
Closed
@realFlowControl realFlowControl mentioned this pull request Sep 17, 2025
Merged
2 tasks
@realFlowControl realFlowControl mentioned this pull request Nov 10, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bwoebi bwoebi bwoebi left review comments
@morrisonlevi morrisonlevi morrisonlevi approved these changes

Assignees

No one assigned

Labels

profiling Relates to the Continuous Profiler tracing

Projects

None yet

Milestone

1.11.0

Development

Successfully merging this pull request may close these issues.

4 participants

@realFlowControl @codecov-commenter @morrisonlevi @bwoebi