Edit report at https://bugs.php.net/bug.php?id=71115&edit=1
ID: 71115
Updated by: krakjoe@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:
I have forwarded bobby and anatol a poc solution ... one that breaks the rest of PHP, but I think we
can do it with small changes contained in zend_string.* without breaking ABI or API.
Previous Comments:
------------------------------------------------------------------------
[2015-12-15 11:04:08] bobby dot mihalca at touchtech dot ro
@ab
- Multiple threads reading read only global data is not of an issue.
- Same mechanics does exist in PHP5, disregarding zend_string.
Yes but,
php 5 uses const char *, copying a const char * is read only operation
php 7 uses zend_string*, copying a zend_string* is NOT read only as writes refcount.
This is the problem, it copies global zend_string* data concurrently causing data race, causing
refcount==0 instead of >=1, causing string free, causing crash.
Global data zend_string* needs to be read only, no refcount without exclusive lock.
Wish it was this clear on initial bug report :)
------------------------------------------------------------------------
[2015-12-15 10:39:26] bobby dot mihalca at touchtech dot ro
@krakjoe
Yes is a hack, it only works because the following stars are aligned :)
* strings are already persistent
* no interned implemented for TS, (hopefully) nothing to mess up
* refcount not used for interned strings
My assumption is that since the names are init during module startup, there should already be code
to release them during module shutdown, else we would have a leak now ?
Also i'm assuming no request can be running at module shutdown so nobody other then module
holds a pointer.
I should have not brought up the workaround, i was trying to help but only managed to distract and
confuse with it.
------------------------------------------------------------------------
[2015-12-15 06:31:25] krakjoe@php.net
I think bobby knows it's a hack, he's saying the result is correct, and I agree with him.
A persistent string should not be reference counted, since it can lead to breaking the API's
promise and freeing the string early, which is the problem here.
The problem is that the solution used is a hack to get the desired result, but I'm not sure
that we can do the same thing internally because I can't see an opportunity to call the final
zend_string_free on all strings allocated persistently.
What we may need to do is store persistent strings in some truly global (safe) structure and destroy
them at process shutdown. Then change zend_string_copy to omit increment, and zend_string_release to
omit decrement, if & IS_STR_PERSISTENT, and change every use of GC_REFCOUNT(str)++|-- to use the
API.
I haven't thought about it for very long, but some of that seems to make sense ... I'll
keep thinking about it ...
------------------------------------------------------------------------
[2015-12-14 21:26:21] ab@php.net
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.
------------------------------------------------------------------------
[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
------------------------------------------------------------------------
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