Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Multiple classes using same trait causes function JIT crash #17654

Closed
YuriWPY opened this issue Jan 31, 2025 · 2 comments
Closed

Multiple classes using same trait causes function JIT crash #17654

YuriWPY opened this issue Jan 31, 2025 · 2 comments

Comments

@YuriWPY
Copy link

YuriWPY commented Jan 31, 2025

Description

I'm a Laravel dev, but it seems the bug I discovered today is likely an OPcache JIT bug.

You can read this thread to understand what I mean.

In short words, we have a GitHub Actions server that runs PHPUnit tests. At some point, one of tests failed with a strange error:

1) Tests\MyTest::testCarbon
TypeError: Carbon\CarbonImmutable::rawAddUnit(): Argument #1 ($date) must be of type Carbon\CarbonImmutable, Carbon\CarbonImmutable given, called in /PATH_TO_THE_PROJECT/vendor/nesbot/carbon/src/Carbon/Traits/Units.php on line 356

/PATH_TO_THE_PROJECT/vendor/nesbot/carbon/src/Carbon/Traits/Units.php:455
/PATH_TO_THE_PROJECT/vendor/nesbot/carbon/src/Carbon/Traits/Units.php:356
/PATH_TO_THE_PROJECT/vendor/nesbot/carbon/src/Carbon/Traits/Units.php:379
/PATH_TO_THE_PROJECT/vendor/nesbot/carbon/src/Carbon/Traits/Date.php:2903
/PATH_TO_THE_PROJECT/vendor/nesbot/carbon/src/Carbon/Traits/Date.php:2594
/PATH_TO_THE_PROJECT/tests/MyTest.php:12

The TypeError error message is generated by the core PHP logic and doesn't make a lot of sense. Also, it was not possible to reproduce this error locally and it looked server-specific.

I spent some time and isolated the following framework code that caused the crash:

for ($i = 0; $i < 1000; $i++)
{
	now()->subHour();
	now()->addMonth();
}

CarbonImmutable::now()->subDays(4);

I also noticed this error is not triggered when OPcache is disabled. I compared OPcache settings between servers and noticed that GitHub Action server uses the following CRTO for some reason: 1235. As soon as I switched to "tracing", the error never showed up again.

I run some additional tests and set opcache.jit in different modes. Here's what I found:

CRTO=1201 segfault
CRTO=1211 segfault
CRTO=1221 OK
CRTO=1231 segfault
CRTO=1241 segfault
CRTO=1251 segfault
CRTO=1221 OK
CRTO=1222 OK
CRTO=1232 OK
CRTO=1242 OK
CRTO=1252 OK
CRTO=1213 OK
CRTO=1223 OK
CRTO=1233 OK
CRTO=1243 OK
CRTO=1253 OK
CRTO=1214 TypeError: Carbon\CarbonImmutable
CRTO=1224 OK
CRTO=1234 TypeError: Carbon\CarbonImmutable
CRTO=1244 OK
CRTO=1254 OK
CRTO=1215 TypeError: Carbon\CarbonImmutable
CRTO=1225 OK
CRTO=1235 TypeError: Carbon\CarbonImmutable
CRTO=1245 OK
CRTO=1255 OK

As you can see, modes XX14, XX34, XX15 and XX35 fire errors.

So far this bug looks like OPcache bug, that's why I've reported it here.

Here's our GitHub server info just in case:

OS:          Ubuntu 24.04.1 LTS.
OS image:    Image: ubuntu-24.04
OS version:  20250126.1.0
Laravel:     laravel/framework (v11.31.0)
Carbon:      nesbot/carbon (3.8.4)
PHP:         8.3 (we use shivammathur/setup-php@v2)
Zend Engine: 4.3.16
OPcache:     8.3.16

PHP Version

PHP 8.3

Operating System

Ubuntu 24.04.1

@nielsdos
Copy link
Member

nielsdos commented Jan 31, 2025

For future reference, there appears to be a reproducer here: laravel/framework#51360 (comment)
I can reproduce this without phpunit (in 8.3, 8.4, and master):

<?php
require __DIR__.'/Carbon-3.8.4/autoload.php';

use Carbon\Carbon;
use Carbon\CarbonImmutable;

function testCarbon()
{
	for ($i = 0; $i < 1000; $i++) {
		Carbon::now()->subHour();
		Carbon::now()->addMonth();
	}

	CarbonImmutable::now()->subDays(4);
}

testCarbon();

@nielsdos
Copy link
Member

nielsdos commented Jan 31, 2025

I reduced it to no longer depend on Carbon. You need to have two different classes using the same trait (with self) to trigger this:

<?php
trait TestTrait {
	public function addUnit(string $x) {
		self::addRawUnit($this, $x);
	}

	public function addRawUnit(self $data, string $x) {
		var_dump($x);
	}
}

class Test {
	use TestTrait;
}

class Test2 {
	use TestTrait;
}

function main()
{
	(new Test2)->addUnit("bar");
	(new Test)->addUnit("foo");
}

main();

EDIT: the problem is that it uses the same cache slot address between the 2 calls. So presumably the runtime cache pointer is wrong?
EDIT 2: the check here seems overly strict: 3e6919c

nielsdos added a commit to nielsdos/php-src that referenced this issue Jan 31, 2025
…5 Crash The Application

This test has two classes that use the same trait. In function JIT mode
the same cache slot will be used. This causes problems because it is
primed for the first class and then reused for the second class,
resulting in an incorrect type check failure.

The current check for a megamorphic trait call requires current_frame to
not be NULL, but this is only set in tracing mode and not in function
mode.

This patch corrects the check.
nielsdos added a commit to nielsdos/php-src that referenced this issue Jan 31, 2025
…5 Crash The Application

This test has two classes that use the same trait. In function JIT mode
the same cache slot will be used. This causes problems because it is
primed for the first class and then reused for the second class,
resulting in an incorrect type check failure.

The current check for a megamorphic trait call requires current_frame to
not be NULL, but this is only set in tracing mode and not in function
mode.

This patch corrects the check.
@nielsdos nielsdos changed the title JIT OPcache with CRTO Modes XX14, XX34, XX15 and XX35 Crash The Application Multiple classes using same trait causes function JIT crash Jan 31, 2025
nielsdos added a commit that referenced this issue Feb 3, 2025
* PHP-8.3:
  Fix GH-17654: Multiple classes using same trait causes function JIT crash
nielsdos added a commit that referenced this issue Feb 3, 2025
* PHP-8.4:
  Fix GH-17654: Multiple classes using same trait causes function JIT crash
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants