Fix PHP crashes due to accessing dangling pointers

[Brian Morton]

SRU Justification

[ Impact ]

Invoking reflection via the observer API on a class with an attribute annotation causes a dangling pointer and segmentation fault. Certain PHP extensions may register an observer of an attribute instantiation using reflection. Since Laravel 9+ and Symfony make use of attribute annotations, it's a fairly common case that can be reproduced using the Datadog PHP extension and any Laravel application. See https://github.com/DataDog/dd-trace-php/issues/1734.

This bug was fixed in PHP 8.1.3 https://bugs.php.net/bug.php?id=81430 in the PR https://github.com/php/php-src/pull/7885/files

This bug potentially impacts the stability of the LTS release for anyone using Laravel or Symfony which are very popular frameworks alongside tracing extensions.

[ Test Plan ]

Run the upstream tests included within this fix. To do so, an upstream development/testing PHP extension for zend introspection is required. We will provide the modified package source code so anyone verifying this bug can build it.

The new package to be built is named "php8.1-ztest".

The modified php8.1 source code to generate the php8.1-ztest package is located in https://code.launchpad.net/~athos-ribeiro/ubuntu/+source/php8.1/+git/php8.1/+ref/zend-test-ext-nofix for a first test to confirm the bug. The test should not meet the described expectation.

The following script should allow you to reproduce the bug:

##### BEGIN REPRODUCER #####

#!/bin/bash

set -eux

trap cleanup EXIT

TEST_CONTAINER=lp-2054621-php-dangling-ptr-reproduce
TEMP_DIR=$(mktemp -d)

cleanup() {
  rm -rf ${TEMP_DIR}
  lxc delete -f ${TEST_CONTAINER}
}

pushd ${TEMP_DIR}

git ubuntu clone php8.1
pushd php8.1
# git ubuntu remote add athos-ribeiro
# let's build the php8.1-ztest packages matching the version from the release pocket
git checkout zend-test-ext-nofix
git ubuntu export-orig

sbuild -d jammy
popd

lxc launch ubuntu-daily:jammy ${TEST_CONTAINER}
lxc exec ${TEST_CONTAINER} -- mkdir -p /usr/local/src

lxc file push php8.1-ztest_8.1.2-1ubuntu2.14_amd64.deb ${TEST_CONTAINER}/var/tmp/
lxc exec ${TEST_CONTAINER} -- apt update
lxc exec ${TEST_CONTAINER} -- apt install -y php git quilt
lxc exec ${TEST_CONTAINER} -- apt install -y /var/tmp/php8.1-ztest_8.1.2-1ubuntu2.14_amd64.deb
# we want the test files shipped with the fix
lxc exec ${TEST_CONTAINER} -- git clone -b zend-test-ext --depth=1 https://git.launchpad.net/~athos-ribeiro/ubuntu/+source/php8.1 /usr/local/src/php8.1
lxc exec --cwd /usr/local/src/php8.1 --env QUILT_PATCHES=debian/patches ${TEST_CONTAINER} -- quilt push -a

# This should fail
lxc exec --cwd /usr/local/src/php8.1 ${TEST_CONTAINER} -- php run-tests.php -P ext/zend_test/tests/observer_bug81430_1.phpt ext/zend_test/tests/observer_bug81430_2.phpt

##### END REPRODUCER #####

The modified php8.1 source code to generate the php8.1-ztest package is located in https://code.launchpad.net/~athos-ribeiro/ubuntu/+source/php8.1/+git/php8.1/+ref/zend-test-ext for a second test to confirm the fix. The test should now meet the expectations described in the test itself.

Note that the versions for the packages shipping "php8.1-ztest" are intentionally conflicting with the version in jammy and the version being proposed with the fix. This is because the generated php8.1-ztest requires other packages built from the php8.1 source in its exact same version.

Do remember that you should only install "php8.1-ztest" from these custom packages. The remaining php8.1 binaries should be installed from the Ubuntu archive.

The following script should allow you to verify the fix:

##### BEGIN CHECKER #####

#!/bin/bash

set -eux

trap cleanup EXIT

TEST_CONTAINER=lp-2054621-php-dangling-ptr-verify
TEMP_DIR=$(mktemp -d)

cleanup() {
  rm -rf ${TEMP_DIR}
  lxc delete -f ${TEST_CONTAINER}
}

pushd ${TEMP_DIR}

cat <<EOF > ubuntu-jammy-proposed.list
deb http://archive.ubuntu.com/ubuntu/ jammy-proposed restricted main multiverse universe
EOF

git ubuntu clone php8.1
pushd php8.1
# git ubuntu remote add athos-ribeiro
# let's build the php8.1-ztest packages matching the fixed version
git checkout zend-test-ext
git ubuntu export-orig

sbuild -d jammy
popd

lxc launch ubuntu-daily:jammy ${TEST_CONTAINER}
lxc exec ${TEST_CONTAINER} -- mkdir -p /usr/local/src

lxc file push php8.1-ztest_8.1.2-1ubuntu2.15_amd64.deb ${TEST_CONTAINER}/var/tmp/
lxc exec ${TEST_CONTAINER} -- apt update
lxc exec ${TEST_CONTAINER} -- apt install -y git quilt
# install php from proposed
lxc file push ubuntu-jammy-proposed.list ${TEST_CONTAINER}/etc/apt/sources.list.d/
lxc exec ${TEST_CONTAINER} -- apt update
lxc exec ${TEST_CONTAINER} -- apt install -y php/jammy-proposed
lxc exec ${TEST_CONTAINER} -- apt install -y /var/tmp/php8.1-ztest_8.1.2-1ubuntu2.15_amd64.deb
# we want the test files shipped with the fix
lxc exec ${TEST_CONTAINER} -- git clone -b zend-test-ext --depth=1 https://git.launchpad.net/~athos-ribeiro/ubuntu/+source/php8.1 /usr/local/src/php8.1
lxc exec --cwd /usr/local/src/php8.1 --env QUILT_PATCHES=debian/patches ${TEST_CONTAINER} -- quilt push -a

# This should succeed
lxc exec --cwd /usr/local/src/php8.1 ${TEST_CONTAINER} -- php run-tests.php -P ext/zend_test/tests/observer_bug81430_1.phpt ext/zend_test/tests/observer_bug81430_2.phpt

##### END CHECKER #####

[ Where problems could occur ]

Could potentially impact the performance or stability of reflection operations, but this is a fairly old patch at this point.

[Brian Morton (rokclimb15)]

It is also the cause of this bug in Symfony when using annotations https://bugs.php.net/bug.php?id=81648

[Ubuntu Foundations Team Bug Bot (crichton)]

The attachment "fix-attribute-instantion-dangling-pointer.patch" seems to be a patch. If it isn't, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are a member of the ~ubuntu-reviewers, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issues please contact him.]

[Athos Ribeiro (athos-ribeiro)]

Thanks for the patch, Brian!

Since this is a potential fix for a stable Ubuntu release, we would need to follow our SRU process here.

Would you be willing to file the template in this bug for that?

All you need to do is to change this bug report original description to match the template at https://wiki.ubuntu.com/StableReleaseUpdates#SRU_Bug_Template.

Now, about the attached patch, it should be OK (and nice to include) the test related to this change available in the original upstream commit at https://github.com/php/php-src/commit/2f6a06ccb0ef78e6122bb9e67f9b8b1ad07776e1. The contents of those tests should be enough to serve as a reproducer and as input for an SRU test case.

In the link above for the upstream commit, I see a discussion regarding a test failure due to a memory overflow.

It seems that Dimitry Stogov fixed it in https://github.com/php/php-src/commit/f7c3f6e7e25471da9cfb2ba082a77cc3c85bc6ed. Do we also need that patch to be included here?

Finally, the DEP3 headers in the patch should not start with '## '. Please check the patches available in this package sources and use https://dep-team.pages.debian.net/deps/dep3/ as a reference. You could instead just download the patches directly from github (e.g., https://github.com/php/php-src/commit/2f6a06ccb0ef78e6122bb9e67f9b8b1ad07776e1.patch) and add the DEP3 headers in them.

Note that I can sponsor an upload for you in case you are willing to push this one forward. For this, you would need to also add a debian/changelog entry to your patch (you should provide an actual debdiff or a merge proposal in our git repository in launchpad - let me know if you need any help with that).

[Brian Morton (rokclimb15)]

Thanks for your pointers! Do you know how to execute the PHP test suite (via debuild or directly) and invoke only a single test? Trying
to verify failure on the first test case.

Edit: I was able to locate https://github.com/php/php-src/blob/master/run-tests.php which seems to do the job.

[Brian Morton (rokclimb15)]

