Edit report at http://bugs.php.net/bug.php?id=49937&edit=1
ID: 49937 Updated by: paj...@php.net Reported by: basa...@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] basa...@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] basa...@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] basa...@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
