Edit report at https://bugs.php.net/bug.php?id=71115&edit=1
ID: 71115
Comment by: bobby dot mihalca at touchtech dot ro
Reported by: bobby dot mihalca at touchtech dot ro
Summary: data race on auto globals names and internal
functions names refcount
Status: Open
Type: Bug
Package: Reproducible crash
Operating System: Any
PHP Version: 7.0.0
Block user comment: N
Private report: N
New Comment:
Exclusive locking the sapi startup was also my first attempt.
Unfortunately valgring's helgrind would still detect a data race in zend_lookup_class_ex at
ZVAL_STR_COPY(&fcall_info.function_name, EG(autoload_func)->common.function_name);
While exclusive locking during startup stop always segfaulting, the data race is still there and
could segfault, is just less probable.
IMHO my fix is quite correct because:
1. names are already persistent allocated and HAVE TO live as long as the sapi
2. AFAIK php uses copy on write for strings, so even is a name is modified a new string will be
created
3. names don't change, _SERVER, _GET, _POST, fopen, fclose all keep their name
4. since interned flag is removed php_module_shutdown would free the strings and everything should
be ok (assuming that request are active at sapi shutdown, witch i guess is true)
However i think my sample does not cover dynamic loaded extensions, for my use case all modules are
static linked, so it works well.
A nice bonus of this fix is a increase from ~35k req/sec to ~50k req/sec due to lower startup cost.
Since all the names are interned even under ZTS it skips hundreds of (pointless) emalloc to copy
them and just uses existing interned zend_string*
If i did not miss somthing, a proper fix could integrate the changes in
php_module_startup/php_module_shutdown and dynamic module loading/unloading.
Previous Comments:
------------------------------------------------------------------------
[2015-12-14 13:18:13] krakjoe@php.net
This is definitely a problem ...
pthreads uses critical sections around startup/shutdown of user threads to work around the problem
... I'd like to remove those sections ...
I'm not sure what the proper fix should look like though, disabling refcounts on those strings
will hide internal programmer error (leaks).
------------------------------------------------------------------------
[2015-12-14 11:50:17] bobby dot mihalca at touchtech dot ro
Description:
------------
There is a data race on string refcount that could/will cause any multithreaded sapi to segfault.
This happens because during request startup ts_resource, php_request_startup, etc will copy
auto_globals, internal functions, etc from php to request.
Since the name is an interned string thus a zend_string* if ZTS enabled, the refcount will be used,
but the refcount is not atomic and we have a data race resulting in segfaults.
Ex while copy _SERVER auto global name (refcount=1):
thread 1 load refcount = 1
thread 2 load refount = 1
thread 1 inc refcount = 2
thread 2 inc refcount = 2
thread 1 store refount
thread 2 store refount
refcount=2 instead of 3
thread 1 release string, refcount = 1
thread 2 release string, refount = 0, free string
thread 3 start, segfault due to accessing freed memory
A quick workaround for this problem is to mark all the names as interned after startup and remove
the flag before shutdown.
This will prevent reference counting and thus data race, premature release and segfaults.
I use the following code for my startup/shutdown:
static int startup(sapi_module_struct * sapi_module)
{
if (php_module_startup(sapi_module, NULL, 0) == SUCCESS) {
zend_auto_global *global;
zend_internal_function *function;
zend_class_entry* class;
ZEND_HASH_FOREACH_PTR(CG(auto_globals), global) {
GC_FLAGS(global->name) |= IS_STR_INTERNED;
zend_string_hash_val(global->name);
} ZEND_HASH_FOREACH_END();
ZEND_HASH_FOREACH_PTR(CG(function_table), function) {
GC_FLAGS(function->function_name) |= IS_STR_INTERNED;
zend_string_hash_val(function->function_name);
} ZEND_HASH_FOREACH_END();
ZEND_HASH_FOREACH_PTR(CG(class_table), class) {
zend_property_info *property;
GC_FLAGS(class->name) |= IS_STR_INTERNED;
zend_string_hash_val(class->name);
ZEND_HASH_FOREACH_PTR(&class->properties_info, property) {
GC_FLAGS(property->name) |= IS_STR_INTERNED;
zend_string_hash_val(property->name);
if (property->doc_comment) {
GC_FLAGS(property->doc_comment) |= IS_STR_INTERNED;
}
} ZEND_HASH_FOREACH_END();
} ZEND_HASH_FOREACH_END();
return SUCCESS;
}
return FAILURE;
}
int shutdown(sapi_module_struct *sapi_globals)
{
zend_auto_global *global;
zend_internal_function *function;
zend_class_entry* class;
ZEND_HASH_FOREACH_PTR(CG(auto_globals), global) {
GC_FLAGS(global->name) &= ~IS_STR_INTERNED;
} ZEND_HASH_FOREACH_END();
ZEND_HASH_FOREACH_PTR(CG(function_table), function) {
GC_FLAGS(function->function_name) &= ~IS_STR_INTERNED;
} ZEND_HASH_FOREACH_END();
ZEND_HASH_FOREACH_PTR(CG(class_table), class) {
zend_property_info *property;
GC_FLAGS(class->name) &= ~IS_STR_INTERNED;
ZEND_HASH_FOREACH_PTR(&class->properties_info, property) {
GC_FLAGS(property->name) &= ~IS_STR_INTERNED;
if (property->doc_comment) {
GC_FLAGS(property->doc_comment) &= ~IS_STR_INTERNED;
}
} ZEND_HASH_FOREACH_END();
} ZEND_HASH_FOREACH_END();
php_module_shutdown();
return SUCCESS;
}
Note that zend_string_hash_val is not necessary but will prevent data race on hash (witch is
probably harmless anyway)
------------------------------------------------------------------------
--
Edit this bug report at https://bugs.php.net/bug.php?id=71115&edit=1