php / php-src

The PHP Interpreter
https://www.php.net
Other
38.11k stars 7.74k forks source link

JIT: undefined variable $i #15145

Open lifeofguenter opened 2 months ago

lifeofguenter commented 2 months ago

Description

The following code:

<?php
        for ($i = 4; $i > 0; --$i) {
            if ($i > $position) {
                $matches[$i] = $pad;
            } elseif ($i === $position && $increment) {
                $matches[$i] += $increment;
                // If $matches[$i] was 0, carry the decrement
                if ($matches[$i] < 0) {
                    $matches[$i] = $pad;
                    --$position;

                    // Return null on a carry overflow
                    if ($i === 1) {
                        return null;
                    }
                }
            }
        }

Full source: https://github.com/composer/semver/blob/3.4.0/src/VersionParser.php#L541

Resulted in this output:

ErrorException: Uncaught ErrorException (500): Undefined variable $i in /app/vendor/composer/semver/src/VersionParser.php:541

But I expected this output instead:

no error

Full stack trace:

#0 /app/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php(255): Illuminate\Foundation\Bootstrap\HandleExceptions->handleError()
#1 /app/vendor/composer/semver/src/VersionParser.php(541): Illuminate\Foundation\Bootstrap\HandleExceptions->Illuminate\Foundation\Bootstrap\{closure}()
#2 /app/vendor/composer/semver/src/VersionParser.php(410): Composer\Semver\VersionParser->manipulateVersionString()
#3 /app/vendor/composer/semver/src/VersionParser.php(276): Composer\Semver\VersionParser->parseConstraint()
#4 /app/vendor/composer/InstalledVersions.php(122): Composer\Semver\VersionParser->parseConstraints()
#5 /app/vendor/maatwebsite/excel/src/Cache/CacheManager.php(43): Composer\InstalledVersions::satisfies()
#6 /app/vendor/laravel/framework/src/Illuminate/Support/Manager.php(106): Maatwebsite\Excel\Cache\CacheManager->createMemoryDriver()
#7 /app/vendor/laravel/framework/src/Illuminate/Support/Manager.php(80): Illuminate\Support\Manager->createDriver()
#8 /app/vendor/maatwebsite/excel/src/SettingsProvider.php(31): Illuminate\Support\Manager->driver()
#9 /app/vendor/maatwebsite/excel/src/SettingsProvider.php(25): Maatwebsite\Excel\SettingsProvider->configureCellCaching()
#10 /app/vendor/maatwebsite/excel/src/ExcelServiceProvider.php(52): Maatwebsite\Excel\SettingsProvider->provide()
#11 /app/vendor/laravel/framework/src/Illuminate/Foundation/Application.php(1073): Maatwebsite\Excel\ExcelServiceProvider->Maatwebsite\Excel\{closure}()
#12 /app/vendor/laravel/framework/src/Illuminate/Foundation/Application.php(1016): Illuminate\Foundation\Application->fireAppCallbacks()
#13 /app/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/BootProviders.php(17): Illuminate\Foundation\Application->boot()
#14 /app/vendor/laravel/framework/src/Illuminate/Foundation/Application.php(263): Illuminate\Foundation\Bootstrap\BootProviders->bootstrap()
#15 /app/vendor/laravel/framework/src/Illuminate/Foundation/Http/Kernel.php(186): Illuminate\Foundation\Application->bootstrapWith()
#16 /app/vendor/laravel/framework/src/Illuminate/Foundation/Http/Kernel.php(170): Illuminate\Foundation\Http\Kernel->bootstrap()
#17 /app/vendor/laravel/framework/src/Illuminate/Foundation/Http/Kernel.php(144): Illuminate\Foundation\Http\Kernel->sendRequestThroughRouter()
#18 /app/public/index.php(51): Illuminate\Foundation\Http\Kernel->handle()
#19 {main}

Next Symfony\Component\HttpKernel\Exception\HttpException: Undefined variable $i in /app/vendor/laravel/framework/src/Illuminate/Foundation/Exceptions/Handler.php:646
Stack trace:
#0 /app/vendor/laravel/framework/src/Illuminate/Foundation/Exceptions/Handler.php(556): Illuminate\Foundation\Exceptions\Handler->prepareResponse()
#1 /app/vendor/laravel/framework/src/Illuminate/Foundation/Exceptions/Handler.php(473): Illuminate\Foundation\Exceptions\Handler->renderExceptionResponse()
#2 /app/vendor/laravel/framework/src/Illuminate/Foundation/Http/Kernel.php(509): Illuminate\Foundation\Exceptions\Handler->render()
#3 /app/vendor/laravel/framework/src/Illuminate/Foundation/Http/Kernel.php(148): Illuminate\Foundation\Http\Kernel->renderException()
#4 /app/public/index.php(51): Illuminate\Foundation\Http\Kernel->handle()
#5 {main}

It is impossible for this error to happen, but also it happens only intermittently with no re-deployment/code-changes.

I suspect something with opcache, current config:

opcache.memory_consumption = 128
opcache.interned_strings_buffer = 8
opcache.max_accelerated_files = 32531
opcache.validate_timestamps = Off
opcache.jit = "function"
opcache.jit_buffer_size = 128M

It seems to happen more often when vapor-core package is installed - despite not running on vapor.

PHP Version

PHP 8.2.21 + 8.3.9

Operating System

Debian Bookworm

iluuu1994 commented 2 months ago

Thank you for the report. Does the error go away if you disable the JIT?

lifeofguenter commented 2 months ago

switching from

opcache.jit = "function"

to

opcache.jit = "tracing"

solved the issue for us on multiple laravel-based applications

iluuu1994 commented 2 months ago

Thank you. Note that, even with tracing JIT, the problem may still reappear when the relevant code actually gets jitted.

dstogov commented 2 months ago

I can't reproduce the problem with the provided code.

Could you please isolate the problem. It should be enough to capture the argument passed to ``Composer\Semver\VersionParser->manipulateVersionString()" from "Composer\Semver\VersionParser->parseConstraint()" at https://github.com/composer/semver/blob/3.4.0/src/VersionParser.php#L276.

Then the test case should look like:

<?php
function manipulateVersionString(array $matches, $position, $increment = 0, $pad = '0') {
    for ($i = 4; $i > 0; --$i) {
        if ($i > $position) {
            $matches[$i] = $pad;
        } elseif ($i === $position && $increment) {
            $matches[$i] += $increment;
            // If $matches[$i] was 0, carry the decrement
            if ($matches[$i] < 0) {
                $matches[$i] = $pad;
                --$position;
                // Return null on a carry overflow
                if ($i === 1) {
                    return null;
                }
            }
        }
    }
}
manipulateVersionString($CAPTURED_ARGUMENT);
?>

However, there is something wrong in your code. https://github.com/composer/semver/blob/3.4.0/src/VersionParser.php#L276 doesn't pass the required second argument.

lifeofguenter commented 2 months ago

@dstogov

  1. its not my code :P - and the purpose of showing the code was not to make it reproducible but to show that the code is simply impossible to throw an error. Is it really just argument that could throw jit off? Then I can attempt to have a look again.
  2. There are two functions constraint vs constraints. The code is calling a different function.
dstogov commented 1 month ago
  • its not my code :P - and the pupose of showing the code was not to make it reproducible but to show that the code is simply impossible to throw an error. Is it really just argument that could throw jit off? Then I can attempt to have a look again.

In case you see this problem and jit=off - the JIT has a bug. I see that exception shouldn't be thrown at that point, but I can't guess about the consequences that leaded to it. In case I had a reproduction, I most probably fixed this in a day.

opcache.jit="function" must be deterministic and should lead to the problem on each function call with the sane arguments.

  • There are two functions constraint vs constraints. The code is calling a different function.

You are right. I was wrong.

I need arguments passed from https://github.com/composer/semver/blob/3.4.0/src/VersionParser.php#L410 that lead to the wrong exception.