consistency_checks definitely used to be a production-quality feature, of course that came with a penalty of a certain performance hit - but running a checksum every now and again is often not a huge price to pay for the safety net it used to provide. Can it go back into being that?
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 🙂
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.
but it doesn't explain why until recently, consistency_checks worked fine while doing a whole ->mem checksum.
I checked and it looks like the original implementation is very similar to what we have now:
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...
So, at least from how I understand it, the
memblock (apart from the
zend_persistent_script.dynamic_members) was indeed completely immutable. This only changed with the JIT (modifying opcode handlers) and inheritance cache (modifying
zend_class_entry.inheritance_cache). The reason this was not noticed is likely because we don't have a single test that enables
consistency_checksmanually, nor do we test it in the opcache variation tests on nightly. This is definitely something we should do if we can sort things out.
I'll wait for Dmitry to weigh in
👍 Since this issue can be avoided by disabling
consistency_checksfor now there's no rush.
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.
I want to point out that generally we compute the checksum by looking into the persistent_script structure, with everything we know about its semantics - factoring in our knowledge on which parts are immutable - while ignoring the parts that aren’t.
Note that this is different from the things that were already mutable because those things were all at the end of
zend_persistent_scriptand thus easily skippable during hash calculation. The things that break the calculation right now are in the middle of the
memblock, as they are changes in classes or op_arrays.
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. Either way - what I remembered - diving into the structures inside ->mem in order to calculate the checksums, clearly doesn't happen in the current codebase.
If you take a look at #9377, the
inheritance_cachecase is relatively easy to fix because the number of classes in a script are usually limited, so we can just loop over them and temporarily reset them. However, for JIT it's not as trivial because the JIT replaces various opcode handlers and managing this would just be increasingly complex.
Another option is to, instead of just iterating over
mem, traverse the
zend_persistent_scriptmanually but this is also complex and will need to be updated every time something changes in the data structures which seems like something that can be forgotten easily (apart from also being much slower).
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.
his is a production-oriented feature
In this case the documentation is wrong, which states this should not be enabled in production.
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...
Now, the problem is that the memory block above is not actually immutable. There are now a few things that can change, and it's very hard to "filter" those things when generating the checksum as it's essentially just a pointer into memory with a given size. But at this point I'm not sure this is even worth fixing.
We have other mechanisms in place to make sure that the shared memory doesn't get modified, namely
mprotectto guarantee it doesn't get modified (for the time it's protected).
I want to point out that generally we compute the checksum by looking into the persistent_script structure, with everything we know about its semantics - factoring in our knowledge on which parts are immutable - while ignoring the parts that aren’t. It’s not just a pointer to a piece of memory, and he checksum should perfectly reflect the immutable parts of the structure and it’s underlying structures.
At his is definitely worth fixing. Unlike opcache.protect_memory - this is a production-oriented feature, a safety net against situations where bugs in PHP - or more likely, an underlying library - could corrupt and crash not just a single request - but potentially bring down the entire server (if such corruption occurs in a common file, for example).
Of course, right now it seems to be useless because it’s implementation is broken - resulting in false corruption detections. But the feature is just as valid and important as the day it was introduced - as long as we can get it back in order. Most probably, new or modified parts in the persistent_script structure weren’t properly reflected in the checksum code. Once that’s fixed - everything would be back in order. There’s also a worse possibility, that a part that is supposed to be immutable is actually updated - and if that’s the case we definitely want to figure out what it is and fix it…
I pinged @dstogov to see if he can figure out what went wrong in there, hopefully he can take a look when he’s back.
I believe that the fact that the first hit is checksum-checked (if consistency checks are enable) is actually intentional. After all, it's the first time we're utilizing the cached version of the file, and if it's DOA, we should know about it - as opposed to waiting N requests to find out.
I agree with @cmb69 that the behavior is correct, but the correct solution comprises of doing both:
Seems like the consistency checks code is broken, and parts of the persistent_script that aren't immutable are getting factored into the checksum, which they shouldn't. Depending on what exactly is the root causes here, it might imply a bigger issue too - it could be that not only the checksum code is broken - but that even with consistency checks disabled, there might be some sort of race condition that could result in a segfault (not very likely but impossible to rule out until we figure out the root cause).