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: Feedback
Type: Bug
Package: Reproducible crash
Operating System: Any
PHP Version: 7.0.0
Assigned To: ab
Block user comment: N
Private report: N
New Comment:
It's a custom sapi based on a civetweb and while i can't publicly share the code, i can
send you a copy.
Just need a bit of time to rip it out of the project.
Previous Comments:
------------------------------------------------------------------------
[2015-12-14 16:53:01] ab@php.net
@bobby, could you please show the code reproducing the crash? It is so, that such global names are
allocated persistently and this is addressed in the GC info. So they should not be freed by GC and
persist all the process life time, while not sure it will go through it.
Currently the TS SAPI I work quite often is Apache, and neither Windows nor Linux show any crashes
or issues with dozens of threads. Of course it is not bug free. But exactly for this reason @bobby
I'd ask you to please show a piece of code to reproduce and debug the behavior you describe.
What SAPI is it? I can see a couple of places that could be improved, but without some real
reproducer it might be just a theory of mine.
Thanks.
------------------------------------------------------------------------
[2015-12-14 14:20:47] laruence@php.net
weltingï¼ what do you think? Make them interned seems okey,but not sure if there is similar
issues out of this?
------------------------------------------------------------------------
[2015-12-14 14:14:03] bobby dot mihalca at touchtech dot ro
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.
------------------------------------------------------------------------
[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