After some consideration, I've decided to include the memory overflow patch as well, since the extensions affected by this are primarily designed to catch, handle, and report such cases where errors are encountered in the code.

Attached is a debdiff, please let me know if you need anything else.

[Brian Morton (rokclimb15)]

Note that you must configure php with `--enable-zend-test` to be able to execute these tests. They are otherwise skipped.

[Athos Ribeiro (athos-ribeiro)]

Hi Brian,

Thank you for all the work you've been putting on this! The debdiff and the SRU paperwork (template) are looking great. I have a few comments below:

- The changelog entry looks good. Would you mind adding a reference to this bug there (check other changelog entries in the same file). It would be something like "d/p/fix-attribute-instantion-dangling-pointer.patch: [...]. (LP: #2054621)". This will instruct launchpad to close this bug when the package lands in the -updates pocket.

- Still in the changelog, you can target it to "jammy" instead of "UNRELEASED". The package will be uploaded as proposed here :)

- Now, regarding the patch file, since these are two different upstream commits, it would be better to have them each in a separate patch file, i.e., we want two new files under debian/patches (and two entries in debian/patches/series). The changelog would need to be adjusted accordingly.

- We only need one "Origin" entry in the DEP3 patch header. This will be solved once the point raised above is fixed.

- Finally, in the test plan, do you think we could just extract the code bits (enclosed in the php tags) from the test files, set the proper configuration in the php config files and check for expectations instead of running the test suite? This seems to be easier than running the test suite (that would require fetching the package sources, setting the user env, etc; instead of installing the package, changing config files and running the script).

Let me know if you need help with any of the above!

[Brian Morton (rokclimb15)]

Hi Athos,

I can do all of that today, with the possible exception of the test changes. These tests must run under the zend observer API to reproduce the crash which is enabled with the zend_test extension. That extension is not built by default and isn't packaged separately in main. Do you have a good source to use to install that extension for someone trying to run the tests? I had to ./configure --enable-zend-test and make to get those tests to execute. They're skipped if the extension is not available.

[Athos Ribeiro (athos-ribeiro)]

> That extension is not built by default and isn't packaged separately in main. Do you have a good source to use to install that extension for someone trying to run the tests?

Ah, we do not ship the extension, you are right!

My concern here is that we want to test the installed package and not a local build (it may sound odd on a first thought, but sometime it is really hard to predict what else could break when providing updates to stable releases - "better safe than sorry"?).

We can leave the test plan as is for now.

I will see if I can provide a PPA from where we can install just that extension on top of a default jammy PHP installation. Then we can update the test plan. Another option would be a local build of the extension only so we could run the test script. Do you have any better ideas?

Thanks again for pushing this through :)

[Brian Morton (rokclimb15)]

> I will see if I can provide a PPA from where we can install just that extension on top of a default jammy PHP installation.

I love this idea if it can be done! If not the local idea should work. I can reproduce the crash without this script, but it involves installing the Datadog extension and running a Laravel app. I'm glad to be the lab rat on the candidate package. It will be run in a production environment.

[Brian Morton (rokclimb15)]

Hi Athos,

Attached is a new patch with changes from your feedback. Please let me know if this needs any further adjustments.

[Bryce Harrington (bryce)]

[Setting targeting for jammy SRU]

[Athos Ribeiro (athos-ribeiro)]

Hi Brian,

Sorry for the delay on reviewing your patch here. As you may know, we had the 24.04 freeze last week and there were other packages/transitions I needed to tend to.

Your patch LGTM. I made a couple minor adjustments in the changelog entry and replaced the patches for the ones ported to PHP 8.1 in the upstream project (the ones in the 8.1 branch) since the ones provided were not being cleanly applied in the package currently available in Jammy.

You can see all the changes here https://code.launchpad.net/~athos-ribeiro/ubuntu/+source/php8.1/+git/php8.1/+merge/462003

Please review them to make sure you agree with them since I will be sponsoring this upload on your behalf.

I also updated the test plan to point to a modified version of the php8.1 sources which can generate the zend_test extension in a standalone package as we discussed before. I decided not to push them in to PPAs because we will need it in the exact same version as the php packages being tested and I cannot buld that in a ppa for the version currently available in jammy without major changes in the packaging sources. So whenever we are testing this, we will need to perform local builds of php8.1 to get those php8.1-zend-test packages.

[Brian Morton (rokclimb15)]

Hi Athos,

Looks great, and thanks for your help! Let me know if I can do anything else to help with this or future bugs.

