php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #81096 Inconsistent opcache behavior with variables passed by reference to mysqli
Submitted: 2021-06-01 18:43 UTC Modified: 2021-06-10 08:59 UTC
From: nickdnk at hotmail dot com Assigned: nikic (profile)
Status: Closed Package: opcache
PHP Version: 8.0.6 OS: Linux/docker
Private report: No CVE-ID: None
View Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
If you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: nickdnk at hotmail dot com
New email:
PHP Version: OS:

 

 [2021-06-01 18:43 UTC] nickdnk at hotmail dot com
Description:
------------
Hello

I have a hard time describing what the problem here actually is, as I'm unsure of the root cause. I have created a complete docker-compose setup that demonstrates the behavior, which is available here: https://github.com/nickdnk/php-bug-demo

Clone it and run docker-compose up -d, wait for the containers to boot and composer to run (just wait like 20 seconds or look at the container logs) then navigate to localhost:8001 for PHP 8.0.6 and localhost:8002 for PHP 7.4.15. Notice that the output is not identical and that PHP 8 got it wrong.

Test script:
---------------
https://github.com/nickdnk/php-bug-demo

See the src/Test.php class. I pass $bind_integer by reference to $stmt->bind_result(), then set it to 0 and call fetch(). Removing the "set to 0"-line causes the problem to go away, but that does not explain what's going on or why this changed between 7 and 8.

The reason I use the Container class to return it is that if I read the variable, error_log or var_dump it directly, it behaves as expected, which is very weird (?). Removing the (int) cast also causes it to behave as expected.

Disabling opcache "fixes" the bug, so it also is not present when using Xdebug, which makes it very hard to debug (and reproduce, mind you), hence the kind of complicated setup to demonstrate it.

If you modify the scripts (for debugging purposes), navigate to localhost:8001/opcache or localhost:8002/opcache to reset the opcache. Otherwise you would have to restart the web containers.

The PHP images used are the official ones from https://hub.docker.com/_/php with Composer, MySQLi, PDO etc. added to them, so everything is more or less default. I only changed the opcache configuration as visible in the php.ini file attached to the containers, as this is not enabled by default. You could probably take some of these lines away though:

FROM php:8.0.6-apache # or 7.4.15
RUN apt-get update && \
    apt-get install -y libpng-dev \
    libjpeg-dev \
    libfreetype6-dev \
    libzip-dev \
    git \
    zip \
    unzip \
    mariadb-client
RUN docker-php-ext-configure gd --with-freetype=/usr/include/ --with-jpeg=/usr/include/
RUN docker-php-ext-install \
    mysqli \
    gd \
    exif \
    zip \
    pdo_mysql \
    bcmath

RUN php -r "readfile('http://getcomposer.org/installer');" | php -- --install-dir=/usr/bin/ --filename=composer && \
    apt-get -y autoremove && \
    apt-get clean && \
    rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/*


Patches

Pull Requests

Pull requests:

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2021-06-06 12:25 UTC] nickdnk at hotmail dot com
I would like to add that I am well aware that this code might not make a lot of sense, and that it can be "fixed" quite easily by changing a few lines, but that's not the point. The point is that there is a difference between PHP 7 and 8 which does not appear to be documented anywhere, and I don't know what other issues this might cause.
 [2021-06-07 13:03 UTC] nikic@php.net
-Status: Open +Status: Verified
 [2021-06-07 13:10 UTC] nikic@php.net
Reduced:

<?php
  
function test() {
    escape_x($x);
    $x = 0;
    modify_x();
    return (int) $x;
}

function escape_x(&$x) {
    $GLOBALS['x'] =& $x;
}

function modify_x() {
    $GLOBALS['x']++;
}

var_dump(test());
 [2021-06-07 13:30 UTC] nikic@php.net
-Status: Verified +Status: Analyzed
 [2021-06-07 13:30 UTC] nikic@php.net
This is a tricky issue. Before SCCP we have:

0000 INIT_FCALL 1 144 string("escape_x")
0001 SEND_REF #0.CV0($x) [undef] RANGE[0..0] -> #1.CV0($x) NOVAL [ref, any] 1
0002 DO_UCALL
0003 ASSIGN #1.CV0($x) NOVAL [ref, any] -> #2.CV0($x) [ref, any] RANGE[0..0] int(0)
0004 INIT_FCALL 0 128 string("modify_x")
0005 DO_UCALL
0006 #3.T1 [long] RANGE[0..0] = CAST (long) #2.CV0($x) [ref, any] RANGE[0..0]
0007 RETURN #3.T1 [long] RANGE[0..0]

SCCP sees that #3.T1 has RANGE[0..0] and replaces it with a zero literal.

Presumably the reason why it appeared in PHP 8.0 is https://github.com/php/php-src/commit/e0a8c7a8d0b312aae45ef46d27686771fc9297e9. Previously the substitution wouldn't have happened because T1 is a temporary. But it's easy to come up with a variant that fails on PHP 7.4 as well:

function test() {
    escape_x($x);
    $x = 0;
    modify_x();
    $y = (int) $x;
    return $y; 
}

We could mitigate this problem in SCCP by simply not using single-element range information. However, the more fundamental problem here is that the range information is simply wrong, and other incorrect optimizations may be done based on that.

For example, after disabling the SCCP replacement, if we consider this example:

function test() {
    escape_x($x);
    $x = 0; 
    modify_x();
    return PHP_INT_MAX + (int) $x;
}

Then the result will change from float(9.223372036854776E+18) to int(-9223372036854775808) with opcache, because a no-overflow assumption has been introduced.

I think the right fix here is going to be something along the lines of: If we infer a ref type during type inference, we also need to clear out the range on that variable, and propagate that to all dependent ranges. This is a non-trivial change.
 [2021-06-07 15:01 UTC] nikic@php.net
The following pull request has been associated:

Patch Name: Fix bug #81096: Re-infer ranges if ref type is inferred
On GitHub:  https://github.com/php/php-src/pull/7114
Patch:      https://github.com/php/php-src/pull/7114.patch
 [2021-06-10 08:43 UTC] git@php.net
Automatic comment on behalf of dstogov
Revision: https://github.com/php/php-src/commit/7368d0c4185b4e65dfb342dc7523727b52a79114
Log: Fixed bug #81096: Inconsistent range inferece for variables passed by reference
 [2021-06-10 08:43 UTC] git@php.net
-Status: Analyzed +Status: Closed
 [2021-06-10 08:59 UTC] nikic@php.net
-Assigned To: +Assigned To: nikic
 [2021-06-10 08:59 UTC] nikic@php.net
For release branches, I've applied https://github.com/php/php-src/commit/3f4bc94b0092aa1699be89a88b8a4e62507d1843 as a workaround to address the originally reported issue.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Tue Dec 10 18:01:28 2024 UTC