Code review comment for ~athos-ribeiro/ubuntu/+source/php8.1:lp1990302-tmp-stream-file-pos

Revision history for this message
Bryce Harrington (bryce) wrote :

Results: (from http://autopkgtest.ubuntu.com/results/autopkgtest-jammy-athos-ribeiro-lp1990302-tmp-stream-file-pos/?format=plain)
  php8.1 @ amd64:
    20.10.22 23:34:14 Log 🗒️ ✅ Triggers: php8.1/8.1.2-1ubuntu2.7~ppa1
  php8.1 @ armhf:
    20.10.22 23:33:56 Log 🗒️ ✅ Triggers: php8.1/8.1.2-1ubuntu2.7~ppa1
  php8.1 @ ppc64el:
    20.10.22 23:37:20 Log 🗒️ ✅ Triggers: php8.1/8.1.2-1ubuntu2.7~ppa1
  php8.1 @ s390x:
    20.10.22 23:25:16 Log 🗒️ ✅ Triggers: php8.1/8.1.2-1ubuntu2.7~ppa1

In a jammy LXC container, verified the reproducer myself:

  $ sudo apt-get install php-common php-cli
  $ php reproduce.php
  string(16) "1111111111111111"
  int(1250)
  $ sudo add-apt-repository -yus ppa:athos-ribeiro/lp1990302-tmp-stream-file-pos
  $ sudo apt-get -y dist-upgrade
  $ php reproduce.php
  string(16) "2222222222222222"
  int(738)

I reviewed the patch's code, and verified it matches upstream.
Packaging changes look good.

The only thing I might mention, is that the changelog entry description is rather terse. The SRU [Impact] text is really informative though, and I wonder if some text could be borrowed from that. E.g.:

  * d/p/0049-Preserve-file-position-when-php-temp-switches.patch: PHP provides a
    temporary data stream, php://temp, whose contents are moved to a temporary file
    when a predefined size limit is hit. In jammy, the file position is set to the
    end of the file, which results in corrupted/unwanted data. Fix this by
    preserving the file position in this situation.
    (LP: #1990302)

Maybe a bit wordy but gives a better explanation of the change. This is just optional though; people can easily link over to the referenced bug to get the full explanation if they need.

review: Approve

« Back to merge proposal