Edit report at http://bugs.php.net/bug.php?id=49937&edit=1
ID: 49937
Updated by: pajoye@php.net
Reported by: basantk@php.net
Summary: [PATCH] Race condition in PDOStatement
Status: Assigned
Type: Bug
Package: PDO related
Operating System: Linux
PHP Version: 5.2.11
Assigned To: basantk
Block user comment: N
New Comment:
I'm not sure the fix is correct (while it can work). It should be done
inside zend_hash_copy instead of adding a zval_add_ref_atomic. Or?
zval_ref_atomic could be added but then I would rather not use the
expensive TSRM locking system but atomic declarations and the OS
specific atomic APIs.
Previous Comments:
------------------------------------------------------------------------
[2010-08-23 12:30:01] kkaminski at itens dot pl
Tested patch and it works, but now I'm having similar problem.
Apache is crashing in
pdo_stmt_construct
on
std_object_handlers.write_property(object, &z_key, query_string
TSRMLS_CC);
Could you please verify this?
------------------------------------------------------------------------
[2010-07-13 09:57:04] kkaminski at itens dot pl
Any chances of getting this patch into PHP 5.2.14? Or getting a windows
build for testing?
I'd like to give it a try but having problems building PHP under
winblows :/
------------------------------------------------------------------------
[2009-11-17 02:51:36] basantk@php.net
Here is the link to the patch which I submitted in previous
comments.
http://bitbucket.org/basantk/phpbugs/raw/5c3ca3a306ed/pdo_bug_52trunk.txt
Marking the bug as Patch available.
------------------------------------------------------------------------
[2009-10-23 18:15:51] basantk@php.net
Here is a revised patch which is much less invasive, restricts totally
to pdo.
Fix is to avoid using shared zval ptr
(PdoStatement->ce->propertiers{"queryString"}) in multiple PdoStatement
objects. Instead it creates a copy of the "null" zval for individual
object.
This fix the race condition for me and have been verified by one of the
olio
php customer. I ran the pdo_sqlite tests and didn't find any
regression.
Here is the fix
---------------------------------------------
Index: ext/pdo/pdo_stmt.c
===================================================================
--- ext/pdo/pdo_stmt.c (revision 289806)
+++ ext/pdo/pdo_stmt.c (working copy)
@@ -2312,6 +2312,54 @@
return -1;
}
+static void init_stmt_properties(pdo_stmt_t* stmt TSRMLS_DC)
+{
+ HashTable* ht = &stmt->ce->default_properties;
+ HashTable* target = stmt->properties;
+
+ HashPosition pos;
+ zend_hash_internal_pointer_reset_ex(ht, &pos);
+ while(zend_hash_has_more_elements_ex(ht, &pos)
+ == SUCCESS) {
+ ulong index;
+ char* key = NULL;
+ uint keylen = 0;
+ int ret = zend_hash_get_current_key_ex(ht,
+ &key,
+ &keylen,
+ &index, 0,
+ &pos);
+ if ((keylen == sizeof("queryString"))
+ && (strncmp(key, "queryString", keylen) == 0)) {
+ zval* qval;
+ /* Since the value for the key queryString in
+ * stmt->ce->default_properties is shared by multiple threads so
+ * we can not add the same zval in stmt->properties. we need to
+ * create a null property object. See Bug 49937 */
+ ALLOC_INIT_ZVAL(qval);
+ zend_hash_add(stmt->properties, "queryString",
+ sizeof("queryString"), (void**) &qval, sizeof(zval*), NULL);
+ }
+ else {
+ void* data = NULL;
+ zend_hash_get_current_data_ex(ht,
+ (void **) &data, &pos);
+ void *new_entry = NULL;
+ if (data) {
+ /* We expect keylen should be > 0. default_properties hash
+ * should only contain named keys */
+ if (keylen) {
+ zend_hash_quick_update(target, key, keylen, 0, data,
sizeof(void*), &new_entry);
+ }
+ if (new_entry) {
+ zval_add_ref(new_entry);
+ }
+ }
+ }
+ zend_hash_move_forward_ex(ht, &pos);
+ }
+}
+
static zend_object_value dbstmt_clone_obj(zval *zobject TSRMLS_DC)
{
zend_object_value retval;
@@ -2325,7 +2373,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 *));
+ init_stmt_properties(stmt TSRMLS_CC);
old_stmt = (pdo_stmt_t *)zend_object_store_get_object(zobject
TSRMLS_CC);
@@ -2454,7 +2502,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 *));
+ init_stmt_properties(stmt TSRMLS_CC);
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;
------------------------------------------------------------------------
[2009-10-21 21:06:58] basantk@php.net
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.
------------------------------------------------------------------------
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
http://bugs.php.net/bug.php?id=49937
--
Edit this bug report at http://bugs.php.net/bug.php?id=49937&edit=1