Skip to content

Conversation

@morrisonlevi
Copy link
Collaborator

@morrisonlevi morrisonlevi commented May 16, 2025

PROF-11814

Description

A PHP application may crash happen if:

  • Using PHP 7.4+
  • The allocation profiler is enabled
  • And the ZEND_INIT_ARRAY opcode is executed

The engine currently does not save the opline in ZEND_INIT_ARRAY. Usually if it's out-dated and the line-number is a bit stale, it's not a big deal. But in some cases the opline will be dangling, and in those cases we read invalid memory. This is a bug in the PHP VM that needs to also be fixed upstream: php/php-src#18578. Since PHP 8.1 and 8.2 are in security fixes only, this will go into PHP 8.3 and 8.4, and we'll have to mitigate it on <8.3.

The fix works because by having a user opcode handler the engine will save the opline before it calls the user opcode handler. This has a small performance penalty for this specific opcode.

I do not have a reproducer for this one yet, but we've hit this issue before in other opcodes. I'm fairly confident this needs to be done until there is an upstream fix in PHP, then we can restrict this mitigation to version-specific ranges.

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

The engine currently does not save the opline in ZEND_INIT_ARRAY, so
under rare situations, that opline will be dangling, which can cause
crashes.

The fix works because by having a user opcode handler the engine will
save the opline before it calls the user opcode handler.

I do not have a reproducer for this one yet, but we've hit this issue
before so I'm fairly confident this needs to be done until there is
an upstream fix in PHP, then we can do version-specific mitigation.
@github-actions github-actions bot added profiling Relates to the Continuous Profiler tracing labels May 16, 2025
@pr-commenter
Copy link

pr-commenter bot commented May 16, 2025

Benchmarks [ profiler ]

Benchmark execution time: 2025-05-16 20:55:21

Comparing candidate commit f17cb34 in PR branch levi/ZEND_INIT_ARRAY with baseline commit 90fb177 in branch master.

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

@codecov-commenter
Copy link

codecov-commenter commented May 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.28%. Comparing base (90fb177) to head (f17cb34).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3255      +/-   ##
============================================
+ Coverage     76.34%   79.28%   +2.93%     
  Complexity     2948     2948              
============================================
  Files           145      118      -27     
  Lines         16082    11633    -4449     
  Branches       1110        0    -1110     
============================================
- Hits          12278     9223    -3055     
+ Misses         3227     2410     -817     
+ Partials        577        0     -577     
Flag Coverage Δ
appsec-extension ?
tracer-php 79.28% <ø> (+0.01%) ⬆️

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

see 28 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 90fb177...f17cb34. 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.

@morrisonlevi morrisonlevi marked this pull request as ready for review May 17, 2025 17:03
@morrisonlevi morrisonlevi requested a review from a team as a code owner May 17, 2025 17:03
@morrisonlevi morrisonlevi merged commit 2bf058f into master May 18, 2025
826 of 829 checks passed
@morrisonlevi morrisonlevi deleted the levi/ZEND_INIT_ARRAY branch May 18, 2025 23:01
@github-actions github-actions bot added this to the 1.10.0 milestone May 18, 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