-
Notifications
You must be signed in to change notification settings - Fork 7.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OPCache with Enum and Callback functions results in segmentation fault #10914
Comments
We also found out that ExampleEnum::cases() passed with the function () use (), also causes a segmentation fault. $cases = ExampleEnum::cases();
triggerException(function () use ($cases) {
auxiliarFunction($cases); //This will not run and trigger a SIGSEGV - core dumped
}); We mitigated the issue with $casesWorkAround = ExampleEnum::cases();
$cases = [];
foreach ($casesWorkAround as $caseEntry) {
$cases [] = $caseEntry->value;
} If needed I can create a docker with a symfony project that recreates this issue. |
I've been unable to reproduce this so far. I tested this without Symfony though, just with the two provided files.
This would be very helpful! Alternatively, are you able to provide a backtrace from GDB? |
We are able to recreate this with a 100% crash rate. I expect to have this ready at max in 7 days, will try today but I can't guarantee it |
After some debugging it seems like this:
I wasn't able to recreate a docker instance with it. Gets weirder in every debug, I am able to duplicate it constantly on our codebase but I am not allowed to share it... Might be an issue with Symfony, or the redis, but xdebug dies on the Enum::cases or Enum::Entry->value. PHP Ini: https://gist.github.com/miguelantoniosantos/200ebb03dbc1996cbbc6c7edc8510d18 |
Thanks. It's definitely not a JIT issue because that's disabled. Since you mention this happens with redis, this makes me wonder if it's an issue with that extension instead of PHP itself. |
I am able to do If you need any more info from me, please guide me as my knowledge of gdb is far lost. |
Interesting how it's not a PECL extension causing the issue. |
For more details on the machine we use a AWS c6g.large, with Ubuntu 20.04.5 ARM base image, nothing was compiled from source, all installed from The Redis server is ElastiCache Redis, and we made sure the server had sufficient RAM and CPU to handle the requests. |
I'm not sure if I actually explained the previous instructions properly. |
My first core dump didn't had all the symbols, generated a new one. Core file: <snip, removed coredump file> From my minimal understanding of the Backtrace, seems like the GC is trying to clear something he can't clear |
Bringing another update, in a completely different method using enums inside the callback for the cache, and it works as intended... So this is only happening one some methods... And we can't find a common ground between them. |
It looks like some sort of memory corruption bug. Notes for myself and other developers: |
Disabling preloading also solves the issue. Changed from: |
Okay thank you this is very useful info! So it's related to preloading, which may explain why you can only reproduce it in some methods and some not (depending if the method was preloaded or not). |
If needed we can schedule, during the week, something and you have a look into our codebase, also the preloaded files and setup. |
I tried a bunch of different stuff, but unfortunately didn't manage to reproduce your issue. I don't have enough information at the moment to know what scenario happens. |
This has not been forgotten, I am trying to free up my week schedule to work on this. |
I'm facing this too. Not many things to add besides we are also disabling OPCache preload as a fix (fix found without this issue ^^). Versions :
|
@magnetik are you able to provide self-contained reproduction instructions? I haven't been able to reproduce the issue locally yet unfortunately |
Okay it took a very long time of looking at this on and off, but I managed to build a reproducer. I used these configure flags: Save this as test.php: <?php
require 'vendor/autoload.php';
Predis\Autoloader::register();
$client = new Predis\Client();
// HERE
//$x = new ReflectionEnum(ExampleEnum::class);
//var_dump($x->getCases()[0]->getValue());
var_dump($client->set('xx', serialize(ExampleEnum::cases())));
var_dump($client->get('xx')); Save this as enum.php and set preloading to preload this file:
Now start a CLI server with opcache on, for me it looks like this: Now go to
|
Enabling opcache.protect_memory reveals what's going on I think. The enum, whose first value is of type IS_CONSTANT_AST, is stored in SHM because of the preloading. ReflectionClassConstant::getValue() tries to modify it with this code: if (Z_TYPE(ref->value) == IS_CONSTANT_AST) {
zval_update_constant_ex(&ref->value, ref->ce);
}
ZVAL_COPY_OR_DUP(return_value, &ref->value); This shouldn't be allowed because it's in SHM. A quick test by replacing it with this fixes it: if (Z_TYPE(ref->value) == IS_CONSTANT_AST) {
ZVAL_COPY(return_value, &ref->value);
zval_update_constant_ex(return_value, ref->ce);
} else {
ZVAL_COPY_OR_DUP(return_value, &ref->value);
} Of course there are more places where this change needs to happen. Relevant ASAN complaint:
|
With protect_memory enabled, this is the minimal reproducer, which will crash instantly if enum.php is preloaded: <?php
$x = new ReflectionEnum(ExampleEnum::class);
var_dump($x->getCases()[0]->getValue()); No need to externals dependencies. The external dependencies just created the "lucky" heap layout that made it able to hit the issue without protect_memory. |
@nielsdos I haven't tried it, but I think this line needs to be fixed: php-src/ext/reflection/php_reflection.c Line 6895 in c1d4d95
We're accessing the class constants directly, instead of using Thank you a lot for the analysis! Edit: And it looks like |
@iluuu1994 Yeah I also found that. I just wasn't sure about the overhead but if this is the general approach I'll go with that. |
@nielsdos Class constants should only be evaluated once. If we copy the constant AST and evaluate it then, we'll lose the value and evaluate it again on the next access. I think, for enum cases this would mean getting non-unique case values. So I don't think we have another option. |
Both seemed to work so thanks for your explanation on why we need separation. I'll fix it using the separation. |
Thank you a lot @nielsdos! |
…segmentation fault See linked issue for analysis.
Awesome @nielsdos ! Thanks for the fix. looking forward to have it :) |
* PHP-8.1: Fix GH-10914: OPCache with Enum and Callback functions results in segmentation fault
* PHP-8.2: Fix GH-10914: OPCache with Enum and Callback functions results in segmentation fault
Description
The following code:
ExampleEnum.php
Trigger.php
This was first spotted on Symfony 5.4 codebase, with a callback of the TagAwareCacheInterface.
If OPCache is disabled, the error doesn't occur.
Replacing the enum with a string variable fixes the issue.
PHP Version
PHP 8.1.11 (fpm-fcgi)
Operating System
Ubuntu 20.04.5 LTS
The text was updated successfully, but these errors were encountered: