Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix high opcache.interned_strings_buffer causing shm corruption #9260

Merged
merged 3 commits into from
Sep 3, 2022
Merged

Fix high opcache.interned_strings_buffer causing shm corruption #9260

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5c2373a
Fix high opcache.interned_strings_buffer causing shm corruption
arnaud-lb Aug 5, 2022
1fac95d
Wording
arnaud-lb Aug 6, 2022
e1eaa8c
Return failure in case of a too large value
arnaud-lb Sep 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions ext/opcache/ZendAccelerator.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
19 changes: 19 additions & 0 deletions ext/opcache/tests/gh9259_001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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--
<?php

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)
18 changes: 18 additions & 0 deletions ext/opcache/tests/gh9259_002.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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--
<?php

echo 'OK';

?>
--EXPECTF--
%sWarning opcache.interned_strings_buffer must be greater than or equal to 0, -1 given%s

OK
15 changes: 15 additions & 0 deletions ext/opcache/tests/gh9259_003.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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--
<?php

echo 'OK';

?>
--EXPECTF--
%sFatal Error Insufficient shared memory for interned strings buffer! (tried to allocate %d bytes)
22 changes: 21 additions & 1 deletion ext/opcache/zend_accelerator_module.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

I do not know for certain if we should return an error for both cases, as you said in the ticket, it can me a mistake. However that fix the issue alright whatsoever.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I wanted to be consistent with OnUpdateMemoryConsumption, but maybe returning a FAILURE in both cases here would be better.

In my opinion, ini handler failures during startup/activate should also result in a fatal error, but that's for an other PR/RFC.

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();
Expand Down Expand Up @@ -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)
Expand Down
6 changes: 5 additions & 1 deletion ext/opcache/zend_shared_alloc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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; \
} \
Expand All @@ -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");
Expand Down