[Robie Basak (racb)]

What's the status of this bug in Mantic and Noble please? Were 8.2 and 8.3 ever affected, and if so, in which versions were they fixed? I couldn't figure this out within a few minutes - sorry!

> [ Test Plan ]

> Run the upstream tests included within this fix. To do so, an upstream development/testing PHP extension for zend introspection is required.

Please could you provide the steps to follow to run the upstream tests included in this fix? This should be in enough detail that a developer who is not familiar with this package should be able to follow it.

Thanks!

[Brian Morton (rokclimb15)]

8.2 and 8.3 are unaffected. I'll let Athos chime in with the steps necessary.

[Athos Ribeiro (athos-ribeiro)]

This is fixed in PHP >= 8.1.3 (https://github.com/php/php-src/commit/2f6a06ccb0ef78e6122bb9e67f9b8b1ad07776e1)

I will update the test plan.

Thanks, Robie.

[Athos Ribeiro (athos-ribeiro)]

Test plan updated.

[Andreas Hasenack (ahasenack)]

I see some:

# git ubuntu remote add athos-ribeiro
# let's build the php8.1-ztest packages matching the version from the release pocket
git checkout zend-test-ext-nofix

I suppose you didn't mean to comment the remote add?

Also, this requires the git-ubuntu snap being installed. Which is fine, it's just not in the instructions, so beware.

[Andreas Hasenack (ahasenack)]

Hello Brian, or anyone else affected,

Accepted php8.1 into jammy-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/php8.1/8.1.2-1ubuntu2.15 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-jammy to verification-done-jammy. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-jammy. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Ubuntu SRU Bot (ubuntu-sru-bot)]

All autopkgtests for the newly accepted php8.1 (8.1.2-1ubuntu2.15) for jammy have finished running.
The following regressions have been reported in tests triggered by the package:

hoteldruid/3.0.3-1 (arm64)
mediawiki/1:1.35.6-1 (arm64)
php-imagick/3.6.0-4ubuntu1 (amd64, arm64, armhf, ppc64el, s390x)
php8.1/8.1.2-1ubuntu2.15 (arm64, i386)

Please visit the excuses page listed below and investigate the failures, proceeding afterwards as per the StableReleaseUpdates policy regarding autopkgtest regressions [1].

https://people.canonical.com/~ubuntu-archive/proposed-migration/jammy/update_excuses.html#php8.1

[1] https://wiki.ubuntu.com/StableReleaseUpdates#Autopkgtest_Regressions

Thank you!

[Athos Ribeiro (athos-ribeiro)]

I verified this change by running the script described in the test plan above.

with php8.1 8.1.2-1ubuntu2.14, the two tests run fail.

With the version in proposed, 8.1.2-1ubuntu2.15, they pass:

Running selected tests.
PASS Bug #81430 (Attribute instantiation frame accessing invalid frame pointer) [ext/zend_test/tests/observer_bug81430_1.phpt]
PASS Bug #81430 (Attribute instantiation leaves dangling execute_data pointer) [ext/zend_test/tests/observer_bug81430_2.phpt]

Before marking this bug as verified, I will investigate the autopkgtest issues listed above.

[Athos Ribeiro (athos-ribeiro)]

hoteldruid/3.0.3-1 (arm64)
mediawiki/1:1.35.6-1 (arm64)
php-imagick/3.6.0-4ubuntu1 (amd64, arm64, armhf, ppc64el, s390x)
php8.1/8.1.2-1ubuntu2.15 (arm64, i386)

These were either flaky tests (re-triggering fixed the issue) or failures unrelated to this SRU (migration-reference/0 runs failed).

We should proceed with the SRU.

[Andreas Hasenack (ahasenack)]

The verification of the Stable Release Update for php8.1 has completed successfully and the package is now being released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

[Launchpad Janitor (janitor)]

This bug was fixed in the package php8.1 - 8.1.2-1ubuntu2.15

---------------
php8.1 (8.1.2-1ubuntu2.15) jammy; urgency=medium

  * d/p/fix-attribute-instantion-dangling-pointer.patch: Fix sigsegv from
    dangling pointer on attribute observer. (LP: #2054621)
  * d/p/fix-attribute-instantion-memory-overflow-recovery.patch: Fix sigsegv
    during memory overflow recovery on attribute observer.

 -- Brian Morton <email address hidden> Fri, 23 Feb 2024 12:26:53 -0500