Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:59353
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain schlueters.de from 217.114.211.66 cause and error)
To: Dmitry Stogov <dmitry@php.net>
Cc: PHP internals list <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Date: Thu, 05 Apr 2012 19:39:14 +0200
Message-ID: <1333647554.1636.48.camel@guybrush>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: Threading issues with internal classes
From: johannes@schlueters.de (Johannes =?ISO-8859-1?Q?Schl=FCter?=)

Dmitry,

while looking at #55334 I discovered two issues regarding the handling
of properties and constants in threaded environments which might cause
race conditions.

The reproduce code for both issues is as simple as
  <?php new Exception; ?>
and then hit it on a 8+ core box in threaded mode. (using less cores
crashes it, too, but might take a bit longer, 8 core is quite reliable
in a few moments)

1) In PHP 5.4 default properties aren't duplicated anymore.
===========================================================

Up to PHP 5.3 the common way to separate properties was by using 

   zend_hash_copy(intern->zo.properties, &class_type->default_properties, (copy_ctor_func_t)  zval_property_ctor,
                  (void *) &tmp, sizeof(zval *));

zval_property_ctor in non-ZTS would then do an ADDREF and in ZTS do a
copy of the property.
Since 5.4 there is a new API object_properties_init which always does an
ADDREF only.
I think a patch like
    https://github.com/johannes/php-src/compare/zts-property-duplication
is required.

2) Class constants are updated in a non-thread-safe way
=======================================================

When running with a threaded SAPI under valgrind's helgrind I can see
reports like these:

==23328== Possible data race during read of size 4 at 0x4d30814 by thread #3
==23328==    at 0x6086C7: ZEND_NEW_SPEC_HANDLER (zend_vm_execute.h:803)
==23328==    by 0x6233F2: execute (zend_vm_execute.h:410)
==23328==    by 0x5B165B: zend_execute_scripts (zend.c:1272)
==23328==    by 0x54AFB5: php_execute_script (main.c:2473)
==23328==    by 0x669B11: pconn_do_request (pconnect-sapi.c:208)
==23328==    by 0x6698F9: php_thread (main.c:63)
==23328==    by 0x4A0C01D: mythread_wrapper (hg_intercepts.c:221)
==23328==    by 0x30BF6077F0: start_thread (in /lib64/libpthread-2.12.so)
==23328==    by 0xBEC46FF: ???
==23328==  This conflicts with a previous write of size 4 by thread #2
==23328==    at 0x5BB16E: zend_update_class_constants (zend_API.c:1082)
==23328==    by 0x5BB3D3: _object_and_properties_init (zend_API.c:1124)
==23328==    by 0x6086FE: ZEND_NEW_SPEC_HANDLER (zend_vm_execute.h:813)
==23328==    by 0x6233F2: execute (zend_vm_execute.h:410)
==23328==    by 0x5B165B: zend_execute_scripts (zend.c:1272)
==23328==    by 0x54AFB5: php_execute_script (main.c:2473)
==23328==    by 0x669B11: pconn_do_request (pconnect-sapi.c:208)
==23328==    by 0x6698F9: php_thread (main.c:63)
==23328==  Address 0x4d30814 is 36 bytes inside a block of size 568 alloc'd
==23328==    at 0x4A0626E: malloc (vg_replace_malloc.c:236)
==23328==    by 0x5B9B4C: do_register_internal_class (zend_API.c:2394)
==23328==    by 0x5CD01F: zend_register_default_exception (zend_exceptions.c:679)
==23328==    by 0x5DC4F0: zend_register_default_classes (zend_default_classes.c:33)
==23328==    by 0x5C7DBD: zm_startup_core (zend_builtin_functions.c:320)
==23328==    by 0x5BA3D8: zend_startup_module_ex (zend_API.c:1661)
==23328==    by 0x5BFFC9: zend_hash_apply (zend_hash.c:716)
==23328==    by 0x5BA1DB: zend_startup_modules (zend_API.c:1788)
==23328==    by 0x54D56B: php_module_startup (main.c:2191)
==23328==    by 0x669C32: startup (pconnect-sapi.c:35)
==23328==    by 0x669CFC: pconn_init_php (pconnect-sapi.c:126)
==23328==    by 0x669859: main (main.c:237)

Which boils down to 
   class_type->ce_flags |= ZEND_ACC_CONSTANTS_UPDATED
in zend_update_class_constants() vs. different places where this is read.

I can't create a crash from this, but I think that should still be
fixed.

Can you look into these things? - Thanks!

johannes

P.S. For most of my tests I used my SAPI from
https://github.com/johannes/pconn-sapi which is more lightweight than
setting up a webserver and load generator (while I have issues with the
buildsystem with 5.4/master, which makes it hard to build right now)

P.P.S. Would anybody mind if we drop TSRM/ZTS? :-)