Don't throw an error when serializing uninitialized typed properties with __sleep()

[Matthew Ruffell]

[Impact]

When you serialize an object in php7.4 on Focal, it checks to see if a magic __sleep() function is present, and if so, calls it before the object is serialized. Its intention is to give you an opportunity to close database connections or drop unnecessary data before you serialize it to disk.

Now, in php 7.4.2 a commit was introduced that implemented this behaviour, and they had implemented throwing an error when you try to serialize uninitialised typed data to match the semantics of what happens if you directly access an uninitialized typed attribute of an object.

This caused some problems, namely, you had to define an exception handler when you attempted to serialize, and you could end up catching actually important errors too early instead of the intended uninitialized data errors.

The community opened some bugs about this upstream, and it was decided to change the behaviour such that serialize with __sleep() has the same semantics as serialize without __sleep(), namely, removing the error that can be thrown.

[Testcase]

Start a fresh Focal VM, and follow the instructions below.

1) sudo apt install docker.io php7.4 php7.4-xml php7.4-pgsql make
2) curl -1sLf 'https://dl.cloudsmith.io/public/symfony/stable/setup.deb.sh' | sudo -E bash
3) sudo apt install symfony-cli composer
4) git clone https://github.com/2ec0b4/symfony-proxy-issue.git
5) cd symfony-proxy-issue
6) composer install --optimize-autoloader
6) make start
7) curl http://127.0.0.1:8000
...
Error:
Typed property Proxies\__CG__\App\Entity\Store::$ must not be accessed before initialization (in __sleep)

  at vendor/symfony/serializer/Normalizer/AbstractObjectNormalizer.php:626
  at serialize()
     (vendor/symfony/serializer/Normalizer/AbstractObjectNormalizer.php:626)
  at Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer->getCacheKey()
     (vendor/symfony/serializer/Normalizer/AbstractObjectNormalizer.php:327)
  at Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer->denormalize()
     (vendor/symfony/serializer/Serializer.php:191)
  at Symfony\Component\Serializer\Serializer->denormalize()
     (src/Controller/DefaultController.php:47)
  at App\Controller\DefaultController->index()
     (vendor/symfony/http-kernel/HttpKernel.php:146)
  at Symfony\Component\HttpKernel\HttpKernel->handleRaw()
     (vendor/symfony/http-kernel/HttpKernel.php:68)
  at Symfony\Component\HttpKernel\HttpKernel->handle()
     (vendor/symfony/http-kernel/Kernel.php:201)
  at Symfony\Component\HttpKernel\Kernel->handle()
     (public/index.php:25)
...

There is a test package available in the following ppa:

https://launchpad.net/~mruffell/+archive/ubuntu/sf350406-test

If you install the test package, the issue no longer occurs and the page renders
successfully.

$ curl -I http://127.0.0.1:8000
HTTP/1.1 200 OK

[Where problems could occur]

We are changing how object serialization occurs within __sleep(), such that it no longer throws an error when we try and serialize uninitialized types.

The impact should be minimal, since it brings the exact same semantics developers expect when they serialize objects without __sleep() to objects that use __sleep().

In the forward case of object serialization, it means we will no longer throw an error, so any exception handlers that have been implemented in users projects for this purpose won't be called anymore, due to serialization always returning success. Apart from some unreachable code, there is no detrimental effect.

In the backward case of creating an object from serialized data, there is a risk of a small regression or more so a change in behaviour. When the object is unserialized, uninitialized attributes will be replaced with default values as mentioned in the commit text:

> As in the non-__sleep() case, unserialize(serialize($x)) identity
> may not be preserved due to replacement of uninitialized/unset
> properties with default values. Fixing this will require changes to
> the serialization format.

If your code makes assumptions on uninitialized attributes vs attributes with default values, you are likely in trouble anyway, but this could cause a regression for some users.

The changes only affect when __sleep() is present, and no changes are made to any code paths where __sleep() is not defined, so the most executed path remains unchanged, and limits regressions to users that use __sleep() only.

[Other info]

Information about the __sleep() method can be found in the upstream documentation:

https://www.php.net/manual/en/language.oop5.magic.php#object.sleep

This issue was introduced upstream in php 7.4.2 in commit:

commit 846b6479537a112d1ded725e6484e46462048b35
From: Nikita Popov <email address hidden>
Date: Fri, 3 Jan 2020 15:35:10 +0100
Subject: Throw Error when referencing uninit typed prop in __sleep
Link: https://github.com/php/php-src/commit/846b6479537a112d1ded725e6484e46462048b35

The issue was then fixed upstream in php 7.4.6 in commit:

commit 73d02c3b3eb8b828a1cc7ae04a4cc4f4875c3ddd
From: Nicolas Grekas <email address hidden>
Date: Thu, 16 Apr 2020 00:11:38 +0200
Subject: Fix bug #79447
Link: https://github.com/php/php-src/commit/73d02c3b3eb8b828a1cc7ae04a4cc4f4875c3ddd

The fix cherry picks cleanly with no modifications necessary, apart from dropping the NEWS hunk.

[Matthew Ruffell (mruffell)]

Attached is a debdiff which fixes this bug on Focal.

[Heitor Alves de Siqueira (halves)]

I did some minor sanity checks before uploading, considering that this could affect a high number of users and reverse-deps:

- upstream has no further "fixes:" or patches touching this specific serialization behavior
- regression potential seems to be focused on users relying on undefined behavior or uninitialized objects, which is less likely to occur in official packages
- autopkgtests look good, as well as minor tests with some important reverse-deps (wordpress, pandoc, mediawiki)

In general, it looks good. Uploaded to focal, thanks Matthew!

[Athos Ribeiro (athos-ribeiro)]

