fix(profiling): potential allocation profiling crashes with certain opcodes

[morrisonlevi]

Description

TODO: update the whole description for state of the current patch.

On PHP 7.4 and on PHP 8.0 before 8.0.26, the engine doesn't SAVE_OPLINE(); for the ZEND_GENERATOR_CREATE opcode. If the allocation profiler triggers on an allocation in this state, then it will sigsegv because the opline is non-null but invalid.

Note that the SAVE_OPLINE(); is missing on PHP < 7.4 as well, but on those versions i_init_func_execute_data initializes the opline. Coupled with the new test, we are confident that we don't need to install an opcode handler for those versions.

Who is affected by the crash?

• If you are running PHP 8.0.0 up to and including 8.0.25, then you are affected if you use a generator and the allocation profiler is enabled, and it's been enabled by default since dd-trace-php 0.89.0 (todo: double-check).
• On PHP 7, if you not running the tracer (only the profiler), then you are affected. This configuration is very rare, generally only Datadog engineers run this configuration.

Everything else is unaffected. To be clear:

• On PHP 7 you are fine if you also run the tracer (highly likely).
• On PHP 8, if you are on 8.0.26 or higher, then you are unaffected.
• On MacOS you are fine

In summary, no known customers are affected. Nearly everyone on PHP 7 runs the tracer, and nearly everyone is on a patched version of PHP 8.0, or on PHP 8.1 or newer. If you are on an affected version of PHP 8.0, as a quick mitigation you can disable the allocation profile by setting the ini datadog.profiling.allocation_enabled=0. You can do this with config mode of the setup script:

# Do this after `php datadog-setup.php --enable-profiling ...`
php datadog-setup.php config set --php-bin all \
    -ddatadog.profiling.allocation_enabled=0

Readiness checklist

• Changelog has been added to the release document.
• Tests added for this feature/bug.

Reviewer checklist

• Appropriate labels assigned.
• Milestone is set.

[pr-commenter]

Benchmarks

Benchmark execution time: 2023-12-29 17:05:07

Comparing candidate commit c004213 in PR branch levi-and-florian/fix-generator-create-crash with baseline commit 9f4cb76 in branch master.

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

[morrisonlevi]

todo: fix this comment lol

[realFlowControl]

Oh I remember. It is not only about fixing the comment, but try and see if the fix is enough, as there is a zend_array_dup() call few lines before the SAVE_OPLINE() happens. I'll check if I can trigger this, I highly assume it will crash as well.

[morrisonlevi]

Yes, we need to adjust the version range too. If the bad opline is still on PHP 8.0.12+, then we need to install the opcode handler on more versions too (maybe up to latest 😞).

[realFlowControl]

Jup, we need to install up to latest, can you check bf3fd02

[morrisonlevi]

Looks good. I add another commit that cleans things up and adds a pointless test for ZEND_BIND_STATIC with a todo.

[morrisonlevi]

I moved this back to draft. I need to figure out:

• Does the JIT do anything with these specific opcodes?
• If not, then bypass the handler check by installing these opcodes after they are checked.
• If so, rework the phpt files or use skipifs or something to accommodate the optional JIT warning about getting disabled if there are user opcode handlers.

[morrisonlevi]

ZEND_GENERATOR_CREATE is definitely referred to in the JIT a few times, but as far as I can tell, it's about determining exit/entry points, and nothing really about replacing the opcode itself. However, I am not the best at reading this JIT code.

ZEND_BIND_STATIC is never referred to in the JIT. I think we can safely install this opcode handler without affecting the JIT.

[realFlowControl]

@morrisonlevi I added ZEND_FUNC_GET_ARGS and finalised the PHP versions, now that the PRs have been merged and released.
There are other opcodes that we think should also crash, but we where unable to reproduce this to date. I would think that after successful review, we can merge this an continue in a new PR to add the other opcodes once we have a reproducer and a fix contributed to upstream

[morrisonlevi]

@realFlowControl Thank you! It looks good to me. I merged in master and added your upstream phpt with a long-form description. Can you do a final code review?

[bwoebi]

Looks good to me.

I also like set_run_time_php_version_id being moved to C code, looks much more readable there.