From fb242f41baa08087723939e8fdc68fa034edb8d9 Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Sat, 3 Sep 2022 11:23:10 +0200 Subject: [PATCH 1/3] Fix high opcache.interned_strings_buffer causing shm corruption (#9260) --- ext/opcache/ZendAccelerator.c | 11 ++++++++--- ext/opcache/tests/gh9259_001.phpt | 18 ++++++++++++++++++ ext/opcache/tests/gh9259_002.phpt | 18 ++++++++++++++++++ ext/opcache/tests/gh9259_003.phpt | 15 +++++++++++++++ ext/opcache/zend_accelerator_module.c | 22 +++++++++++++++++++++- ext/opcache/zend_shared_alloc.c | 6 +++++- 6 files changed, 85 insertions(+), 5 deletions(-) create mode 100644 ext/opcache/tests/gh9259_001.phpt create mode 100644 ext/opcache/tests/gh9259_002.phpt create mode 100644 ext/opcache/tests/gh9259_003.phpt diff --git a/ext/opcache/ZendAccelerator.c b/ext/opcache/ZendAccelerator.c index fd2792028b1..a74ad34ab8e 100644 --- a/ext/opcache/ZendAccelerator.c +++ b/ext/opcache/ZendAccelerator.c @@ -2840,18 +2840,23 @@ static inline int accel_find_sapi(void) static int zend_accel_init_shm(void) { int i; + size_t accel_shared_globals_size; zend_shared_alloc_lock(); if (ZCG(accel_directives).interned_strings_buffer) { - accel_shared_globals = zend_shared_alloc((ZCG(accel_directives).interned_strings_buffer * 1024 * 1024)); + accel_shared_globals_size = ZCG(accel_directives).interned_strings_buffer * 1024 * 1024; } else { /* Make sure there is always at least one interned string hash slot, * so the table can be queried unconditionally. */ - accel_shared_globals = zend_shared_alloc(sizeof(zend_accel_shared_globals) + sizeof(uint32_t)); + accel_shared_globals_size = sizeof(zend_accel_shared_globals) + sizeof(uint32_t); } + + accel_shared_globals = zend_shared_alloc(accel_shared_globals_size); if (!accel_shared_globals) { - zend_accel_error_noreturn(ACCEL_LOG_FATAL, "Insufficient shared memory!"); + zend_accel_error_noreturn(ACCEL_LOG_FATAL, + "Insufficient shared memory for interned strings buffer! (tried to allocate %zu bytes)", + accel_shared_globals_size); zend_shared_alloc_unlock(); return FAILURE; } diff --git a/ext/opcache/tests/gh9259_001.phpt b/ext/opcache/tests/gh9259_001.phpt new file mode 100644 index 00000000000..bcc0f113c57 --- /dev/null +++ b/ext/opcache/tests/gh9259_001.phpt @@ -0,0 +1,18 @@ +--TEST-- +Bug GH-9259 001 (Setting opcache.interned_strings_buffer to a very high value leads to corruption of shm) +--EXTENSIONS-- +opcache +--INI-- +opcache.interned_strings_buffer=131072 +opcache.log_verbosity_level=2 +opcache.enable_cli=1 +--FILE-- + +--EXPECTF-- +%sWarning opcache.interned_strings_buffer must be less than or equal to 4095, 131072 given%s + +OK diff --git a/ext/opcache/tests/gh9259_002.phpt b/ext/opcache/tests/gh9259_002.phpt new file mode 100644 index 00000000000..8b74949b494 --- /dev/null +++ b/ext/opcache/tests/gh9259_002.phpt @@ -0,0 +1,18 @@ +--TEST-- +Bug GH-9259 002 (Setting opcache.interned_strings_buffer to a very high value leads to corruption of shm) +--EXTENSIONS-- +opcache +--INI-- +opcache.interned_strings_buffer=-1 +opcache.log_verbosity_level=2 +opcache.enable_cli=1 +--FILE-- + +--EXPECTF-- +%sWarning opcache.interned_strings_buffer must be greater than or equal to 0, -1 given%s + +OK diff --git a/ext/opcache/tests/gh9259_003.phpt b/ext/opcache/tests/gh9259_003.phpt new file mode 100644 index 00000000000..91bfcad917b --- /dev/null +++ b/ext/opcache/tests/gh9259_003.phpt @@ -0,0 +1,15 @@ +--TEST-- +Bug GH-9259 003 (Setting opcache.interned_strings_buffer to a very high value leads to corruption of shm) +--EXTENSIONS-- +opcache +--INI-- +opcache.interned_strings_buffer=500 +opcache.enable_cli=1 +--FILE-- + +--EXPECTF-- +%sFatal Error Insufficient shared memory for interned strings buffer! (tried to allocate %d bytes) diff --git a/ext/opcache/zend_accelerator_module.c b/ext/opcache/zend_accelerator_module.c index 6a5eb2b69b3..270ceda7c62 100644 --- a/ext/opcache/zend_accelerator_module.c +++ b/ext/opcache/zend_accelerator_module.c @@ -40,6 +40,7 @@ #define STRING_NOT_NULL(s) (NULL == (s)?"":s) #define MIN_ACCEL_FILES 200 #define MAX_ACCEL_FILES 1000000 +#define MAX_INTERNED_STRINGS_BUFFER_SIZE ((zend_long)((UINT32_MAX-PLATFORM_ALIGNMENT)/(1024*1024))) #define TOKENTOSTR(X) #X static zif_handler orig_file_exists = NULL; @@ -78,6 +79,25 @@ static ZEND_INI_MH(OnUpdateMemoryConsumption) return SUCCESS; } +static ZEND_INI_MH(OnUpdateInternedStringsBuffer) +{ + zend_long *p = (zend_long *) ZEND_INI_GET_ADDR(); + zend_long size = zend_ini_parse_quantity_warn(new_value, entry->name); + + if (size < 0) { + zend_accel_error(ACCEL_LOG_WARNING, "opcache.interned_strings_buffer must be greater than or equal to 0, " ZEND_LONG_FMT " given.\n", size); + return FAILURE; + } + if (size > MAX_INTERNED_STRINGS_BUFFER_SIZE) { + zend_accel_error(ACCEL_LOG_WARNING, "opcache.interned_strings_buffer must be less than or equal to " ZEND_LONG_FMT ", " ZEND_LONG_FMT " given.\n", MAX_INTERNED_STRINGS_BUFFER_SIZE, size); + return FAILURE; + } + + *p = size; + + return SUCCESS; +} + static ZEND_INI_MH(OnUpdateMaxAcceleratedFiles) { zend_long *p = (zend_long *) ZEND_INI_GET_ADDR(); @@ -239,7 +259,7 @@ ZEND_INI_BEGIN() STD_PHP_INI_ENTRY("opcache.log_verbosity_level" , "1" , PHP_INI_SYSTEM, OnUpdateLong, accel_directives.log_verbosity_level, zend_accel_globals, accel_globals) STD_PHP_INI_ENTRY("opcache.memory_consumption" , "128" , PHP_INI_SYSTEM, OnUpdateMemoryConsumption, accel_directives.memory_consumption, zend_accel_globals, accel_globals) - STD_PHP_INI_ENTRY("opcache.interned_strings_buffer", "8" , PHP_INI_SYSTEM, OnUpdateLong, accel_directives.interned_strings_buffer, zend_accel_globals, accel_globals) + STD_PHP_INI_ENTRY("opcache.interned_strings_buffer", "8" , PHP_INI_SYSTEM, OnUpdateInternedStringsBuffer, accel_directives.interned_strings_buffer, zend_accel_globals, accel_globals) STD_PHP_INI_ENTRY("opcache.max_accelerated_files" , "10000", PHP_INI_SYSTEM, OnUpdateMaxAcceleratedFiles, accel_directives.max_accelerated_files, zend_accel_globals, accel_globals) STD_PHP_INI_ENTRY("opcache.max_wasted_percentage" , "5" , PHP_INI_SYSTEM, OnUpdateMaxWastedPercentage, accel_directives.max_wasted_percentage, zend_accel_globals, accel_globals) STD_PHP_INI_ENTRY("opcache.consistency_checks" , "0" , PHP_INI_ALL , OnUpdateLong, accel_directives.consistency_checks, zend_accel_globals, accel_globals) diff --git a/ext/opcache/zend_shared_alloc.c b/ext/opcache/zend_shared_alloc.c index 7f820fcdc27..3f18a4db604 100644 --- a/ext/opcache/zend_shared_alloc.c +++ b/ext/opcache/zend_shared_alloc.c @@ -328,7 +328,7 @@ static size_t zend_shared_alloc_get_largest_free_block(void) #define MIN_FREE_MEMORY 64*1024 #define SHARED_ALLOC_FAILED() do { \ - zend_accel_error(ACCEL_LOG_WARNING, "Not enough free shared space to allocate "ZEND_LONG_FMT" bytes ("ZEND_LONG_FMT" bytes free)", (zend_long)size, (zend_long)ZSMMG(shared_free)); \ + zend_accel_error(ACCEL_LOG_WARNING, "Not enough free shared space to allocate %zu bytes (%zu bytes free)", size, ZSMMG(shared_free)); \ if (zend_shared_alloc_get_largest_free_block() < MIN_FREE_MEMORY) { \ ZSMMG(memory_exhausted) = 1; \ } \ @@ -339,6 +339,10 @@ void *zend_shared_alloc(size_t size) int i; unsigned int block_size = ZEND_ALIGNED_SIZE(size); + if (UNEXPECTED(block_size < size)) { + zend_accel_error_noreturn(ACCEL_LOG_ERROR, "Possible integer overflow in shared memory allocation (%zu + %zu)", size, PLATFORM_ALIGNMENT); + } + #if 1 if (!ZCG(locked)) { zend_accel_error_noreturn(ACCEL_LOG_ERROR, "Shared memory lock not obtained"); From ea1287b015b713909096b5469ecc7fa569b13b45 Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Sat, 3 Sep 2022 11:23:43 +0200 Subject: [PATCH 2/3] Log the cause of error when opcache cannot write to file cache (#9258) --- ext/opcache/tests/file_cache_error.phpt | 42 +++++++++++++++++++++ ext/opcache/zend_file_cache.c | 49 +++++++++++++++++++++---- 2 files changed, 83 insertions(+), 8 deletions(-) create mode 100644 ext/opcache/tests/file_cache_error.phpt diff --git a/ext/opcache/tests/file_cache_error.phpt b/ext/opcache/tests/file_cache_error.phpt new file mode 100644 index 00000000000..75b63cd21cc --- /dev/null +++ b/ext/opcache/tests/file_cache_error.phpt @@ -0,0 +1,42 @@ +--TEST-- +File cache error 001 +--EXTENSIONS-- +opcache +posix +pcntl +--INI-- +opcache.enable_cli=1 +opcache.file_cache={TMP} +opcache.log_verbosity_level=2 +--SKIPIF-- + +--FILE-- + +--EXPECTF-- +bool(true) +%sWarning opcache cannot write to file %s: %s + +OK diff --git a/ext/opcache/zend_file_cache.c b/ext/opcache/zend_file_cache.c index badbac21855..a15736b72e5 100644 --- a/ext/opcache/zend_file_cache.c +++ b/ext/opcache/zend_file_cache.c @@ -1004,10 +1004,13 @@ static char *zend_file_cache_get_bin_file_path(zend_string *script_path) /** * Helper function for zend_file_cache_script_store(). * - * @return true on success, false on error + * @return true on success, false on error and errno is set to indicate the cause of the error */ static bool zend_file_cache_script_write(int fd, const zend_persistent_script *script, const zend_file_cache_metainfo *info, const void *buf, const zend_string *s) { + ssize_t written; + const ssize_t total_size = (ssize_t)(sizeof(*info) + script->size + info->str_size); + #ifdef HAVE_SYS_UIO_H const struct iovec vec[] = { { .iov_base = (void *)info, .iov_len = sizeof(*info) }, @@ -1015,12 +1018,42 @@ static bool zend_file_cache_script_write(int fd, const zend_persistent_script *s { .iov_base = (void *)ZSTR_VAL(s), .iov_len = info->str_size }, }; - return writev(fd, vec, sizeof(vec) / sizeof(vec[0])) == (ssize_t)(sizeof(*info) + script->size + info->str_size); + written = writev(fd, vec, sizeof(vec) / sizeof(vec[0])); + if (EXPECTED(written == total_size)) { + return true; + } + + errno = written == -1 ? errno : EAGAIN; + return false; #else - return ZEND_LONG_MAX >= (zend_long)(sizeof(*info) + script->size + info->str_size) && - write(fd, info, sizeof(*info)) == sizeof(*info) && - write(fd, buf, script->size) == script->size && - write(fd, ZSTR_VAL(s), info->str_size) == info->str_size; + if (UNEXPECTED(ZEND_LONG_MAX < (zend_long)total_size)) { +# ifdef EFBIG + errno = EFBIG; +# else + errno = ERANGE; +# endif + return false; + } + + written = write(fd, info, sizeof(*info)); + if (UNEXPECTED(written != sizeof(*info))) { + errno = written == -1 ? errno : EAGAIN; + return false; + } + + written = write(fd, buf, script->size); + if (UNEXPECTED(written != script->size)) { + errno = written == -1 ? errno : EAGAIN; + return false; + } + + written = write(fd, ZSTR_VAL(s), info->str_size); + if (UNEXPECTED(written != info->str_size)) { + errno = written == -1 ? errno : EAGAIN; + return false; + } + + return true; #endif } @@ -1095,7 +1128,7 @@ int zend_file_cache_script_store(zend_persistent_script *script, bool in_shm) #endif if (!zend_file_cache_script_write(fd, script, &info, buf, s)) { - zend_accel_error(ACCEL_LOG_WARNING, "opcache cannot write to file '%s'\n", filename); + zend_accel_error(ACCEL_LOG_WARNING, "opcache cannot write to file '%s': %s\n", filename, strerror(errno)); zend_string_release_ex(s, 0); close(fd); efree(mem); @@ -1107,7 +1140,7 @@ int zend_file_cache_script_store(zend_persistent_script *script, bool in_shm) zend_string_release_ex(s, 0); efree(mem); if (zend_file_cache_flock(fd, LOCK_UN) != 0) { - zend_accel_error(ACCEL_LOG_WARNING, "opcache cannot unlock file '%s'\n", filename); + zend_accel_error(ACCEL_LOG_WARNING, "opcache cannot unlock file '%s': %s\n", filename, strerror(errno)); } close(fd); efree(filename); From 349fdc9042883e67b59950972fa1c5776515dde6 Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Sat, 3 Sep 2022 11:28:18 +0200 Subject: [PATCH 3/3] [ci skip] NEWS --- NEWS | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/NEWS b/NEWS index b10c23ee486..4cab719fb8f 100644 --- a/NEWS +++ b/NEWS @@ -2,6 +2,10 @@ PHP NEWS ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||| ?? ??? ????, PHP 8.2.0RC2 +- Opcache: + . Fixed bug GH-9259 (opcache.interned_strings_buffer setting integer + overflow). (Arnaud) + - Standard: . Marked crypt()'s $string parameter as #[\SensitiveParameter]. (timwolla)