Edit report at https://bugs.php.net/bug.php?id=71115&edit=1
ID: 71115
Updated by: ab@php.net
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:
Here are some points:
- Persistent allocation is the promise of the API, it is not only about zend_string.
- Marking interned in TS build is a hack, no interned strings are implemented for thread safe
builds.
- Multiple threads reading read only global data is not of an issue.
- Same mechanics does exist in PHP5, disregarding zend_string.
Anyway, we should debug and evaluate the issue. It can have to do with a bug in PHP same way with a
bug in the given SAPI implementation. Anyway one should be careful before making any conclusions.
@bobby, thanks for sending. I'll arrive by debugging on Friday. If @krakjoe has time/mood
before, please share it with him as well. He is the author of the ext/pthreads and is quite
experienced in the matter.
Thanks.
Previous Comments:
------------------------------------------------------------------------
[2015-12-14 20:28:37] bobby dot mihalca at touchtech dot ro
@ab sent you the sources for the sapi
@krakjoe
being persistent allocated just allows my workaround to work as they are not freed at request
shutdown.
Now "_GET" is copied to a new persistent string as part php_module_startup in
php_startup_auto_globals
zend_register_auto_global(zend_string_init("_GET", sizeof("_GET")-1, 1),
0,php_auto_globals_create_get);
then each request will copy it in compiler_globals_ctor
zend_hash_copy(compiler_globals->auto_globals, global_auto_globals_table,
auto_global_copy_ctor);
When 2 threads/requests do the copy at the same time you have the described data race and segfault
Sice auto globals are allocated by sapi, they live as long as the sapi and there is no need to copy
them on each request, we can safely mark them as interned.
Before module shutdown i remove the interned flag and module shutdown will free the memory.
The same goes to internal function names, classes,etc
For dynamic extensions/dl() the making/unmarking should be part of module load/unload.
Since ZVAL_STR_COPY (as in zend_lookup_class_ex) will increase refcount for non interned strings i
see no viable solution other then marking them interned.
I was also thinking about making the refcount atomic but that will have performance implications.
Besides would break ABI
------------------------------------------------------------------------
[2015-12-14 19:20:59] krakjoe@php.net
Persistent strings still use a refcount, they are just allocated using system memory rather than
zend heap.
There is nothing about using system memory that means they should actually persist.
------------------------------------------------------------------------
[2015-12-14 17:51:31] bobby dot mihalca at touchtech dot ro
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.
------------------------------------------------------------------------
[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?
------------------------------------------------------------------------
The remainder of the comments for this report are too long. To view
the rest of the comments, please view the bug report online at
https://bugs.php.net/bug.php?id=71115
--
Edit this bug report at https://bugs.php.net/bug.php?id=71115&edit=1