From 5c2373a3587f562370a96341a3e9d860e154282d Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Fri, 5 Aug 2022 21:03:37 +0200 Subject: [PATCH 1/3] Fix high opcache.interned_strings_buffer causing shm corruption --- ext/opcache/ZendAccelerator.c | 11 ++++++++--- ext/opcache/tests/gh9259_001.phpt | 19 +++++++++++++++++++ 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, 86 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 0478e8067e314..0c4f67ccad5a6 100644 --- a/ext/opcache/ZendAccelerator.c +++ b/ext/opcache/ZendAccelerator.c @@ -2838,18 +2838,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 0000000000000..8d7ec6fa290d4 --- /dev/null +++ b/ext/opcache/tests/gh9259_001.phpt @@ -0,0 +1,19 @@ +--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 + +%sWarning Not enough free shared space to allocate %d bytes (%d bytes free) +%sFatal Error Insufficient shared memory for interned strings buffer! (tried to allocate %d bytes) diff --git a/ext/opcache/tests/gh9259_002.phpt b/ext/opcache/tests/gh9259_002.phpt new file mode 100644 index 0000000000000..3f75d362a0c76 --- /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 higher 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 0000000000000..91bfcad917b36 --- /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 2271920b302c7..4ceaab0f4c61d 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 higher 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); + *p = MAX_INTERNED_STRINGS_BUFFER_SIZE; + } else { + *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 7f820fcdc27b0..3f18a4db6040e 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 1fac95dceb3ff436e2897761216d90e64a2f1c0a Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Sat, 6 Aug 2022 10:39:56 +0200 Subject: [PATCH 2/3] Wording --- ext/opcache/tests/gh9259_002.phpt | 2 +- ext/opcache/zend_accelerator_module.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ext/opcache/tests/gh9259_002.phpt b/ext/opcache/tests/gh9259_002.phpt index 3f75d362a0c76..8b74949b49469 100644 --- a/ext/opcache/tests/gh9259_002.phpt +++ b/ext/opcache/tests/gh9259_002.phpt @@ -13,6 +13,6 @@ echo 'OK'; ?> --EXPECTF-- -%sWarning opcache.interned_strings_buffer must be higher than or equal to 0, -1 given%s +%sWarning opcache.interned_strings_buffer must be greater than or equal to 0, -1 given%s OK diff --git a/ext/opcache/zend_accelerator_module.c b/ext/opcache/zend_accelerator_module.c index 4ceaab0f4c61d..19db31765c341 100644 --- a/ext/opcache/zend_accelerator_module.c +++ b/ext/opcache/zend_accelerator_module.c @@ -85,7 +85,7 @@ static ZEND_INI_MH(OnUpdateInternedStringsBuffer) 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 higher than or equal to 0, " ZEND_LONG_FMT " given.\n", size); + 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) { From e1eaa8c1e73bca522eee1d16b9a8eb0c469253bc Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Sat, 3 Sep 2022 10:38:36 +0200 Subject: [PATCH 3/3] Return failure in case of a too large value --- ext/opcache/tests/gh9259_001.phpt | 3 +-- ext/opcache/zend_accelerator_module.c | 6 +++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/ext/opcache/tests/gh9259_001.phpt b/ext/opcache/tests/gh9259_001.phpt index 8d7ec6fa290d4..bcc0f113c57f6 100644 --- a/ext/opcache/tests/gh9259_001.phpt +++ b/ext/opcache/tests/gh9259_001.phpt @@ -15,5 +15,4 @@ echo 'OK'; --EXPECTF-- %sWarning opcache.interned_strings_buffer must be less than or equal to 4095, 131072 given%s -%sWarning Not enough free shared space to allocate %d bytes (%d bytes free) -%sFatal Error Insufficient shared memory for interned strings buffer! (tried to allocate %d bytes) +OK diff --git a/ext/opcache/zend_accelerator_module.c b/ext/opcache/zend_accelerator_module.c index 19db31765c341..1c4b3002bc666 100644 --- a/ext/opcache/zend_accelerator_module.c +++ b/ext/opcache/zend_accelerator_module.c @@ -90,11 +90,11 @@ static ZEND_INI_MH(OnUpdateInternedStringsBuffer) } 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); - *p = MAX_INTERNED_STRINGS_BUFFER_SIZE; - } else { - *p = size; + return FAILURE; } + *p = size; + return SUCCESS; }