opcache.consistency_checks > 0 causes segfaults in PHP >= 8.1.5 in fpm context #8065
Comments
|
Do you get any "Checksum failed for" log entries, if you set |
|
|
Thanks! If the checksum comparison fails, the file is compiled again, and the old oparray is not freed – that is by design, to avoid copying shared memory. As such, this doesn't look like a bug, but I wonder why the checksum comparison fails in the first place. Are these files modified during script execution? |
No, they aren't. You can run the same code on PHP 7.4 or 8.0 without "Checksum failed" error 🤷 Same output with php7.4 or 8.0: |
|
So with PHP 8.1.5 this causes random SIGSEGV instead of wasting just memory |
|
Ugh. |
cmb69
changed the title
|
I went down this rabbit hole for a few hours and I have an incomplete explanation. This seems to be related to multiple changes in PHP 8.1. The following fields differ for each new creation of
Also, I noticed that there's likely a mistake in the hash calculation. I think this line should be: checksum = zend_adler32(checksum, mem, persistent_script_check_block_size);As I'll take a closer look soon but wanted to document my findings, even if small. |
|
Nice one. Dunno if it could help, but even with checksums disabled, our prod fpm is segfaulting randomly on deploy when we call opcache_reset() on the pool. Eventually i can catch a core dump. |
Oh, good catch! The code as is makes no sense. |
|
Is there anything where I can be of help here? I do work together with @CyberLine and we also have some reports from other parts of our company where random segfaults do appear after production deployments. From my PoV, the suggestion from @iluuu1994 might be a first step? Any hint on how to create a failing test case for this would be awesome, I'd be happy to contribute as much as I can if that brings us closer to a fix here. |
|
I completely forgot about this one.
I don't think that will solve the issue. If anything, the mistake in the hash calculation would lead to stale cache being used but we're having the opposite problem here. (it should still be fixed of course, or at least removed). I'll have another look at this soon. |
|
@iluuu1994 anything we can help with?
The SIGSEV's are coming after 128GB of ram, went full. |
|
I'm looking more into it right now. |
|
@zsuraski Thank you for your assessment!
Note that this is different from the things that were already mutable because those things were all at the end of If you take a look at #9377, the Another option is to, instead of just iterating over Of course, if we can find a simple way to fix this it's preferable to keep the consistency checks. Maybe there's a simple solution that I'm missing.
In this case the documentation is wrong, which states this should not be enabled in production. https://www.php.net/manual/en/opcache.configuration.php#ini.opcache.consistency-checks |
|
Thanks @iluuu1994!
To be perfectly honest - I remembered an entirely different implementation of the checksum code, that accounts for things that can change within op_arrays (most notably refcounts on zvals). But either my memory is wrong, or this somehow evolved into not being necessary - as early as when we open source opcache.
It may be that since the (apparent) change to the way checksums are calculated - mprotect is as good as consistency_checks. But I think we need to get to the bottom of this before moving forward:
I'll wait for Dmitry to weigh in - maybe he remembers more about the evolution of consistency_checks, and he's certainly a lot more into the current codebase than I am... |
Provided we can fix the checksum calculation, I think we should change how errors are handled. Currently, only an info is logged (which are not enabled by default), so these errors are easily missed. From an info it's also unclear to the user whether that is even an issue. Scripts are also recompiled with every checksum mismatch which will fill shared memory very quickly if it happens after every request, and thus lead to a complete restart of opcache very often. I think that does more harm than good. It might make more sense to just log the error, keep using the old script and ask the user file a bug. If the safety net leads to filling of memory and crashing servers people will likely not use it 🙂
I checked and it looks like the original implementation is very similar to what we have now: Lines 818 to 839 in 528006a The zend_persistent_script.dynamic_members are explicitly filtered out, the rest is used for the checksum calculation. If I remember correctly classes were always copied from shared memory into process memory at the time specifically to avoid modifications to shared memory, so this is not something that should've occurred back then. So, at least from how I understand it, the
👍 Since this issue can be avoided by disabling |
What we're currently seeing is a result of the checksum calculation being inherently broken. Behind that feature, there's a hidden assumption that generally speaking - neither that feature, nor some other glaring part in PHP - are fundamentally broken, and repeatedly result in a varying checksum on every request - which would in turn fill up the shared memory and do a lot more harm than good. If we put these scenarios aside (which apparently aren't that common) - this feature is aimed at providing a safety net against unexpected, not necessarily recurring bugs in PHP or underlying library that happen in certain edge cases. E.g., IIRC in the past (20 years ago or so), there was a bug in the OCI8 library that overwrote parts of opcodes in some rare situations. The problem is that without that feature - it would effectively mean the server would start crashing on every access to the corrupted file (not just in case of the rare extreme bug, but any subsequent request following it) - without any realistic prospect for recovery other than a server restart. Over the years there have been issues that were detected by this feature, and I can't recall (not that it means that much 🙂) situations similar to the one we're currently experiencing, when corruption was so prevalent that this feature was effectively useless.
It's old, but it's certainly not the original implementation... The original was written back in 1999-2000. So a lot has passed between then and when we open sourced it in 2013. That said, it's quite possible that I'm just not remembering correctly (I remember a much more complex checksum calculation implementation). Unfortunately I no longer have access to the original CVS (!) code that hosted these archeological versions...
If there's no way around this then I guess disabling consistency_checks when jit is enabled could be a way to go... But, still need to understand whether we need both consistency_checks and protect_memory. |
|
This is definitely a bug that was introduced with tracing (and adoptive) JIT and then with inheritance cache. I think the easiest way to fix the problem (without binary compatibility breaks) is maintaining of a skip list (just a sorted plain array of offsets from @iluuu1994 would you like to try implement this solution? |
|
@dstogov Sure, I can give it a try. This should work well for |
|
I think, we will need to skip only the handlers overridden by |
|
I would construct that skip list, by adding pointers directly from |
|
@dstogov Thanks for the suggestion! I'll give that a try soon. |
|
@dstogov This seems to also be necessary for master...iluuu1994:php-src:checksum-skip-list It works well for the |
|
@iluuu1994 I made only a quick review...
It would be great to avoid CFG recalculation. For tracing JIT you may call Binary compatibility shouldn't be a problem, because |
Fixes phpGH-8065 (see also for analysis) The class inheritance cache must be excluded from the checksum computation because it may change (legally) in between the computation and recomputation. Furthermore the handler pointer in the zend_op structure may be modified by the JIT, and the op_array->reserved[] could be changed as well. This approach is based on iluuu1994's original approach, with some slight changes. First let's explain the main idea. The idea is to keep a "skip list" of offsets to skip in the checksum computation. This will include the offsets described above (class inheritance cache, handler, reserved). These skips are of size pointer_size (equals to the native pointer size). It differs in the sense that this is a pragmatic approach. It was found that in the original approach it was very difficult to figure out what can be exactly changed when, and let to redundant computation of the CFG for example. Furthermore, such a process is also error-prone and complex to maintain. Instead of that, we just always skip the handler pointer and the reserved area. The positive is that it leads to code which is easier to understand and maintain. The negative is that we won't catch corruptions in those memory areas. However, I believe this is acceptable because of the following reasons: * We'd have to be very unlucky to *only* have corruptions in *those* areas. Usually, corruptions are not that well-targetted. * A checksum isn't a fully error-proof thing either, it's very possible that there is a corruption in other areas that the checksum doesn't capture. Let's now talk about the implementation. Keeping all those entries in a skip list wouldn't be efficient: we'd need an entry for every opcode, which would lead to many many entries. However, this approach actually implements a "compression" scheme. Instead of just storing the offset, we actually store a triple: <offset, size of the area to be checked, amount of repetitions>. The offset indicates which pointer needs to be skipped. If the number of repetitions is greater than zero, it will do the following `repetitions` times: * Checksum the bytes at [offset + pointer_size, offset + pointer_size + size of area to be checked] * Move the offset to after the area that was just checksummed + pointer_size. This allows us to only use one skip list entry for all the opcodes in an op_array. The offsets also aren't checked in the zend_adler32() function itself, the zend_accel_script_checksum() function will checksum in "blocks" of bytes that should not be skipped, by setting the size for zend_alder32() in such a way that it computes the checksum until the next offset to skip. The main advantage of this is better performance, since there are fewer checks, and the accelerated SSE2 (or future other accelerated) routines can keep being used. Finally some other minor differences: * We precompute the exact size needed for the skip list. This avoids reallocations and also fixes one issue where there was a warning about the computed size not being equals to the actual size.
…P >= 8.1.5 in fpm context Fixes phpGH-8065 (see also for analysis) The class inheritance cache must be excluded from the checksum computation because it may change (legally) in between the computation and recomputation. Furthermore the handler pointer in the zend_op structure may be modified by the JIT, and the op_array->reserved[] could be changed as well. This approach is based on iluuu1994's original approach, with some slight changes. First let's explain the main idea. The idea is to keep a "skip list" of offsets to skip in the checksum computation. This will include the offsets described above (class inheritance cache, handler, reserved). These skips are of size pointer_size (equals to the native pointer size). It differs in the sense that this is a pragmatic approach. It was found that in the original approach it was very difficult to figure out what can be exactly changed when, and let to redundant computation of the CFG for example. Furthermore, such a process is also error-prone and complex to maintain. Instead of that, we just always skip the handler pointer and the reserved area. The positive is that it leads to code which is easier to understand and maintain. The negative is that we won't catch corruptions in those memory areas. However, I believe this is acceptable because of the following reasons: * We'd have to be very unlucky to *only* have corruptions in *those* areas. Usually, corruptions are not that well-targetted. * A checksum isn't a fully error-proof thing either, it's very possible that there is a corruption in other areas that the checksum doesn't capture. Let's now talk about the implementation. Keeping all those entries in a skip list wouldn't be efficient: we'd need an entry for every opcode, which would lead to many many entries. However, this approach actually implements a "compression" scheme. Instead of just storing the offset, we actually store a triple: <offset, size of the area to be checked, amount of repetitions>. The offset indicates which pointer needs to be skipped. If the number of repetitions is greater than zero, it will do the following `repetitions` times: * Checksum the bytes at [offset + pointer_size, offset + pointer_size + size of area to be checked] * Move the offset to after the area that was just checksummed + pointer_size. This allows us to only use one skip list entry for all the opcodes in an op_array. The offsets also aren't checked in the zend_adler32() function itself, the zend_accel_script_checksum() function will checksum in "blocks" of bytes that should not be skipped, by setting the size for zend_alder32() in such a way that it computes the checksum until the next offset to skip. The main advantage of this is better performance, since there are fewer checks, and the accelerated SSE2 (or future other accelerated) routines can keep being used. Finally some other minor differences: * We precompute the exact size needed for the skip list. This avoids reallocations and also fixes one issue where there was a warning about the computed size not being equals to the actual size.
…ts in PHP >= 8.1.5 in fpm context" This reverts commit 9353452.
…P >= 8.1.5 in fpm context Fixes phpGH-8065 (see also for analysis) The class inheritance cache must be excluded from the checksum computation because it may change (legally) in between the computation and recomputation. Furthermore the handler pointer in the zend_op structure may be modified by the JIT, and the op_array->reserved[] could be changed as well. This approach is based on iluuu1994's original approach, with some slight changes. First let's explain the main idea. The idea is to keep a "skip list" of offsets to skip in the checksum computation. This will include the offsets described above (class inheritance cache, handler, reserved). These skips are of size pointer_size (equals to the native pointer size). It differs in the sense that this is a pragmatic approach. It was found that in the original approach it was very difficult to figure out what can be exactly changed when, and let to redundant computation of the CFG for example. Furthermore, such a process is also error-prone and complex to maintain. Instead of that, we just always skip the handler pointer and the reserved area. The positive is that it leads to code which is easier to understand and maintain. The negative is that we won't catch corruptions in those memory areas. However, I believe this is acceptable because of the following reasons: * We'd have to be very unlucky to *only* have corruptions in *those* areas. Usually, corruptions are not that well-targetted. * A checksum isn't a fully error-proof thing either, it's very possible that there is a corruption in other areas that the checksum doesn't capture. Let's now talk about the implementation. Keeping all those entries in a skip list wouldn't be efficient: we'd need an entry for every opcode, which would lead to many many entries. However, this approach actually implements a "compression" scheme. Instead of just storing the offset, we actually store a triple: <offset, size of the area to be checked, amount of repetitions>. The offset indicates which pointer needs to be skipped. If the number of repetitions is greater than zero, it will do the following `repetitions` times: * Checksum the bytes at [offset + pointer_size, offset + pointer_size + size of area to be checked] * Move the offset to after the area that was just checksummed + pointer_size. This allows us to only use one skip list entry for all the opcodes in an op_array. The offsets also aren't checked in the zend_adler32() function itself, the zend_accel_script_checksum() function will checksum in "blocks" of bytes that should not be skipped, by setting the size for zend_alder32() in such a way that it computes the checksum until the next offset to skip. The main advantage of this is better performance, since there are fewer checks, and the accelerated SSE2 (or future other accelerated) routines can keep being used. Finally some other minor differences: * We precompute the exact size needed for the skip list. This avoids reallocations and also fixes one issue where there was a warning about the computed size not being equals to the actual size.
…P >= 8.1.5 in fpm context Fixes phpGH-8065 (see also for analysis) The class inheritance cache must be excluded from the checksum computation because it may change (legally) in between the computation and recomputation. Furthermore the handler pointer in the zend_op structure may be modified by the JIT, and the op_array->reserved[] could be changed as well. This approach is based on iluuu1994's original approach, with some slight changes. First let's explain the main idea. The idea is to keep a "skip list" of offsets to skip in the checksum computation. This will include the offsets described above (class inheritance cache, handler, reserved). These skips are of size pointer_size (equals to the native pointer size). It differs in the sense that this is a pragmatic approach. It was found that in the original approach it was very difficult to figure out what can be exactly changed when, and let to redundant computation of the CFG for example. Furthermore, such a process is also error-prone and complex to maintain. Instead of that, we just always skip the handler pointer and the reserved area. The positive is that it leads to code which is easier to understand and maintain. The negative is that we won't catch corruptions in those memory areas. However, I believe this is acceptable because of the following reasons: * We'd have to be very unlucky to *only* have corruptions in *those* areas. Usually, corruptions are not that well-targetted. * A checksum isn't a fully error-proof thing either, it's very possible that there is a corruption in other areas that the checksum doesn't capture. Let's now talk about the implementation. Keeping all those entries in a skip list wouldn't be efficient: we'd need an entry for every opcode, which would lead to many many entries. However, this approach actually implements a "compression" scheme. Instead of just storing the offset, we actually store a triple: <offset, size of the area to be checked, amount of repetitions>. The offset indicates which pointer needs to be skipped. If the number of repetitions is greater than zero, it will do the following `repetitions` times: * Checksum the bytes at [offset + pointer_size, offset + pointer_size + size of area to be checked] * Move the offset to after the area that was just checksummed + pointer_size. This allows us to only use one skip list entry for all the opcodes in an op_array. The offsets also aren't checked in the zend_adler32() function itself, the zend_accel_script_checksum() function will checksum in "blocks" of bytes that should not be skipped, by setting the size for zend_alder32() in such a way that it computes the checksum until the next offset to skip. The main advantage of this is better performance, since there are fewer checks, and the accelerated SSE2 (or future other accelerated) routines can keep being used. Finally some other minor differences: * We precompute the exact size needed for the skip list. This avoids reallocations and also fixes one issue where there was a warning about the computed size not being equals to the actual size.
…P >= 8.1.5 in fpm context Fixes phpGH-8065 (see also for analysis) The class inheritance cache must be excluded from the checksum computation because it may change (legally) in between the computation and recomputation. Furthermore the handler pointer in the zend_op structure may be modified by the JIT, and the op_array->reserved[] could be changed as well. This approach is based on iluuu1994's original approach, with some slight changes. First let's explain the main idea. The idea is to keep a "skip list" of offsets to skip in the checksum computation. This will include the offsets described above (class inheritance cache, handler, reserved). These skips are of size pointer_size (equals to the native pointer size). It differs in the sense that this is a pragmatic approach. It was found that in the original approach it was very difficult to figure out what can be exactly changed when, and let to redundant computation of the CFG for example. Furthermore, such a process is also error-prone and complex to maintain. Instead of that, we just always skip the handler pointer and the reserved area. The positive is that it leads to code which is easier to understand and maintain. The negative is that we won't catch corruptions in those memory areas. However, I believe this is acceptable because of the following reasons: * We'd have to be very unlucky to *only* have corruptions in *those* areas. Usually, corruptions are not that well-targetted. * A checksum isn't a fully error-proof thing either, it's very possible that there is a corruption in other areas that the checksum doesn't capture. Let's now talk about the implementation. Keeping all those entries in a skip list wouldn't be efficient: we'd need an entry for every opcode, which would lead to many many entries. However, this approach actually implements a "compression" scheme. Instead of just storing the offset, we actually store a triple: <offset, size of the area to be checked, amount of repetitions>. The offset indicates which pointer needs to be skipped. If the number of repetitions is greater than zero, it will do the following `repetitions` times: * Checksum the bytes at [offset + pointer_size, offset + pointer_size + size of area to be checked] * Move the offset to after the area that was just checksummed + pointer_size. This allows us to only use one skip list entry for all the opcodes in an op_array. The offsets also aren't checked in the zend_adler32() function itself, the zend_accel_script_checksum() function will checksum in "blocks" of bytes that should not be skipped, by setting the size for zend_alder32() in such a way that it computes the checksum until the next offset to skip. The main advantage of this is better performance, since there are fewer checks, and the accelerated SSE2 (or future other accelerated) routines can keep being used. Finally some other minor differences: * We precompute the exact size needed for the skip list. This avoids reallocations and also fixes one issue where there was a warning about the computed size not being equals to the actual size.
This feature does not work right now and leads to memory leaks and other problems. For analysis and discussion see phpGH-8065. In phpGH-10624 it was decided to disable the feature to prevent problems for end users. If end users which to get some consistency guarantees, they can rely on opcache.protect_memory.
This feature does not work right now and leads to memory leaks and other problems. For analysis and discussion see phpGH-8065. In phpGH-10624 it was decided to disable the feature to prevent problems for end users. If end users which to get some consistency guarantees, they can rely on opcache.protect_memory.
and others
Description
After upgrading some projects to PHP 8.1.0 and 8.1.2 we could see requests getting slower and slower over time. Restart of the correspondig fpm-pool fixed that for short. After digging in the problem we could isolate and reproduce it:
composer require laminas/laminas-servicemanagerfor i in {1..10000}; do curl localhost/test.php; done;You would see an increment usage of memory that will not getting freed after all. Set opcache.consistency_checks to 0 to check that it will not keep any used memory.
PHP Version
8.1.0
Operating System
Debian 10/11
The text was updated successfully, but these errors were encountered: