Skip to content

Commit

Permalink
Fix #79825: opcache.file_cache causes SIGSEGV with custom opcode hand…
Browse files Browse the repository at this point in the history
…lers

Modules may have changed after restart which can cause dangling pointers from custom opcode handlers in the second-level cache files. This fix includes the installed module names and versions in the accel_system_id hash as entropy. Closes GH-5836
  • Loading branch information
SammyK committed Sep 9, 2020
1 parent 5dcb8f2 commit 2d4aa1e
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 0 deletions.
2 changes: 2 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ PHP NEWS
- OPcache:
. Fixed bug #80002 (calc free space for new interned string is wrong).
(t-matsuno)
. Fixed bug #79825 (opcache.file_cache causes SIGSEGV when custom opcode
handlers changed). (SammyK)

- PDO:
. Fixed bug #80027 (Terrible performance using $query->fetch on queries with
Expand Down
16 changes: 16 additions & 0 deletions ext/opcache/ZendAccelerator.c
Original file line number Diff line number Diff line change
Expand Up @@ -2692,6 +2692,9 @@ static void accel_gen_system_id(void)
unsigned char digest[16], c;
char *md5str = ZCG(system_id);
int i;
zend_module_entry *module;
zend_extension *extension;
zend_llist_position pos;

PHP_MD5Init(&context);
PHP_MD5Update(&context, PHP_VERSION, sizeof(PHP_VERSION)-1);
Expand All @@ -2702,6 +2705,19 @@ static void accel_gen_system_id(void)
PHP_MD5Update(&context, __DATE__, sizeof(__DATE__)-1);
PHP_MD5Update(&context, __TIME__, sizeof(__TIME__)-1);
}
/* Modules may have changed after restart which can cause dangling pointers from
* custom opcode handlers in the second-level cache files
*/
ZEND_HASH_FOREACH_PTR(&module_registry, module) {
PHP_MD5Update(&context, module->name, strlen(module->name));
PHP_MD5Update(&context, module->version, strlen(module->version));
} ZEND_HASH_FOREACH_END();
extension = (zend_extension *) zend_llist_get_first_ex(&zend_extensions, &pos);
while (extension) {
PHP_MD5Update(&context, extension->name, strlen(extension->name));
PHP_MD5Update(&context, extension->version, strlen(extension->version));
extension = (zend_extension *) zend_llist_get_next_ex(&zend_extensions, &pos);
}
PHP_MD5Final(digest, &context);
for (i = 0; i < 16; i++) {
c = digest[i] >> 4;
Expand Down

3 comments on commit 2d4aa1e

@nikic
Copy link
Member

@nikic nikic commented on 2d4aa1e Sep 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this causes a bunch of segfaults on Travis: https://travis-ci.org/github/php/php-src/jobs/725704433

@SammyK
Copy link
Contributor Author

@SammyK SammyK commented on 2d4aa1e Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikic 🤦 Oh my god. I can't believe I missed that. I spent two hours making sure every "t" was crossed and "i" dotted since it was very first merge and I still effed it up. I'm really sorry about that! Admittedly I only ran the tests for Zend/tests and ext/opcache/tests for each branch marge, but that was a bad call. Next time I'll absolutely run the full suite (I keep forgetting about the new fancy parallelization!)

It looks like @cmb69 fixed this in 4e198c0. Thank you for that! And again, I'm really sorry for the added hassle. 😞

@cmb69
Copy link
Contributor

@cmb69 cmb69 commented on 2d4aa1e Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, @SammyK! :)

Please sign in to comment.