Hi Heitor, Matthew (and SRU team).

Note that the NEW queue for focal [1] has a pending upload for php7.4. This is a minor fix for LP: #1989196, which I intended to let sit in blocked in proposed (staged SRU). That upload's version was already overriden once (by a security upload).

Would you like to merge both changes in a single upload?

[1] https://launchpad.net/ubuntu/focal/+queue?queue_state=1

[Matthew Ruffell (mruffell)]

Hi Athos,

Sorry, I didn't notice your upload, but yes, they should be merged for sure. This bug is perfect for yours to piggyback on without being marked as blocked-proposed.

Do you want me to make a new debdiff with both, or Heitor, can you merge them?

[Athos Ribeiro (athos-ribeiro)]

I can merge them and upload if you don't mind if I do it :)

[Athos Ribeiro (athos-ribeiro)]

Hi Matthew,

Bryce raised an interesting concern regarding the test case proposed in this bug [1]. The test case here requires downloading external software and is far from a minimal test. Would you be willing to provide a shorter test case? One option would be to just verify the upstream test case covers the issue, explain how it does so and use that test as the test case here.

[1] https://code.launchpad.net/~athos-ribeiro/ubuntu/+source/php7.4/+git/php7.4/+merge/434796

[Matthew Ruffell (mruffell)]

Hi Athos,

As mentioned in the merge request, the minimal testcase is the php testcase ext/standard/tests/serialize/sleep_uninitialized_typed_prop.phpt found in the patch itself, and when you build the package, it is executed and passes:

...
TEST 12251/13940 [ext/standard/tests/serialize/sleep_uninitialized_typed_prop.phpt]
PASS Referencing an uninitialized typed property in __sleep() should be skipped [ext/standard/tests/serialize/sleep_uninitialized_typed_prop.phpt]
...

I left the external testcase in the bug report since it is closer to what the customer sees on their systems during deployment, and the linked source repo has minimal additional dependencies and reproduces the problem nicely.

[Athos Ribeiro (athos-ribeiro)]

I uploaded the SRUs combined. Thanks!

[Andreas Hasenack (ahasenack)]

Hello Matthew, or anyone else affected,

Accepted php7.4 into focal-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/php7.4/7.4.3-4ubuntu2.16 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-focal to verification-done-focal. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-focal. 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 php7.4 (7.4.3-4ubuntu2.16) for focal have finished running.
The following regressions have been reported in tests triggered by the package:

php-excimer/1.0.0~git20190913.d82eaf7-1build1 (amd64)

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/focal/update_excuses.html#php7.4

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

Thank you!

[Matthew Ruffell (mruffell)]

Performing verification for Focal

I started a fresh new Focal VM and followed the testcase to get symfony installed and configured, as well as docker. The VM is running php7.4 7.4.3-4ubuntu2.15 from -updates.

Upon running the cloned project that reproduces the issue, I see:

$ curl http://127.0.0.1:8000
...
Error:
Typed property Proxies\__CG__\App\Entity\Store::$ must not be accessed before initialization (in __sleep)

  at vendor/symfony/serializer/Normalizer/AbstractObjectNormalizer.php:626
  at serialize()
     (vendor/symfony/serializer/Normalizer/AbstractObjectNormalizer.php:626)
  at Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer-&gt;getCacheKey()
     (vendor/symfony/serializer/Normalizer/AbstractObjectNormalizer.php:327)
  at Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer-&gt;denormalize()
     (vendor/symfony/serializer/Serializer.php:191)
  at Symfony\Component\Serializer\Serializer-&gt;denormalize()
     (src/Controller/DefaultController.php:47)
  at App\Controller\DefaultController-&gt;index()
     (vendor/symfony/http-kernel/HttpKernel.php:146)
  at Symfony\Component\HttpKernel\HttpKernel-&gt;handleRaw()
     (vendor/symfony/http-kernel/HttpKernel.php:68)
  at Symfony\Component\HttpKernel\HttpKernel-&gt;handle()
     (vendor/symfony/http-kernel/Kernel.php:201)
  at Symfony\Component\HttpKernel\Kernel-&gt;handle()
     (public/index.php:25)
...

I then went and enabled -proposed and upgraded php7.4 to 7.4.3-4ubuntu2.16, and restarted the reproducer.

We now see:

$ curl -I http://127.0.0.1:8000
HTTP/1.1 200 OK

The issue no longer reproduces.

Additionally, looking at the php testcase added to the source tarball, we see during build of 7.4.3-4ubuntu2.16 the test passes:

TEST 12251/13940 [ext/standard/tests/serialize/sleep_uninitialized_typed_prop.phpt]
PASS Referencing an uninitialized typed property in __sleep() should be skipped [ext/standard/tests/serialize/sleep_uninitialized_typed_prop.phpt]

The package in -proposed fixes the issue, happy to mark verified for Focal.

[Launchpad Janitor (janitor)]

This bug was fixed in the package php7.4 - 7.4.3-4ubuntu2.16

---------------
php7.4 (7.4.3-4ubuntu2.16) focal; urgency=medium

  [ Athos Ribeiro ]
  * d/rules: fix PHP_EXTRA_VERSION setting. (LP: #1989196)
  * Test PHP_EXTRA_VERSION setting with autopkgtest.

  [ Matthew Ruffell ]
  * No longer throw an error when serializing uninitialized typed
    properties with __sleep(), which makes serializing objects with
    __sleep() behave the same as serializing objects without
    __sleep(). (LP: #1999598)
    - d/p/lp-1999598-Fix-bug-79447.patch

 -- Athos Ribeiro <email address hidden> Thu, 15 Sep 2022 19:53:21 -0300

[Łukasz Zemczak (sil2100)]

The verification of the Stable Release Update for php7.4 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.