Edit report at http://bugs.php.net/bug.php?id=49937&edit=1
ID: 49937 Comment by: kkaminski at itens dot pl 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: 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? Previous Comments: ------------------------------------------------------------------------ [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. ------------------------------------------------------------------------ [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() ------------------------------------------------------------------------ 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
