Fix #79825: [OPcache] Include installed modules in accel_system_id hash

[SammyK]

Fixes bug 79825.

Modules may have changed after restarting the PHP process which can cause dangling pointers from custom opcode handlers in the second-level cache files (opcache.file_cache). This fixes the issue by including the installed module names and versions in the accel_system_id hash.

It would be great to get this fix merged into 7.3 and 7.4 as well.

cc/ @dstogov

[cmb69]

Thanks for the PR, @SammyK! I'd really like to see this fixed, since on Windows it also affects shared memory OPcache, but I'm afraid this solution is depending on the loading of zend_extensions; i.e. any zend_extension loaded after OPcache would be ignored regarding the hash.

[dstogov]

@SammyK I think, this doesn't fix the problem in general, because extensions may override handlers depending on configuration directives, and opcache can't care about behavior of all other extensions. Currently extensions may disable opcode caching (e.g. for debugging). We may also extend API to allow extensions changing the "system_id". I think, this would be a better solution, but for PHP 8+ only.

@cmb69 opcache performs its initialization in post_startup callback, when all other modules should be already loaded and initialized through MINIT.

@SammyK what do you think? can you take care about implementation?

[cmb69]

@dstogov, accel_gen_system_id() is called in accel_startup() even before zend_post_startup_cb is set to accel_post_startup. Anyhow, introducing an API which allows extensions to modify the system ID appears to be a better solution.

[morrisonlevi]

I'd still like to see something go into our active 7.X releases. What do you think about merging this PR into 7.3/7.4 only and pursuing something different for PHP 8? Even if it's not perfect if it's better than what we have that's important, as these are usually sigsegvs.

[SammyK]

@cmb69 Thanks for the feedback. AFAIK the zend_extensions llist is fully generated after the php_ini_register_extensions() call and before zend_startup_extensions() is called. I wasn't able to get a Zend extension to avoid the hash by changing the load order in my manual tests but I might be missing something.

@dstogov Good point. I'm certainly open to creating an API for modifying the system ID during MINIT/startup. I'm guessing we would have to move the accel_gen_system_id call to accel_post_startup to ensure that all the extensions had a chance to add entropy before generating the final ID.

But if we do that, I'm wondering if we can grab entropy directly from any custom opcode handlers that are set at that point. Can you think of other dangling pointers that could occur on op_array's outside of custom opcode handlers? If not, then moving the accel_gen_system_id call to accel_post_startup would allow us to loop though all the zend_user_opcodes and use the opcode + handler address as entropy for accel_system_id. Then we wouldn't need an API for extensions to maintain. If that is an acceptable solution, would that also work as a patch for 7.3+?

@morrisonlevi +1 :)

[morrisonlevi]

I think the reserved slot is likely to have similar issues. Imagine an extension reserves a slot and uses it for something at compile time, then on the next run the extension doesn't reserve a slot (due to environment variable/ini settings). Imagine a second extension that also uses the slot; the slot number may be different between the requests depending on order. If it is different it may read from the other slot with bad assumptions. The newer op_array extension bits are done per-request, so those should be fine, I think. @dstogov, can you confirm?

[dstogov]

I think the reserved slot is likely to have similar issues. Imagine an extension reserves a slot and uses it for something at compile time, then on the next run the extension doesn't reserve a slot...

Right. This is a similar problem that may be handled only by extension itself.

I think, merging this patch into 7.* is not a big problem, but I would prefer to see a better solution for PHP-8 first.

@cmb69 except of overriding opcode handlers, extensions may introduce new USER opcodes, reserve handlers and run-time cache slots. They also may override zend_execute_ex(), zend_execute_internal(), zend
_compile().

Moving accel_gen_system_id to accel_post_startup looks right.

[cmb69]

Moving accel_gen_system_id to accel_post_startup looks right.

ACK.

[morrisonlevi]

I think the reserved slot is likely to have similar issues. Imagine an extension reserves a slot and uses it for something at compile time, then on the next run the extension doesn't reserve a slot...

Right. This is a similar problem that may be handled only by extension itself.

Since you can only request a reserved slot in startup, we should be able to factor the number of reserved slots in use into the hash at least, couldn't we?

[SammyK]

@SammyK what do you think? can you take care about implementation?

Done! :) #5871

[cmb69]

[…] I'm afraid this solution is depending on the loading of zend_extensions; i.e. any zend_extension loaded after OPcache would be ignored regarding the hash.

I've double-checked, and this is apparently not the case.

This seems to be sensible mitigation for the reported issues for PHP 7, particularly for Windows. I'm thinking about multiple CLI processes with different extensions loaded running in parallel, which may share a single OPcache instance.

[SammyK]

Gentle bump to see if we can get this bug fix merged in before the next patch releases go out. :)

[morrisonlevi]

@sgolemon and/or @remicollet: just checking with you two; assuming any merge conflicts are dealt with, am I good to merge this into 7.2+?

[cmb69]

@morrisonlevi, 7.2 is for security fixes only, so this can only go into 7.3+.

[morrisonlevi]

It avoids a segmentation fault; is that not a security fix? But this is why I checked :)

[SammyK]

@cmb69 I just wanted to confirm that a segmentation fault isn't considered a security issue. If not, I'll target this for 7.3+.

[nikic]

@SammyK It is not a security issue.

[SammyK]

I just merged this into 7.3 (via 2d4aa1e) and 7.4 (via 1b52682). I didn't merge into master since that will be addressed with #5871.

Since this was the first time I've merged to php-src, would someone mind double checking that I did the merges properly? I did add Closes GH-5836 to the commit message, but it didn't seem to auto-close this PR. Not sure what I messed up there.

[nikic]

@SammyK It is necessary to always merge up to master -- it can be an empty merge that doesn't change anything, but it needs to be merged. Otherwise the next person will be left wondering what to do with your unmerged changes.

[SammyK]

@nikic Got it, thanks. I just merged to master via 3375374.

[nikic]

@SammyK Thanks! By the way, that's also the reason why the "Closes GH-X" did not work. Those keywords only apply once a change is merged into the master branch.