Edit report at http://bugs.php.net/bug.php?id=49937&edit=1
ID: 49937 Updated by: basa...@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: pajoye, my first patch was a rough patch which used zval_add_ref_atomic. Second patch posted on 2009-11-17 is my final patch. This patch doesn't need any enhancement to php core and it worked for me and several others without crashes. I had tested with iplanet web server on Solaris and Linux and I believe it will work with other multithreaded web servers. Please review the patch posted on 2009-11-17, kkaminski, which patch did you tested. You should test the revised patch submitted on 2009-11-17. Please post the stack trace of the crash if you tested the right patch. Unfortunately php's bug tracker is very primitive in terms of feature. Apache's bug tracker is lot better :-) Previous Comments: ------------------------------------------------------------------------ [2010-08-23 13:50:14] paj...@php.net 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. ------------------------------------------------------------------------ [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; ------------------------------------------------------------------------ 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
