ID: 49937 Updated by: basa...@php.net -Summary: Race condition in PDOStatement Reported By: basa...@php.net Status: Open Bug Type: PDO related Operating System: Linux PHP Version: 5.2.11 New Comment:
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. Previous Comments: ------------------------------------------------------------------------ [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() ------------------------------------------------------------------------ [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
