ID: 49937
Updated by: basa...@php.net
Reported By: basa...@php.net
Status: Open
Bug Type: PDO related
Operating System: Linux
PHP Version: 5.2.11
New Comment:
More explaination about the problem :
Php class PDOStatement is created inside pdo_stmt_init. A property
named
"queryString" is inserted to the properties table (zend_hash) using
zend_declare_property_null.
INIT_CLASS_ENTRY(ce, "PDOStatement", pdo_dbstmt_functions);
pdo_dbstmt_ce = zend_register_internal_class(&ce TSRMLS_CC);
pdo_dbstmt_ce->get_iterator = pdo_stmt_iter_get;
pdo_dbstmt_ce->create_object = pdo_dbstmt_new;
zend_class_implements(pdo_dbstmt_ce TSRMLS_CC, 1,
zend_ce_traversable);
zend_declare_property_null(pdo_dbstmt_ce, "queryString",
sizeof("queryString")-1, ZEND_ACC_PUBLIC TSRMLS_CC);
zend_declare_property_null allocates a null zval using malloc for
"queryString" property.
int zend_declare_property_null(...) {
zval *property;
if (ce->type & ZEND_INTERNAL_CLASS) {
property = malloc(sizeof(zval));
} else {
...
}
Now pdoStatement->ce->default_properties{"queryString"} is a null zval
object
created by "malloc". Let me call this zval object zPdoStmtQstrProp for
explaination.
Now notice the function call in dbstmt_clone_obj or pdo_dbstmt_new :
zend_hash_copy(stmt->properties, &stmt->ce->default_properties,
(copy_ctor_func_t) zval_add_ref, (void *) &tmp, sizeof(zval *));
When PDOStatement is cloned, it copies the stmt->ce->default_properties
to the
new statement using zend_hash_copy. zend_hash_copy will call
zPdoStmtQstrProp->ref_count++ by calling zval_add_ref.
This is what is thread-unsafe. zPdoStmtQstrProp is a global object
whose
reference count is incremented without any kind of lock. Under stress
situation since ref count becomes 0 and efree tries to invoke for a
object
which is created using malloc.
--------------------------------------------------------------
Where does decrement happens :
-------------------------
gdb) where
#0 _zval_ptr_dtor (zval_ptr=0x4d7fa10) at
../PHP_5_2/Zend/zend_execute_API.c:413
#1 0x015cf4cb in zend_std_write_property (object=0xb7718ad8,
member=0x4d7faa4, value=0xb7718c90,
tsrm_ls=0xb77087e8) at ../PHP_5_2/Zend/zend_object_handlers.c:417
#2 0x013dfb76 in pdo_stmt_construct (stmt=0xb7718b48,
object=0xb7718ad8, dbstmt_ce=0x9d0f0c8, ctor_args=0x0,
tsrm_ls=0xb77087e8) at ../PHP_5_2/ext/pdo/pdo_dbh.c:446
This is the place where we see the crash. As the object was allocated
using
malloc and _zval_ptr_dtor tries to free the object using efree.
--------------------------------------------------------------
Regarding submitted fix :
Now regarding the fix which I submitted though seems to solve the
problem but
the fix still doesn't address the issue completely because it doesn't
ensure
the atomicity of the decrement call. A new fix is required which ensure
the
thread safety of zPdoStmtQstrProp->refcount.
Previous Comments:
------------------------------------------------------------------------
[2009-10-20 22:45:34] basa...@php.net
Here is the patch which fixes the race condition in pdo :
Patch is generated against PHP_5_2 svn branch.
Index: ext/pdo/pdo_stmt.c
===================================================================
--- ext/pdo/pdo_stmt.c (revision 289806)
+++ ext/pdo/pdo_stmt.c (working copy)
@@ -2325,7 +2325,7 @@
stmt->refcount = 1;
ALLOC_HASHTABLE(stmt->properties);
zend_hash_init(stmt->properties, 0, NULL, ZVAL_PTR_DTOR, 0);
- zend_hash_copy(stmt->properties, &stmt->ce->default_properties,
(copy_ctor_func_t) zval_add_ref, (void *) &tmp, sizeof(zval *));
+ zend_hash_copy(stmt->properties, &stmt->ce->default_properties,
(copy_ctor_func_t) zval_add_ref_atomic, (void *) &tmp, sizeof(zval *));
old_stmt = (pdo_stmt_t *)zend_object_store_get_object(zobject
TSRMLS_CC);
@@ -2454,7 +2454,7 @@
stmt->refcount = 1;
ALLOC_HASHTABLE(stmt->properties);
zend_hash_init(stmt->properties, 0, NULL, ZVAL_PTR_DTOR, 0);
- zend_hash_copy(stmt->properties, &ce->default_properties,
(copy_ctor_func_t) zval_add_ref, (void *) &tmp, sizeof(zval *));
+ zend_hash_copy(stmt->properties, &ce->default_properties,
(copy_ctor_func_t) zval_add_ref_atomic, (void *) &tmp, sizeof(zval *));
retval.handle = zend_objects_store_put(stmt,
(zend_objects_store_dtor_t)zend_objects_destroy_object,
(zend_objects_free_object_storage_t)pdo_dbstmt_free_storage,
(zend_objects_store_clone_t)dbstmt_clone_obj TSRMLS_CC);
retval.handlers = &pdo_dbstmt_object_handlers;
Index: TSRM/TSRM.c
===================================================================
--- TSRM/TSRM.c (revision 289806)
+++ TSRM/TSRM.c (working copy)
@@ -714,6 +714,12 @@
return retval;
}
+TSRM_API void *tsrm_atomic_incr(volatile unsigned int* val)
+{
+ tsrm_mutex_lock(tsmm_mutex);
+ ++*val;
+ tsrm_mutex_unlock(tsmm_mutex);
+}
/*
Index: TSRM/TSRM.h
===================================================================
--- TSRM/TSRM.h (revision 289806)
+++ TSRM/TSRM.h (working copy)
@@ -139,6 +139,7 @@
TSRM_API void
*tsrm_set_new_thread_begin_handler(tsrm_thread_begin_func_t
new_thread_begin_handler);
TSRM_API void *tsrm_set_new_thread_end_handler(tsrm_thread_end_func_t
new_thread_end_handler);
+TSRM_API void *tsrm_atomic_incr(volatile unsigned int* val);
/* these 3 APIs should only be used by people that fully understand
the threading model
* used by PHP/Zend and the selected SAPI. */
Index: Zend/zend_variables.c
===================================================================
--- Zend/zend_variables.c (revision 289806)
+++ Zend/zend_variables.c (working copy)
@@ -100,6 +100,17 @@
}
/* }}} */
+
+ZEND_API void zval_add_ref_atomic(zval **p) /* {{{ */
+{
+#ifdef ZTS
+ tsrm_atomic_incr(&(*p)->refcount);
+#else
+ (*p)->refcount++;
+#endif
+}
+/* }}} */
+
ZEND_API void _zval_copy_ctor_func(zval *zvalue ZEND_FILE_LINE_DC) /*
{{{ */
{
switch (zvalue->type) {
Index: Zend/zend_variables.h
===================================================================
--- Zend/zend_variables.h (revision 289806)
+++ Zend/zend_variables.h (working copy)
@@ -76,6 +76,7 @@
#endif
ZEND_API void zval_add_ref(zval **p);
+ZEND_API void zval_add_ref_atomic(zval **p);
END_EXTERN_C()
------------------------------------------------------------------------
[2009-10-20 22:43:54] basa...@php.net
Description:
------------
There is a race condition in pdo's stmt PDOStatement class.
This class is dynamically created and it adds a member named
queryString
(inside pdo_stmt_init).
zend_declare_property_null allocates property using malloc.
Later pdo_dbstmt_ce is copied to other hashes in pdo_dbstmt_new.
zend_hash_copy increments refcount of pdo_dbstmt_ce->queryString
property. In
multithreaded php refcount increment was not atomic. It was causing
refcount
to become 0 and hence efree was trying to delete something which was
allocated
from malloc.
There is a php benchmark kit named olio and can be downloaded from :
https://cds.sun.com/is-bin/INTERSHOP.enfinity/WFS/CDS-CDS_SMI-Site/en_US/-/USD/viewproductdetail-start?productref=olio-php-1.0-a-...@cds-cds_smi
The bug is easily reproducible with olio php benchmark inside Sun Web
Server.
Expected result:
----------------
Correct functionality
Actual result:
--------------
Stack trace :
--------------
Program terminated with signal 11, Segmentation fault.
#0 0x00002ba1630451e0 in _zend_mm_free_int ()
from /home/sun/webserver7/bin/libphp5.so
#1 0x00002ba163084aa0 in zend_std_write_property ()
from /home/sun/webserver7/bin/libphp5.so
#2 0x00002ba162ebfc4a in pdo_stmt_construct ()
from /home/sun/webserver7/bin/libphp5.so
#3 0x00002ba162ec0073 in zim_PDO_query ()
from /home/sun/webserver7/bin/libphp5.so
#4 0x00002ba1630999f9 in zend_do_fcall_common_helper_SPEC ()
from /home/sun/webserver7/bin/libphp5.so
#5 0x00002ba16308705f in execute () from
/home/sun/webserver7/bin/libphp5.so
#6 0x00002ba1630993d8 in zend_do_fcall_common_helper_SPEC ()
from /home/sun/webserver7/bin/libphp5.so
#7 0x00002ba16308705f in execute () from
/home/sun/webserver7/bin/libphp5.so
#8 0x00002ba1630630fa in zend_execute_scripts ()
from /home/sun/webserver7/bin/libphp5.so
#9 0x00002ba1630188bb in php_execute_script ()
from /home/sun/webserver7/bin/libphp5.so
#10 0x00002ba1630ee465 in php5_execute ()
------------------------------------------------------------------------
--
Edit this bug report at http://bugs.php.net/?id=49937&edit=1
