From f3be8d1832ef461e93a3ecbb9712819746035537 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 16 Jan 2025 15:39:18 +0000 Subject: [PATCH 01/10] First look at shimming std property handlers This works by pushing values from shared storage to local cache on read, and then letting std handlers operate on the local property table. Vice versa for write. This allows us to get support for stuff like 8.4 property hooks without copy pasting lots of code and having inconsistencies. this isn't fully stable yet, but should be working. --- classes/thread_safe.c | 2 +- src/handlers.c | 139 +++----- src/object.c | 4 +- src/routine.c | 4 +- src/store.c | 410 +++++++++++++--------- src/store.h | 16 +- src/worker.c | 4 +- tests/uninitialized-typed-properties.phpt | 4 +- 8 files changed, 319 insertions(+), 264 deletions(-) diff --git a/classes/thread_safe.c b/classes/thread_safe.c index 15fa82f5..77089409 100644 --- a/classes/thread_safe.c +++ b/classes/thread_safe.c @@ -83,7 +83,7 @@ ThreadSafe_method(synchronized) if (pmmpthread_monitor_lock(&threaded->monitor)) { /* synchronize property tables */ - pmmpthread_store_sync_local_properties(Z_OBJ_P(getThis())); + pmmpthread_store_clean_stale_cache(Z_OBJ_P(getThis())); zend_try { /* call the closure */ diff --git a/src/handlers.c b/src/handlers.c index 2d06bc47..40eebf49 100644 --- a/src/handlers.c +++ b/src/handlers.c @@ -66,7 +66,7 @@ zval *pmmpthread_get_property_ptr_ptr_stub(zend_object *object, zend_string *mem /* {{{ */ zval* pmmpthread_read_dimension(PMMPTHREAD_READ_DIMENSION_PASSTHRU_D) { - if (pmmpthread_store_read(object, member, type, rv) == FAILURE) { + if (pmmpthread_store_read(object, member, NULL, type, rv) == FAILURE) { //TODO: this ought to generate warnings, but this is a pain right now due to key type juggling //for now this maintains the v4 behaviour of silently generating NULL, which is better than segfaulting if (!EG(exception)) { @@ -81,39 +81,27 @@ zval* pmmpthread_read_dimension(PMMPTHREAD_READ_DIMENSION_PASSTHRU_D) { zval* pmmpthread_read_property(PMMPTHREAD_READ_PROPERTY_PASSTHRU_D) { zval zmember; - zend_guard* guard; - - ZVAL_STR(&zmember, member); + zval result; - if (object->ce->__get && (guard = zend_get_property_guard(object, member)) && !((*guard) & IN_GET)) { - (*guard) |= IN_GET; - zend_call_known_instance_method_with_1_params(object->ce->__get, object, rv, &zmember); - (*guard) &= ~IN_GET; + zend_property_info* info = zend_get_property_info(object->ce, member, 0); + if (info != NULL && info != ZEND_WRONG_PROPERTY_INFO) { + ZVAL_STR(&zmember, info->name); } else { - zend_property_info* info = zend_get_property_info(object->ce, member, 0); - if (info == ZEND_WRONG_PROPERTY_INFO) { - rv = &EG(uninitialized_zval); - } else if (info == NULL || !PMMPTHREAD_OBJECT_PROPERTY(info)) { //dynamic property - if (pmmpthread_store_read(object, &zmember, type, rv) == FAILURE) { - if (type != BP_VAR_IS) { - zend_error(E_WARNING, "Undefined property: %s::$%s", ZSTR_VAL(object->ce->name), ZSTR_VAL(member)); - } - rv = &EG(uninitialized_zval); - } - } else { - //defined property, use mangled name - ZVAL_STR(&zmember, info->name); - - if (pmmpthread_store_read(object, &zmember, type, rv) == FAILURE) { - if (type != BP_VAR_IS && !EG(exception)) { - zend_throw_error(NULL, "Typed property %s::$%s must not be accessed before initialization", - ZSTR_VAL(info->ce->name), - ZSTR_VAL(member)); - } - rv = &EG(uninitialized_zval); - } + ZVAL_STR(&zmember, member); + } + //this moves the value to cache for zend_std_read_property() to work on + pmmpthread_store_read_ex(object, &zmember, info, type, rv, 1); + if (EG(exception)) { + rv = &EG(uninitialized_zval); + } else { + //no cache for now - we don't want the VM bypassing this handler + zend_std_read_property(object, member, type, NULL, rv); + //tidy property cache so we don't read wrong values later + if (!pmmpthread_store_retain_in_local_cache(rv)) { + pmmpthread_store_clean_local_property(object, &zmember, info); } } + return rv; } /* }}} */ @@ -129,7 +117,7 @@ zval* pmmpthread_read_property_deny(PMMPTHREAD_READ_PROPERTY_PASSTHRU_D) { /* {{{ */ void pmmpthread_write_dimension(PMMPTHREAD_WRITE_DIMENSION_PASSTHRU_D) { - if (pmmpthread_store_write(object, member, value, PMMPTHREAD_STORE_NO_COERCE_ARRAY) == FAILURE && !EG(exception)){ + if (pmmpthread_store_write(object, member, NULL, value, PMMPTHREAD_STORE_NO_COERCE_ARRAY) == FAILURE && !EG(exception)){ zend_throw_error( pmmpthread_ce_nts_value_error, "Cannot assign non-thread-safe value of type %s to %s", @@ -144,43 +132,27 @@ zval* pmmpthread_write_property(PMMPTHREAD_WRITE_PROPERTY_PASSTHRU_D) { zval tmp; zend_guard* guard; - ZVAL_STR(&zmember, member); - ZVAL_UNDEF(&tmp); - - if (object->ce->__set && (guard = zend_get_property_guard(object, member)) && !((*guard) & IN_SET)) { - zval rv; - ZVAL_UNDEF(&rv); + //no cache for now - cache would allow the VM to bypass this handler + //std_write may coerce the var to a different type, so we need to use the result + value = zend_std_write_property(object, member, value, NULL); - (*guard) |= IN_SET; - zend_call_known_instance_method_with_2_params(object->ce->__set, object, &rv, &zmember, value); - (*guard) &= ~IN_SET; - - if (Z_TYPE(rv) != IS_UNDEF) - zval_dtor(&rv); - } else { - bool ok = true; + if (value != &EG(error_zval)) { + zval* real_value = NULL; + zval zmember; + ZVAL_UNDEF(&zmember); zend_property_info* info = zend_get_property_info(object->ce, member, 0); - if (info != ZEND_WRONG_PROPERTY_INFO) { - if (info != NULL && PMMPTHREAD_OBJECT_PROPERTY(info)) { - ZVAL_STR(&zmember, info->name); //use mangled name to avoid private member shadowing issues - - zend_execute_data* execute_data = EG(current_execute_data); - bool strict = execute_data - && execute_data->func - && ZEND_CALL_USES_STRICT_TYPES(EG(current_execute_data)); - - //zend_verify_property_type() might modify the value - //value is not copied before we receive it, so it might be - //from opcache protected memory which we can't modify - ZVAL_COPY(&tmp, value); - value = &tmp; - - if (ZEND_TYPE_IS_SET(info->type) && !zend_verify_property_type(info, value, strict)) { - ok = false; - } + if (info != NULL) { + if (info != ZEND_WRONG_PROPERTY_INFO) { + ZVAL_STR(&zmember, info->name); + real_value = OBJ_PROP(object, info->offset); } - - if (ok && pmmpthread_store_write(object, &zmember, value, PMMPTHREAD_STORE_NO_COERCE_ARRAY) == FAILURE && !EG(exception)) { + } else if (object->properties != NULL) { + ZVAL_STR(&zmember, member); + real_value = zend_hash_find(object->properties, member); + } + if (real_value != NULL) { + zend_bool cached = 0; + if (pmmpthread_store_write_ex(object, &zmember, info, real_value, PMMPTHREAD_STORE_NO_COERCE_ARRAY, &cached) == FAILURE && !EG(exception)) { zend_throw_error( pmmpthread_ce_nts_value_error, "Cannot assign non-thread-safe value of type %s to thread-safe class property %s::$%s", @@ -188,12 +160,14 @@ zval* pmmpthread_write_property(PMMPTHREAD_WRITE_PROPERTY_PASSTHRU_D) { ZSTR_VAL(object->ce->name), ZSTR_VAL(member) ); + value = &EG(error_zval); + } + if (!cached) { + pmmpthread_store_clean_local_property(object, &zmember, info); } } } - zval_ptr_dtor(&tmp); - return EG(exception) ? &EG(error_zval) : value; } /* }}} */ @@ -249,34 +223,21 @@ int pmmpthread_has_property_deny(PMMPTHREAD_HAS_PROPERTY_PASSTHRU_D) { /* {{{ */ void pmmpthread_unset_dimension(PMMPTHREAD_UNSET_DIMENSION_PASSTHRU_D) { - pmmpthread_store_delete(object, member); + pmmpthread_store_delete(object, member, NULL); } void pmmpthread_unset_property(PMMPTHREAD_UNSET_PROPERTY_PASSTHRU_D) { zval zmember; - zend_guard* guard; - - ZVAL_STR(&zmember, member); - - if (object->ce->__unset && (guard = zend_get_property_guard(object, member)) && !((*guard) & IN_UNSET)) { - zval rv; - ZVAL_UNDEF(&rv); - - (*guard) |= IN_UNSET; - zend_call_known_instance_method_with_1_params(object->ce->__unset, object, &rv, &zmember); - (*guard) &= ~IN_UNSET; - - if (Z_TYPE(rv) != IS_UNDEF) { - zval_dtor(&rv); - } - } else { + + zend_std_unset_property(object, member, NULL); + if (!EG(exception)) { zend_property_info* info = zend_get_property_info(object->ce, member, 0); - if (info != ZEND_WRONG_PROPERTY_INFO) { - if (info != NULL && PMMPTHREAD_OBJECT_PROPERTY(info)) { - ZVAL_STR(&zmember, info->name); //defined property, use mangled name - } - pmmpthread_store_delete(object, &zmember); + if (info != NULL && info != ZEND_WRONG_PROPERTY_INFO) { + ZVAL_STR(&zmember, info->name); + } else { + ZVAL_STR(&zmember, member); } + pmmpthread_store_delete(object, &zmember, info); } } /* }}} */ diff --git a/src/object.c b/src/object.c index 0b9dac98..b46b3cd3 100644 --- a/src/object.c +++ b/src/object.c @@ -258,7 +258,9 @@ static inline void pmmpthread_base_write_property_defaults(pmmpthread_zend_objec value = OBJ_PROP(&base->std, info->offset); if (!Z_ISUNDEF_P(value)) { result = pmmpthread_store_write( - &base->std, &key, + &base->std, + &key, + info, value, PMMPTHREAD_STORE_NO_COERCE_ARRAY ); diff --git a/src/routine.c b/src/routine.c index 8cca1f9c..2c485532 100644 --- a/src/routine.c +++ b/src/routine.c @@ -242,7 +242,7 @@ zend_bool pmmpthread_join(pmmpthread_zend_object_t* thread) { //now, synchronize all object properties that may have been assigned by the thread if (pmmpthread_monitor_lock(&thread->ts_obj->monitor)) { - pmmpthread_store_full_sync_local_properties(&thread->std); + pmmpthread_store_cache_all(&thread->std); pmmpthread_monitor_unlock(&thread->ts_obj->monitor); } if (thread->worker_data != NULL) { @@ -250,7 +250,7 @@ zend_bool pmmpthread_join(pmmpthread_zend_object_t* thread) { } pmmpthread_zend_object_t* user_globals = PMMPTHREAD_ZG(thread_shared_globals); if (pmmpthread_monitor_lock(&user_globals->ts_obj->monitor)) { - pmmpthread_store_full_sync_local_properties(&user_globals->std); + pmmpthread_store_cache_all(&user_globals->std); pmmpthread_monitor_unlock(&user_globals->ts_obj->monitor); } diff --git a/src/store.c b/src/store.c index 002501c2..1fbbf893 100644 --- a/src/store.c +++ b/src/store.c @@ -68,7 +68,50 @@ static void pmmpthread_store_init_local_properties(zend_object* object) { } } /* }}} */ -void pmmpthread_store_sync_local_properties(zend_object* object) { /* {{{ */ +static bool cache_item_stale(const zval* val, const pmmpthread_storage* ts_val) { + zend_bool remove = 1; + if (ts_val) { + ZVAL_DEINDIRECT(val); + if (ts_val->type == STORE_TYPE_THREADSAFE_OBJECT && IS_THREADSAFE_CLASS_INSTANCE(val)) { + pmmpthread_zend_object_t* shared = ((pmmpthread_zend_object_storage_t*)ts_val)->object; + pmmpthread_zend_object_t* local = PMMPTHREAD_FETCH_FROM(Z_OBJ_P(val)); + + if ( + shared == local || //same object + (pmmpthread_globals_object_valid(shared) && shared->ts_obj == local->ts_obj) //connection to a valid foreign object + ) { + remove = 0; + } + } else if (ts_val->type == STORE_TYPE_CLOSURE && IS_CLOSURE_OBJECT(val)) { + zend_closure* shared = ((pmmpthread_closure_storage_t*)ts_val)->closure; + zend_closure* local = (zend_closure*)Z_OBJ_P(val); + if (shared == local) { + remove = 0; + } +#if HAVE_PMMPTHREAD_EXT_SOCKETS_SUPPORT + } else if (ts_val->type == STORE_TYPE_SOCKET && IS_EXT_SOCKETS_OBJECT(val)) { + pmmpthread_socket_storage_t* shared = (pmmpthread_socket_storage_t*)ts_val; + php_socket* local = Z_SOCKET_P(val); + if (shared->bsd_socket == local->bsd_socket) { + remove = 0; + } +#endif + } else if (ts_val->type == STORE_TYPE_STRING_PTR && Z_TYPE_P(val) == IS_STRING) { + pmmpthread_string_storage_t* string = (pmmpthread_string_storage_t*)ts_val; + if (string->owner.ls == TSRMLS_CACHE && string->string == Z_STR_P(val)) { + //local caching of this by other threads is probably fine too, but fully caching it would probably + //require bytewise comparison, which isn't gonna be very performant + //it should be sufficient to only persist the owner thread's ref, since that's where copies will be + //made from anyway. + remove = 0; + } + } + } + + return remove; +} + +void pmmpthread_store_clean_stale_cache(zend_object* object) { /* {{{ */ pmmpthread_zend_object_t *threaded = PMMPTHREAD_FETCH_FROM(object); pmmpthread_object_t *ts_obj = threaded->ts_obj; zend_ulong idx; @@ -81,77 +124,46 @@ void pmmpthread_store_sync_local_properties(zend_object* object) { /* {{{ */ return; } + for (int prop_index = 0; prop_index < object->ce->default_properties_count; prop_index++) { + zend_property_info* prop_info = object->ce->properties_info_table[prop_index]; + if (!prop_info) { + continue; + } + + ts_val = TRY_PMMPTHREAD_STORAGE_PTR_P(zend_hash_find(&ts_obj->props.hash, prop_info->name)); + val = OBJ_PROP(object, prop_info->offset); + + if (cache_item_stale(val, ts_val)) { + zval_ptr_dtor(val); + ZVAL_UNDEF(val); + } + } + if (threaded->std.properties) { ZEND_HASH_FOREACH_KEY_VAL(threaded->std.properties, idx, name, val) { + if (Z_TYPE_P(val) == IS_INDIRECT) { + //ptr to known property, don't mess with it + continue; + } if (!name) { ts_val = TRY_PMMPTHREAD_STORAGE_PTR_P(zend_hash_index_find(&ts_obj->props.hash, idx)); } else { ts_val = TRY_PMMPTHREAD_STORAGE_PTR_P(zend_hash_find(&ts_obj->props.hash, name)); } - remove = 1; - if (ts_val) { - ZVAL_DEINDIRECT(val); - if (ts_val->type == STORE_TYPE_THREADSAFE_OBJECT && IS_THREADSAFE_CLASS_INSTANCE(val)) { - pmmpthread_zend_object_t* shared = ((pmmpthread_zend_object_storage_t*)ts_val)->object; - pmmpthread_zend_object_t* local = PMMPTHREAD_FETCH_FROM(Z_OBJ_P(val)); - - if ( - shared == local || //same object - (pmmpthread_globals_object_valid(shared) && shared->ts_obj == local->ts_obj) //connection to a valid foreign object - ) { - remove = 0; - } - } else if (ts_val->type == STORE_TYPE_CLOSURE && IS_CLOSURE_OBJECT(val)) { - zend_closure* shared = ((pmmpthread_closure_storage_t*)ts_val)->closure; - zend_closure* local = (zend_closure*)Z_OBJ_P(val); - if (shared == local) { - remove = 0; - } -#if HAVE_PMMPTHREAD_EXT_SOCKETS_SUPPORT - } else if (ts_val->type == STORE_TYPE_SOCKET && IS_EXT_SOCKETS_OBJECT(val)) { - pmmpthread_socket_storage_t* shared = (pmmpthread_socket_storage_t*)ts_val; - php_socket* local = Z_SOCKET_P(val); - if (shared->bsd_socket == local->bsd_socket) { - remove = 0; - } -#endif - } else if (ts_val->type == STORE_TYPE_STRING_PTR && Z_TYPE_P(val) == IS_STRING) { - pmmpthread_string_storage_t* string = (pmmpthread_string_storage_t*)ts_val; - if (string->owner.ls == TSRMLS_CACHE && string->string == Z_STR_P(val)) { - //local caching of this by other threads is probably fine too, but fully caching it would probably - //require bytewise comparison, which isn't gonna be very performant - //it should be sufficient to only persist the owner thread's ref, since that's where copies will be - //made from anyway. - remove = 0; - } - } - } - - if (remove) { + if (cache_item_stale(val, ts_val)) { if (!name) { zend_hash_index_del(threaded->std.properties, idx); - } - else { + } else { zend_hash_del(threaded->std.properties, name); } } } ZEND_HASH_FOREACH_END(); - HT_FLAGS(threaded->std.properties) &= ~HASH_FLAG_HAS_EMPTY_IND; } + threaded->local_props_modcount = ts_obj->props.modcount; } /* }}} */ -static inline zend_bool pmmpthread_store_retain_in_local_cache(zval* val) { - return IS_THREADSAFE_CLASS_INSTANCE(val) || IS_CLOSURE_OBJECT(val) || IS_EXT_SOCKETS_OBJECT(val) || Z_TYPE_P(val) == IS_STRING; -} - -static inline zend_bool pmmpthread_store_valid_local_cache_item(zval* val) { - //zend_std_get_properties_ex() may add IS_INDIRECT zvals to point to the linear property table - //we don't want that, because they aren't used by pmmpthread and are always uninitialized - return Z_TYPE_P(val) != IS_INDIRECT; -} - /* {{{ */ static inline zend_bool pmmpthread_store_storage_is_cacheable(zval* zstorage) { if (zstorage == NULL) { @@ -172,7 +184,7 @@ static inline zend_bool pmmpthread_store_storage_is_pmmpthread_obj(zval* zstorag } /* }}} */ /* {{{ Syncs all the cacheable properties from TS storage into local cache */ -void pmmpthread_store_full_sync_local_properties(zend_object *object) { +void pmmpthread_store_cache_all(zend_object *object) { pmmpthread_zend_object_t* threaded = PMMPTHREAD_FETCH_FROM(object); if (GC_IS_RECURSIVE(&threaded->std)) { @@ -184,47 +196,70 @@ void pmmpthread_store_full_sync_local_properties(zend_object *object) { } GC_PROTECT_RECURSION(&threaded->std); - pmmpthread_store_sync_local_properties(object); //remove any outdated cache elements + pmmpthread_store_clean_stale_cache(object); //remove any unwanted cache elements + + //TODO: we might be able to avoid initializing this if there are no dynamic properties + pmmpthread_store_init_local_properties(object); pmmpthread_object_t* ts_obj = threaded->ts_obj; zend_long idx; zend_string* name; zval* zstorage; + zval* cached; - pmmpthread_store_init_local_properties(&threaded->std); + for (int prop_index = 0; prop_index < object->ce->default_properties_count; prop_index++) { + zend_property_info* prop_info = object->ce->properties_info_table[prop_index]; + if (!prop_info || !PMMPTHREAD_OBJECT_PROPERTY(prop_info)) { + continue; + } + + zstorage = zend_hash_find(&ts_obj->props.hash, prop_info->name); + cached = OBJ_PROP(object, prop_info->offset); + if (Z_ISUNDEF_P(cached) && pmmpthread_store_storage_is_cacheable(zstorage)) { + pmmpthread_store_restore_zval(cached, zstorage); + } + + if (pmmpthread_store_storage_is_pmmpthread_obj(zstorage)) { + pmmpthread_store_cache_all(Z_OBJ_P(cached)); + } + } ZEND_HASH_FOREACH_KEY_VAL(&ts_obj->props.hash, idx, name, zstorage) { - zval* cached; zval pzval; + ZVAL_UNDEF(&pzval); //we just synced local cache, so if something is already here, it doesn't need to be modified - if (!name) { - cached = zend_hash_index_find(threaded->std.properties, idx); - } else { - cached = zend_hash_find(threaded->std.properties, name); - } - if (cached && pmmpthread_store_valid_local_cache_item(cached)) { - if (pmmpthread_store_storage_is_pmmpthread_obj(zstorage)) { - pmmpthread_store_full_sync_local_properties(Z_OBJ_P(cached)); + cached = name ? + zend_hash_find(threaded->std.properties, name) : + zend_hash_index_find(threaded->std.properties, idx); + + if (cached) { + if (Z_TYPE_P(cached) == IS_INDIRECT) { + //ptr to known property which we've already processed + continue; } + //we still need to call store_cache_all() on thread-safe objects even if cached + } else if (pmmpthread_store_storage_is_cacheable(zstorage)) { + pmmpthread_store_restore_zval(&pzval, zstorage); + cached = &pzval; + } else { + //not cached or cacheable continue; } - if (pmmpthread_store_storage_is_cacheable(zstorage)) { - pmmpthread_store_restore_zval(&pzval, zstorage); - if (pmmpthread_store_storage_is_pmmpthread_obj(zstorage)) { - pmmpthread_store_full_sync_local_properties(Z_OBJ(pzval)); - } + if (pmmpthread_store_storage_is_pmmpthread_obj(zstorage)) { + pmmpthread_store_cache_all(Z_OBJ_P(cached)); + } - if (!name) { - if (!zend_hash_index_update(threaded->std.properties, idx, &pzval)) { - zval_ptr_dtor(&pzval); - } - } else { + if (!Z_ISUNDEF(pzval)) { + zval* update = name ? /* we can't use zend_hash_update() here - the string from store.props must not be returned to user code */ - if (!zend_hash_str_update(threaded->std.properties, ZSTR_VAL(name), ZSTR_LEN(name), &pzval)) - zval_ptr_dtor(&pzval); + zend_hash_str_update(threaded->std.properties, ZSTR_VAL(name), ZSTR_LEN(name), &pzval) : + zend_hash_index_update(threaded->std.properties, idx, &pzval); + + if (!update) { + zval_ptr_dtor(&pzval); } } } ZEND_HASH_FOREACH_END(); @@ -280,8 +315,35 @@ static inline zend_bool pmmpthread_store_member_is_cacheable(zend_object *object return pmmpthread_store_storage_is_cacheable(zstorage); } /* }}} */ + +void pmmpthread_store_clean_local_property(zend_object* object, zval* key, zend_property_info* prop_info) { + zval* property; + if (prop_info != NULL) { + property = OBJ_PROP(object, prop_info->offset); + zval_ptr_dtor(property); + ZVAL_UNDEF(property); + } else if (object->properties != NULL) { + if (Z_TYPE_P(key) == IS_LONG) { + zend_hash_index_del(object->properties, Z_LVAL_P(key)); + } else { + zval* property = zend_hash_find(object->properties, Z_STR_P(key)); + if (property) { + if (Z_TYPE_P(property) == IS_INDIRECT) { + //known property, but we don't have prop_info + property = Z_INDIRECT_P(property); + zval_ptr_dtor(property); + ZVAL_UNDEF(property); + } else { + //dynamic property + zend_hash_del(object->properties, Z_STR_P(key)); + } + } + } + } +} + /* {{{ */ -int pmmpthread_store_delete(zend_object *object, zval *key) { +int pmmpthread_store_delete(zend_object *object, zval *key, zend_property_info* prop_info) { int result = FAILURE; zval member; pmmpthread_zend_object_t *threaded = PMMPTHREAD_FETCH_FROM(object); @@ -304,10 +366,8 @@ int pmmpthread_store_delete(zend_object *object, zval *key) { pmmpthread_monitor_unlock(&ts_obj->monitor); } else result = FAILURE; - if (result == SUCCESS && threaded->std.properties) { - if (Z_TYPE(member) == IS_LONG) { - zend_hash_index_del(threaded->std.properties, Z_LVAL(member)); - } else zend_hash_del(threaded->std.properties, Z_STR(member)); + if (result == SUCCESS) { + pmmpthread_store_clean_local_property(object, &member, prop_info); } if (coerced) @@ -389,23 +449,33 @@ zend_bool pmmpthread_store_isset(zend_object *object, zval *key, int has_set_exi return isset; } /* }}} */ -static inline void pmmpthread_store_update_local_property(zend_object* object, zval* key, zval* value) { - if (pmmpthread_store_retain_in_local_cache(value)) { +static inline void pmmpthread_store_update_local_property(zend_object* object, zval* key, zend_property_info* prop_info, zval* value) { + Z_TRY_ADDREF_P(value); + + zval* property; + if (prop_info != NULL && prop_info != ZEND_WRONG_PROPERTY_INFO) { + property = OBJ_PROP(object, prop_info->offset); + zval_ptr_dtor(property); + ZVAL_COPY_VALUE(property, value); + } else { pmmpthread_store_init_local_properties(object); if (Z_TYPE_P(key) == IS_LONG) { zend_hash_index_update(object->properties, Z_LVAL_P(key), value); } else { zend_string* str_key = Z_STR_P(key); - if ((GC_FLAGS(str_key) & (IS_STR_PERSISTENT|IS_STR_INTERNED)) == IS_STR_PERSISTENT) { - //refcounted persistent string from pmmpthread_store - we can't use it directly - //if a bucket with this key already exists, it'll be reused - zend_hash_str_update(object->properties, Z_STRVAL_P(key), Z_STRLEN_P(key), value); - } else { - //any other interned or emalloc'd strings should be safe to use directly here - zend_hash_update(object->properties, str_key, value); + property = zend_hash_find(object->properties, str_key); + if (property != value) { + //update_ind() ensures defined properties get properly updated even if we didn't have prop_info + if ((GC_FLAGS(str_key) & (IS_STR_PERSISTENT | IS_STR_INTERNED)) == IS_STR_PERSISTENT) { + //refcounted persistent string from pmmpthread_store - we can't use it directly + //if a bucket with this key already exists, it'll be reused + zend_hash_str_update_ind(object->properties, Z_STRVAL_P(key), Z_STRLEN_P(key), value); + } else { + //any other interned or emalloc'd strings should be safe to use directly here + zend_hash_update_ind(object->properties, str_key, value); + } } } - Z_TRY_ADDREF_P(value); } } @@ -433,7 +503,7 @@ static inline zend_bool pmmpthread_store_update_shared_property(pmmpthread_objec } /* {{{ */ -int pmmpthread_store_read(zend_object *object, zval *key, int type, zval *read) { +int pmmpthread_store_read_ex(zend_object *object, zval *key, zend_property_info *prop_info, int type, zval *read, zend_bool force_cache) { int result = FAILURE; zval member, *property = NULL; pmmpthread_zend_object_t *threaded = PMMPTHREAD_FETCH_FROM(object); @@ -441,17 +511,22 @@ int pmmpthread_store_read(zend_object *object, zval *key, int type, zval *read) zend_bool coerced = pmmpthread_store_coerce(key, &member); if (pmmpthread_monitor_lock(&ts_obj->monitor)) { - if (threaded->std.properties) { - pmmpthread_store_sync_local_properties(object); + if (prop_info != NULL || threaded->std.properties) { + pmmpthread_store_clean_stale_cache(object); /* check if there's still a ref in local cache after sync - this ensures ref reuse for ThreadSafe and Closure objects */ - if (Z_TYPE(member) == IS_LONG) { - property = zend_hash_index_find(threaded->std.properties, Z_LVAL(member)); - } else property = zend_hash_find(threaded->std.properties, Z_STR(member)); + if (prop_info) { + property = OBJ_PROP(object, prop_info->offset); + } else { + if (Z_TYPE(member) == IS_LONG) { + property = zend_hash_index_find(threaded->std.properties, Z_LVAL(member)); + } else property = zend_hash_find(threaded->std.properties, Z_STR(member)); + } - if (property && pmmpthread_store_valid_local_cache_item(property)) { + if (property && !Z_ISUNDEF_P(property)) { pmmpthread_monitor_unlock(&ts_obj->monitor); + //even if we don't have prop_info, this might be a known property ZVAL_DEINDIRECT(property); ZVAL_COPY(read, property); if (coerced) { @@ -481,8 +556,9 @@ int pmmpthread_store_read(zend_object *object, zval *key, int type, zval *read) if (result != SUCCESS) { ZVAL_UNDEF(read); - } else { - pmmpthread_store_update_local_property(&threaded->std, &member, read); + } else if (force_cache || pmmpthread_store_retain_in_local_cache(read)) { + //always cache this - we'll need it for std_read handler + pmmpthread_store_update_local_property(&threaded->std, &member, prop_info, read); } if (coerced) @@ -491,6 +567,10 @@ int pmmpthread_store_read(zend_object *object, zval *key, int type, zval *read) return result; } /* }}} */ +int pmmpthread_store_read(zend_object* object, zval* key, zend_property_info* prop_info, int type, zval* read) { + return pmmpthread_store_read_ex(object, key, prop_info, type, read, 0); +} + /* {{{ Copies strings (as needed) for use in thread-safe object tables */ static zend_string* pmmpthread_store_save_string(zend_string* string) { zend_string* result; @@ -516,7 +596,7 @@ static zend_string* pmmpthread_store_restore_string(zend_string* string) { } /* {{{ */ -int pmmpthread_store_write(zend_object *object, zval *key, zval *write, zend_bool coerce_array_to_threaded) { +int pmmpthread_store_write_ex(zend_object *object, zval *key, zend_property_info* prop_info, zval *write, zend_bool coerce_array_to_threaded, zend_bool *cached) { int result = FAILURE; zval vol, member, zstorage; pmmpthread_zend_object_t *threaded = @@ -524,6 +604,8 @@ int pmmpthread_store_write(zend_object *object, zval *key, zval *write, zend_boo pmmpthread_object_t *ts_obj = threaded->ts_obj; zend_bool coerced = 0; + *cached = 0; + if (Z_TYPE_P(write) == IS_ARRAY && coerce_array_to_threaded == PMMPTHREAD_STORE_COERCE_ARRAY) { /* coerce arrays into threaded objects */ object_init_ex( @@ -572,15 +654,16 @@ int pmmpthread_store_write(zend_object *object, zval *key, zval *write, zend_boo } //this isn't necessary for any specific property write, but since we don't have any other way to clean up local //cached ThreadSafe references that are dead, we have to take the opportunity - pmmpthread_store_sync_local_properties(object); + pmmpthread_store_clean_stale_cache(object); pmmpthread_monitor_unlock(&ts_obj->monitor); } if (result != SUCCESS) { pmmpthread_store_storage_dtor(&zstorage); - } else { - pmmpthread_store_update_local_property(&threaded->std, &member, write); + } else if (pmmpthread_store_retain_in_local_cache(write)) { + *cached = 1; + pmmpthread_store_update_local_property(&threaded->std, &member, prop_info, write); } if (coerced) @@ -589,6 +672,11 @@ int pmmpthread_store_write(zend_object *object, zval *key, zval *write, zend_boo return result; } /* }}} */ +int pmmpthread_store_write(zend_object* object, zval* key, zend_property_info* prop_info, zval* write, zend_bool coerce_array_to_threaded) { + zend_bool dummy = 0; + return pmmpthread_store_write_ex(object, key, prop_info, write, coerce_array_to_threaded, &dummy); +} + /* {{{ */ int pmmpthread_store_count(zend_object *object, zend_long *count) { pmmpthread_object_t* ts_obj = PMMPTHREAD_FETCH_TS_FROM(object); @@ -637,6 +725,8 @@ int pmmpthread_store_shift(zend_object *object, zval *member) { if (threaded->std.properties) { zend_hash_del(threaded->std.properties, Z_STR(key)); } + //TODO: this should also clear defined properties + //it's probably fine not to do this for now since shift() is exclusive to arrays? zend_string_release(Z_STR(key)); } @@ -701,6 +791,8 @@ int pmmpthread_store_chunk(zend_object *object, zend_long size, zend_bool preser if (threaded->std.properties) { zend_hash_del(threaded->std.properties, Z_STR(key)); } + //TODO: this should also clear defined properties + //it's probably fine not to do this for now since shift() is exclusive to arrays? zend_string_release(Z_STR(key)); } } @@ -754,6 +846,8 @@ int pmmpthread_store_pop(zend_object *object, zval *member) { if (threaded->std.properties) { zend_hash_del(threaded->std.properties, Z_STR(key)); } + //TODO: this should also clear defined properties + //it's probably fine not to do this for now since shift() is exclusive to arrays? zend_string_release(Z_STR(key)); } if (may_be_locally_cached) { @@ -781,77 +875,67 @@ void pmmpthread_store_tohash(zend_object *object, HashTable *hash) { zend_string *name = NULL; zend_ulong idx; zval *zstorage; - zend_bool changed = 0; - pmmpthread_store_sync_local_properties(object); + pmmpthread_store_clean_stale_cache(object); + for (int i = 0; i < object->ce->default_properties_count; i++) { + zend_property_info* info = object->ce->properties_info_table[i]; + if (info == NULL || !PMMPTHREAD_OBJECT_PROPERTY(info)) { + continue; + } - ZEND_HASH_FOREACH_KEY_VAL(&ts_obj->props.hash, idx, name, zstorage) { - zval *cached; zval pzval; - - //we just synced local cache, so if something is already here, it doesn't need to be modified - if (hash == threaded->std.properties) { - if (!name) { - cached = zend_hash_index_find(threaded->std.properties, idx); - } else { - cached = zend_hash_find(threaded->std.properties, name); - } - if (cached && pmmpthread_store_valid_local_cache_item(cached)) { - continue; - } - } else { - if (!name) { - cached = zend_hash_index_find(threaded->std.properties, idx); - if (cached && pmmpthread_store_valid_local_cache_item(cached)) { - zend_hash_index_update(hash, idx, cached); - Z_TRY_ADDREF_P(cached); - continue; - } + zval zname; + ZVAL_STR(&zname, info->name); + pmmpthread_store_read_ex(object, &zname, info, BP_VAR_R, &pzval, hash == object->properties); + + //indirections should already exist if this is the normal property table + if (hash != object->properties) { + if (Z_ISUNDEF(pzval)) { + ZVAL_INDIRECT(&pzval, OBJ_PROP(object, info->offset)); + zend_hash_update(hash, info->name, &pzval); + HT_FLAGS(hash) |= HASH_FLAG_HAS_EMPTY_IND; } else { - cached = zend_hash_find(threaded->std.properties, name); - if (cached && pmmpthread_store_valid_local_cache_item(cached)) { - /* we can't use zend_hash_update() here - the string from store.props must not be returned to user code */ - zend_hash_str_update(hash, ZSTR_VAL(name), ZSTR_LEN(name), cached); - Z_TRY_ADDREF_P(cached); - continue; - } + Z_TRY_ADDREF(pzval); + zend_hash_update(hash, info->name, &pzval); } } + } - pmmpthread_store_restore_zval(&pzval, zstorage); + ZEND_HASH_FOREACH_KEY_VAL(&ts_obj->props.hash, idx, name, zstorage) { + zval pzval; + zval* existing; - if (!name) { - if (!zend_hash_index_update(hash, idx, &pzval)) { - zval_ptr_dtor(&pzval); - } + zval key; + if (name) { + ZVAL_STR(&key, name); + existing = zend_hash_find(hash, name); } else { - /* we can't use zend_hash_update() here - the string from store.props must not be returned to user code */ - if (!zend_hash_str_update(hash, ZSTR_VAL(name), ZSTR_LEN(name), &pzval)) - zval_ptr_dtor(&pzval); + ZVAL_LONG(&key, idx); + existing = zend_hash_index_find(hash, idx); } - changed = 1; - } ZEND_HASH_FOREACH_END(); - - for (int i = 0; i < object->ce->default_properties_count; i++) { - zend_property_info* info = object->ce->properties_info_table[i]; - if (info == NULL || !PMMPTHREAD_OBJECT_PROPERTY(info)) { + if (existing) { + //either this is a new hash, or clean cache, so if an element already exists, + //don't mess with it continue; } - zval pzval; - if (zend_hash_find(hash, info->name) == NULL) { - //uninitialized typed property - ZVAL_INDIRECT(&pzval, &object->properties_table[OBJ_PROP_TO_NUM(info->offset)]); - zend_hash_update(hash, info->name, &pzval); - HT_FLAGS(hash) |= HASH_FLAG_HAS_EMPTY_IND; - changed = 1; + pmmpthread_store_read_ex(object, &key, NULL, BP_VAR_R, &pzval, hash == object->properties); + if (hash != object->properties) { + Z_TRY_ADDREF(pzval); + if (name) { + /* we can't use zend_hash_update() here - the string from store.props must not be returned to user code */ + zend_hash_str_update(hash, ZSTR_VAL(name), ZSTR_LEN(name), &pzval); + } else { + zend_hash_index_update(hash, idx, &pzval); + } } - } + } ZEND_HASH_FOREACH_END(); - if (changed && hash == threaded->std.properties) { + if (hash == threaded->std.properties) { //if this is the object's own properties table, we need to ensure that junk added here //doesn't get incorrectly treated as gospel + //TODO: we should use get_properties_for() so we don't have to deal with this bullshit threaded->local_props_modcount = ts_obj->props.modcount - 1; } @@ -1248,6 +1332,8 @@ int pmmpthread_store_merge(zend_object *destination, zval *from, zend_bool overw zend_hash_index_del(destination->properties, Z_LVAL(key)); } else { zend_hash_del(destination->properties, Z_STR(key)); + //TODO: this should also clear defined properties + //it's probably fine not to do this for now since this operation is exclusive to arrays? } } } @@ -1297,7 +1383,7 @@ int pmmpthread_store_merge(zend_object *destination, zval *from, zend_bool overw if (!overwrite && zend_hash_index_exists(&ts_obj->props.hash, Z_LVAL(key))) { goto next; } - if (pmmpthread_store_write(destination, &key, pzval, coerce_array_to_threaded) == FAILURE) { + if (pmmpthread_store_write(destination, &key, NULL, pzval, coerce_array_to_threaded) == FAILURE) { zend_throw_error( pmmpthread_ce_nts_value_error, "Cannot merge non-thread-safe value of type %s (input key %zd) into %s", @@ -1312,7 +1398,7 @@ int pmmpthread_store_merge(zend_object *destination, zval *from, zend_bool overw if (!overwrite && zend_hash_exists(&ts_obj->props.hash, Z_STR(key))) { goto next; } - if (pmmpthread_store_write(destination, &key, pzval, coerce_array_to_threaded) == FAILURE) { + if (pmmpthread_store_write(destination, &key, NULL, pzval, coerce_array_to_threaded) == FAILURE) { zend_throw_error( pmmpthread_ce_nts_value_error, "Cannot merge non-thread-safe value of type %s (input key \"%s\") into %s", @@ -1416,7 +1502,7 @@ void pmmpthread_store_data(zend_object *object, zval *value, HashPosition *posit zval key; zend_hash_get_current_key_zval_ex(&ts_obj->props.hash, &key, position); - if (pmmpthread_store_read(object, &key, BP_VAR_R, value) == FAILURE) { + if (pmmpthread_store_read(object, &key, NULL, BP_VAR_R, value) == FAILURE) { ZVAL_UNDEF(value); } if (Z_TYPE(key) == IS_STRING) { diff --git a/src/store.h b/src/store.h index 9eb12ead..0f2acd36 100644 --- a/src/store.h +++ b/src/store.h @@ -38,13 +38,16 @@ typedef struct _pmmpthread_store_t { void pmmpthread_store_init(pmmpthread_store_t* store); void pmmpthread_store_destroy(pmmpthread_store_t* store); -void pmmpthread_store_sync_local_properties(zend_object* object); -void pmmpthread_store_full_sync_local_properties(zend_object *object); +void pmmpthread_store_clean_stale_cache(zend_object* object); +void pmmpthread_store_cache_all(zend_object *object); int pmmpthread_store_merge(zend_object *destination, zval *from, zend_bool overwrite, zend_bool coerce_array_to_threaded); -int pmmpthread_store_delete(zend_object *object, zval *key); -int pmmpthread_store_read(zend_object *object, zval *key, int type, zval *read); +void pmmpthread_store_clean_local_property(zend_object* object, zval* key, zend_property_info* prop_info); +int pmmpthread_store_delete(zend_object *object, zval *key, zend_property_info* prop_info); +int pmmpthread_store_read_ex(zend_object *object, zval *key, zend_property_info* prop_info, int type, zval *read, zend_bool force_cache); +int pmmpthread_store_read(zend_object *object, zval *key, zend_property_info* prop_info, int type, zval *read); zend_bool pmmpthread_store_isset(zend_object *object, zval *key, int has_set_exists); -int pmmpthread_store_write(zend_object *object, zval *key, zval *write, zend_bool coerce_array_to_threaded); +int pmmpthread_store_write_ex(zend_object *object, zval *key, zend_property_info* prop_info, zval *write, zend_bool coerce_array_to_threaded, zend_bool *cached); +int pmmpthread_store_write(zend_object *object, zval *key, zend_property_info* prop_info, zval *write, zend_bool coerce_array_to_threaded); void pmmpthread_store_tohash(zend_object *object, HashTable *hash); int pmmpthread_store_shift(zend_object *object, zval *member); int pmmpthread_store_chunk(zend_object *object, zend_long size, zend_bool preserve, zval *chunk); @@ -59,4 +62,7 @@ void pmmpthread_store_key(zend_object *object, zval *key, HashPosition *position void pmmpthread_store_data(zend_object *object, zval *value, HashPosition *position); void pmmpthread_store_forward(zend_object *object, HashPosition *position); /* }}} */ +static inline zend_bool pmmpthread_store_retain_in_local_cache(zval* val) { + return IS_THREADSAFE_CLASS_INSTANCE(val) || IS_CLOSURE_OBJECT(val) || IS_EXT_SOCKETS_OBJECT(val) || Z_TYPE_P(val) == IS_STRING; +} #endif diff --git a/src/worker.c b/src/worker.c index 00e0da60..b5da5767 100644 --- a/src/worker.c +++ b/src/worker.c @@ -118,7 +118,7 @@ zend_long pmmpthread_worker_collect_tasks(pmmpthread_worker_data_t *worker_data, item = worker_data->gc.head; zend_long tasks_collected = 0; while (item) { - pmmpthread_store_full_sync_local_properties(Z_OBJ(item->value)); + pmmpthread_store_cache_all(Z_OBJ(item->value)); if (collect(call, &item->value)) { item = item->next; @@ -149,7 +149,7 @@ zend_result pmmpthread_worker_sync_collectable_tasks(pmmpthread_worker_data_t* w while (item) { pmmpthread_zend_object_t* threaded = PMMPTHREAD_FETCH_FROM(Z_OBJ(item->value)); if (pmmpthread_monitor_lock(&threaded->ts_obj->monitor)) { - pmmpthread_store_full_sync_local_properties(Z_OBJ(item->value)); + pmmpthread_store_cache_all(Z_OBJ(item->value)); pmmpthread_monitor_unlock(&threaded->ts_obj->monitor); } item = item->next; diff --git a/tests/uninitialized-typed-properties.phpt b/tests/uninitialized-typed-properties.phpt index 37c3d873..a76524ba 100644 --- a/tests/uninitialized-typed-properties.phpt +++ b/tests/uninitialized-typed-properties.phpt @@ -80,12 +80,12 @@ object(TS)#3 (3) { NULL } object(TS)#3 (2) { + ["a"]=> + uninitialized(int) ["b"]=> string(5) "hello" ["c"]=> NULL - ["a"]=> - uninitialized(int) } bool(false) bool(true) From ec4887b7c87fac764230bcf6be62eb3111df9636 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 16 Jan 2025 17:39:53 +0000 Subject: [PATCH 02/10] Fix property write leak --- src/store.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/store.c b/src/store.c index 1fbbf893..626de17e 100644 --- a/src/store.c +++ b/src/store.c @@ -450,21 +450,23 @@ zend_bool pmmpthread_store_isset(zend_object *object, zval *key, int has_set_exi } /* }}} */ static inline void pmmpthread_store_update_local_property(zend_object* object, zval* key, zend_property_info* prop_info, zval* value) { - Z_TRY_ADDREF_P(value); - zval* property; if (prop_info != NULL && prop_info != ZEND_WRONG_PROPERTY_INFO) { + Z_TRY_ADDREF_P(value); property = OBJ_PROP(object, prop_info->offset); zval_ptr_dtor(property); ZVAL_COPY_VALUE(property, value); } else { pmmpthread_store_init_local_properties(object); - if (Z_TYPE_P(key) == IS_LONG) { - zend_hash_index_update(object->properties, Z_LVAL_P(key), value); - } else { - zend_string* str_key = Z_STR_P(key); - property = zend_hash_find(object->properties, str_key); - if (property != value) { + property = Z_TYPE_P(key) == IS_LONG ? + zend_hash_index_find(object->properties, Z_LVAL_P(key)) : + zend_hash_find(object->properties, Z_STR_P(key)); + if (property != value) { + Z_TRY_ADDREF_P(value); + if (Z_TYPE_P(key) == IS_LONG) { + zend_hash_index_update(object->properties, Z_LVAL_P(key), value); + } else { + zend_string* str_key = Z_STR_P(key); //update_ind() ensures defined properties get properly updated even if we didn't have prop_info if ((GC_FLAGS(str_key) & (IS_STR_PERSISTENT | IS_STR_INTERNED)) == IS_STR_PERSISTENT) { //refcounted persistent string from pmmpthread_store - we can't use it directly From 4858bb11983b959cd2c886a1502fcb3996d5667d Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 16 Jan 2025 18:00:48 +0000 Subject: [PATCH 03/10] store_tohash: do not add refs to results of store_read --- src/store.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/store.c b/src/store.c index 626de17e..1792ae99 100644 --- a/src/store.c +++ b/src/store.c @@ -898,7 +898,6 @@ void pmmpthread_store_tohash(zend_object *object, HashTable *hash) { zend_hash_update(hash, info->name, &pzval); HT_FLAGS(hash) |= HASH_FLAG_HAS_EMPTY_IND; } else { - Z_TRY_ADDREF(pzval); zend_hash_update(hash, info->name, &pzval); } } @@ -924,7 +923,6 @@ void pmmpthread_store_tohash(zend_object *object, HashTable *hash) { pmmpthread_store_read_ex(object, &key, NULL, BP_VAR_R, &pzval, hash == object->properties); if (hash != object->properties) { - Z_TRY_ADDREF(pzval); if (name) { /* we can't use zend_hash_update() here - the string from store.props must not be returned to user code */ zend_hash_str_update(hash, ZSTR_VAL(name), ZSTR_LEN(name), &pzval); From 37f24c3d8d87bf936d230190b4b1e16242bc8587 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 16 Jan 2025 18:37:21 +0000 Subject: [PATCH 04/10] Fix some wild IS_UNDEF vars --- src/handlers.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/handlers.c b/src/handlers.c index 40eebf49..14383d2b 100644 --- a/src/handlers.c +++ b/src/handlers.c @@ -81,7 +81,7 @@ zval* pmmpthread_read_dimension(PMMPTHREAD_READ_DIMENSION_PASSTHRU_D) { zval* pmmpthread_read_property(PMMPTHREAD_READ_PROPERTY_PASSTHRU_D) { zval zmember; - zval result; + zval *result; zend_property_info* info = zend_get_property_info(object->ce, member, 0); if (info != NULL && info != ZEND_WRONG_PROPERTY_INFO) { @@ -95,7 +95,8 @@ zval* pmmpthread_read_property(PMMPTHREAD_READ_PROPERTY_PASSTHRU_D) { rv = &EG(uninitialized_zval); } else { //no cache for now - we don't want the VM bypassing this handler - zend_std_read_property(object, member, type, NULL, rv); + result = zend_std_read_property(object, member, type, NULL, rv); + ZVAL_COPY_VALUE(rv, result); //tidy property cache so we don't read wrong values later if (!pmmpthread_store_retain_in_local_cache(rv)) { pmmpthread_store_clean_local_property(object, &zmember, info); From 76099d80204f2c6adf382d39063ddf34edac67bb Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sat, 12 Apr 2025 22:42:07 +0100 Subject: [PATCH 05/10] Fix a ton of issues with uninit properties, virtual properties, use of static as dynamic, etc --- src/globals.c | 6 +- src/globals.h | 3 +- src/handlers.c | 163 ++++++++++++++------- src/object.c | 83 ++++++----- src/pmmpthread.h | 7 +- src/store.c | 167 +++++++++++++--------- tests/uninitialized-typed-properties.phpt | 26 ++++ 7 files changed, 296 insertions(+), 159 deletions(-) diff --git a/src/globals.c b/src/globals.c index b9eb1ef8..aa4b993d 100644 --- a/src/globals.c +++ b/src/globals.c @@ -114,7 +114,11 @@ zend_bool pmmpthread_globals_init(){ (dtor_func_t)pmmpthread_globals_string_dtor_func, 1 ); - ZVAL_UNDEF(&PMMPTHREAD_G(undef_zval)); + + ZVAL_UNDEF(&PMMPTHREAD_G(uninitialized_property)); + Z_PROP_FLAG_P(&PMMPTHREAD_G(uninitialized_property)) |= IS_PROP_UNINIT; + ZVAL_UNDEF(&PMMPTHREAD_G(unset_property)); + PMMPTHREAD_G(thread_count) = 0; //only counting threads explicitly created by pmmpthread } diff --git a/src/globals.h b/src/globals.h index f331e420..d956db7f 100644 --- a/src/globals.h +++ b/src/globals.h @@ -59,7 +59,8 @@ struct _pmmpthread_globals { */ HashTable interned_strings; - zval undef_zval; + zval uninitialized_property; + zval unset_property; /* * High Frequency Strings diff --git a/src/handlers.c b/src/handlers.c index 14383d2b..3818a9db 100644 --- a/src/handlers.c +++ b/src/handlers.c @@ -82,24 +82,44 @@ zval* pmmpthread_read_dimension(PMMPTHREAD_READ_DIMENSION_PASSTHRU_D) { zval* pmmpthread_read_property(PMMPTHREAD_READ_PROPERTY_PASSTHRU_D) { zval zmember; zval *result; + zend_guard* guard; - zend_property_info* info = zend_get_property_info(object->ce, member, 0); - if (info != NULL && info != ZEND_WRONG_PROPERTY_INFO) { - ZVAL_STR(&zmember, info->name); - } else { + if (object->ce->__get && (guard = zend_get_property_guard(object, member)) && !((*guard) & IN_GET)) { ZVAL_STR(&zmember, member); - } - //this moves the value to cache for zend_std_read_property() to work on - pmmpthread_store_read_ex(object, &zmember, info, type, rv, 1); - if (EG(exception)) { - rv = &EG(uninitialized_zval); + (*guard) |= IN_GET; + zend_call_known_instance_method_with_1_params(object->ce->__get, object, rv, &zmember); + (*guard) &= ~IN_GET; } else { - //no cache for now - we don't want the VM bypassing this handler - result = zend_std_read_property(object, member, type, NULL, rv); - ZVAL_COPY_VALUE(rv, result); - //tidy property cache so we don't read wrong values later - if (!pmmpthread_store_retain_in_local_cache(rv)) { - pmmpthread_store_clean_local_property(object, &zmember, info); + zend_property_info* info = zend_get_property_info(object->ce, member, 0); + ZVAL_STR(&zmember, member); + if (info != NULL) { + if (info->flags & ZEND_ACC_VIRTUAL) { + //virtual properties are similar to magic methods - don't touch store or cache + return zend_std_read_property(object, &zmember, type, NULL, rv); + } + if (!PMMPTHREAD_OBJECT_PROPERTY(info)) { + info = NULL; //don't send invalid infos into store + } + } + + if (info != NULL) { + ZVAL_STR(&zmember, info->name); + } else { + ZVAL_STR(&zmember, member); + } + + //this moves the value to cache for zend_std_read_property() to work on + pmmpthread_store_read_ex(object, &zmember, info, type, rv, 1); + if (EG(exception)) { + rv = &EG(uninitialized_zval); + } else { + //no cache for now - we don't want the VM bypassing this handler + result = zend_std_read_property(object, member, type, NULL, rv); + ZVAL_COPY_VALUE(rv, result); + //tidy property cache so we don't read wrong values later + if (!pmmpthread_store_retain_in_local_cache(rv)) { + pmmpthread_store_clean_local_property(object, &zmember, info); + } } } @@ -133,38 +153,59 @@ zval* pmmpthread_write_property(PMMPTHREAD_WRITE_PROPERTY_PASSTHRU_D) { zval tmp; zend_guard* guard; - //no cache for now - cache would allow the VM to bypass this handler - //std_write may coerce the var to a different type, so we need to use the result - value = zend_std_write_property(object, member, value, NULL); + if (object->ce->__set && (guard = zend_get_property_guard(object, member)) && !((*guard) & IN_SET)) { + zval rv; + ZVAL_UNDEF(&rv); + ZVAL_STR(&zmember, member); + + (*guard) |= IN_SET; + zend_call_known_instance_method_with_2_params(object->ce->__set, object, &rv, &zmember, value); + (*guard) &= ~IN_SET; - if (value != &EG(error_zval)) { - zval* real_value = NULL; - zval zmember; - ZVAL_UNDEF(&zmember); - zend_property_info* info = zend_get_property_info(object->ce, member, 0); - if (info != NULL) { - if (info != ZEND_WRONG_PROPERTY_INFO) { + if (Z_TYPE(rv) != IS_UNDEF) + zval_dtor(&rv); + } else { + //no cache for now - cache would allow the VM to bypass this handler + //std_write may coerce the var to a different type, so we need to use the result + value = zend_std_write_property(object, member, value, NULL); + + if (value != &EG(error_zval)) { + zval* real_value = NULL; + zval zmember; + + ZVAL_STR(&zmember, member); + zend_property_info* info = zend_get_property_info(object->ce, member, 0); + if (info != NULL) { + if (info->flags & ZEND_ACC_VIRTUAL) { + return value; + } + if (!PMMPTHREAD_OBJECT_PROPERTY(info)) { + info = NULL; //don't send invalid property infos into store + } + } + + if (info != NULL) { ZVAL_STR(&zmember, info->name); real_value = OBJ_PROP(object, info->offset); + } else if (object->properties != NULL) { + ZVAL_STR(&zmember, member); + real_value = zend_hash_find(object->properties, member); } - } else if (object->properties != NULL) { - ZVAL_STR(&zmember, member); - real_value = zend_hash_find(object->properties, member); - } - if (real_value != NULL) { - zend_bool cached = 0; - if (pmmpthread_store_write_ex(object, &zmember, info, real_value, PMMPTHREAD_STORE_NO_COERCE_ARRAY, &cached) == FAILURE && !EG(exception)) { - zend_throw_error( - pmmpthread_ce_nts_value_error, - "Cannot assign non-thread-safe value of type %s to thread-safe class property %s::$%s", - zend_zval_type_name(value), - ZSTR_VAL(object->ce->name), - ZSTR_VAL(member) - ); - value = &EG(error_zval); - } - if (!cached) { - pmmpthread_store_clean_local_property(object, &zmember, info); + if (real_value != NULL && real_value == value) { //if real_value doesn't match, a hook or magic method was probably involved + zend_bool cached = 0; + if (pmmpthread_store_write_ex(object, &zmember, info, real_value, PMMPTHREAD_STORE_NO_COERCE_ARRAY, &cached) == FAILURE && !EG(exception)) { + zend_throw_error( + pmmpthread_ce_nts_value_error, + "Cannot assign non-thread-safe value of type %s to thread-safe class property %s::$%s", + zend_zval_type_name(real_value), + ZSTR_VAL(object->ce->name), + ZSTR_VAL(member) + ); + value = &EG(error_zval); + } + if (!cached) { + pmmpthread_store_clean_local_property(object, &zmember, info); + } } } } @@ -229,16 +270,34 @@ void pmmpthread_unset_dimension(PMMPTHREAD_UNSET_DIMENSION_PASSTHRU_D) { void pmmpthread_unset_property(PMMPTHREAD_UNSET_PROPERTY_PASSTHRU_D) { zval zmember; - - zend_std_unset_property(object, member, NULL); - if (!EG(exception)) { - zend_property_info* info = zend_get_property_info(object->ce, member, 0); - if (info != NULL && info != ZEND_WRONG_PROPERTY_INFO) { - ZVAL_STR(&zmember, info->name); - } else { - ZVAL_STR(&zmember, member); + zend_guard* guard; + + if (object->ce->__unset && (guard = zend_get_property_guard(object, member)) && !((*guard) & IN_UNSET)) { + zval rv; + ZVAL_UNDEF(&rv); + ZVAL_STR(&zmember, member); + + (*guard) |= IN_UNSET; + zend_call_known_instance_method_with_1_params(object->ce->__unset, object, &rv, &zmember); + (*guard) &= ~IN_UNSET; + + if (Z_TYPE(rv) != IS_UNDEF) { + zval_dtor(&rv); + } + } else { + zend_std_unset_property(object, member, NULL); + if (!EG(exception)) { + zend_property_info* info = zend_get_property_info(object->ce, member, 0); + if (info != NULL && !PMMPTHREAD_OBJECT_PROPERTY(info)) { + info = NULL; //don't send invalid infos into store + } + if (info != NULL) { + ZVAL_STR(&zmember, info->name); + } else { + ZVAL_STR(&zmember, member); + } + pmmpthread_store_delete(object, &zmember, info); } - pmmpthread_store_delete(object, &zmember, info); } } /* }}} */ diff --git a/src/object.c b/src/object.c index b46b3cd3..87d6c4ba 100644 --- a/src/object.c +++ b/src/object.c @@ -187,6 +187,21 @@ static inline int _pmmpthread_connect_nolock(pmmpthread_zend_object_t* source, p if (destination->std.properties) zend_hash_clean(destination->std.properties); + for (int i = 0; i < destination->std.ce->default_properties_count; i++) { + zend_property_info* prop_info = destination->std.ce->properties_info_table[i]; + if (!prop_info || !PMMPTHREAD_OBJECT_PROPERTY(prop_info)) { + continue; + } + + zval* local = OBJ_PROP(destination, prop_info->offset); + zval* shared = zend_hash_find(&source->ts_obj->props.hash, prop_info->name); + + //ensure connected objects reflect unset/uninit property state correctly + //indirections will point to PMMPTHREAD_G if they exist + Z_PROP_FLAG_P(local) = Z_TYPE_P(shared) == IS_INDIRECT ? + Z_PROP_FLAG_P(Z_INDIRECT_P(shared)) : + 0; + } return SUCCESS; } else return FAILURE; } /* }}} */ @@ -243,43 +258,43 @@ static inline void pmmpthread_base_write_property_defaults(pmmpthread_zend_objec zend_class_entry* ce = base->std.ce; - while (ce != NULL) { - ZEND_HASH_FOREACH_PTR(&ce->properties_info, info) { - zval* value; - int result; + for (int i = 0; i < ce->default_properties_count; i++) { + info = ce->properties_info_table[i]; + if (info == NULL || !PMMPTHREAD_OBJECT_PROPERTY(info)) { + continue; + } + + zval* value; + int result; - if (!PMMPTHREAD_OBJECT_PROPERTY(info)) { - continue; - } + zend_string* interned_name = pmmpthread_globals_add_interned_string(info->name); + ZVAL_INTERNED_STR(&key, interned_name); - zend_string* interned_name = pmmpthread_globals_add_interned_string(info->name); - ZVAL_INTERNED_STR(&key, interned_name); - - value = OBJ_PROP(&base->std, info->offset); - if (!Z_ISUNDEF_P(value)) { - result = pmmpthread_store_write( - &base->std, - &key, - info, - value, - PMMPTHREAD_STORE_NO_COERCE_ARRAY - ); - if (result == FAILURE) { - zend_throw_error( - NULL, - "Cannot use non-thread-safe default of type %s for thread-safe class property %s::$%s", - zend_zval_type_name(value), - ZSTR_VAL(ce->name), - ZSTR_VAL(Z_STR(key)) - ); - break; - } - zval_ptr_dtor(value); - ZVAL_UNDEF(value); - } - } ZEND_HASH_FOREACH_END(); + value = OBJ_PROP(&base->std, info->offset); + if (Z_ISUNDEF_P(value)) { + ZEND_ASSERT(Z_PROP_FLAG_P(value) & IS_PROP_UNINIT); + value = &PMMPTHREAD_G(uninitialized_property); + } + result = pmmpthread_store_write( + &base->std, + &key, + info, + value, + PMMPTHREAD_STORE_NO_COERCE_ARRAY + ); + if (result == FAILURE) { + zend_throw_error( + NULL, + "Cannot use non-thread-safe default of type %s for thread-safe class property %s::$%s", + zend_zval_type_name(value), + ZSTR_VAL(ce->name), + ZSTR_VAL(Z_STR(key)) + ); + break; + } + zval_ptr_dtor(value); + ZVAL_UNDEF(value); - ce = ce->parent; } } /* }}} */ diff --git a/src/pmmpthread.h b/src/pmmpthread.h index 840238f9..79b1abbb 100644 --- a/src/pmmpthread.h +++ b/src/pmmpthread.h @@ -163,11 +163,10 @@ typedef struct _pmmpthread_call_t { #define PMMPTHREAD_CALL_EMPTY {empty_fcall_info, empty_fcall_info_cache} -#if PHP_VERSION_ID >= 80400 -#define PMMPTHREAD_OBJECT_PROPERTY(prop_info) ((prop_info->flags & (ZEND_ACC_STATIC | ZEND_ACC_VIRTUAL)) == 0) -#else -#define PMMPTHREAD_OBJECT_PROPERTY(prop_info) ((prop_info->flags & ZEND_ACC_STATIC) == 0) +#if PHP_VERSION_ID < 80400 +#define ZEND_ACC_VIRTUAL 0 #endif +#define PMMPTHREAD_OBJECT_PROPERTY(prop_info) ((prop_info->flags & (ZEND_ACC_STATIC | ZEND_ACC_VIRTUAL)) == 0) /* this is a copy of the same struct in zend_closures.c, which unfortunately isn't exported */ typedef struct _zend_closure { diff --git a/src/store.c b/src/store.c index 1792ae99..ed7a8ce7 100644 --- a/src/store.c +++ b/src/store.c @@ -316,6 +316,61 @@ static inline zend_bool pmmpthread_store_member_is_cacheable(zend_object *object } /* }}} */ +static inline void pmmpthread_store_update_local_property(zend_object* object, zval* key, zend_property_info* prop_info, zval* value) { + zval* property; + if (prop_info != NULL && prop_info != ZEND_WRONG_PROPERTY_INFO) { + Z_TRY_ADDREF_P(value); + property = OBJ_PROP(object, prop_info->offset); + zval_ptr_dtor(property); + ZVAL_COPY_VALUE(property, value); + } else { + pmmpthread_store_init_local_properties(object); + property = Z_TYPE_P(key) == IS_LONG ? + zend_hash_index_find(object->properties, Z_LVAL_P(key)) : + zend_hash_find(object->properties, Z_STR_P(key)); + if (property != value) { + Z_TRY_ADDREF_P(value); + if (Z_TYPE_P(key) == IS_LONG) { + zend_hash_index_update(object->properties, Z_LVAL_P(key), value); + } else { + zend_string* str_key = Z_STR_P(key); + //update_ind() ensures defined properties get properly updated even if we didn't have prop_info + if ((GC_FLAGS(str_key) & (IS_STR_PERSISTENT | IS_STR_INTERNED)) == IS_STR_PERSISTENT) { + //refcounted persistent string from pmmpthread_store - we can't use it directly + //if a bucket with this key already exists, it'll be reused + zend_hash_str_update_ind(object->properties, Z_STRVAL_P(key), Z_STRLEN_P(key), value); + } else { + //any other interned or emalloc'd strings should be safe to use directly here + zend_hash_update_ind(object->properties, str_key, value); + } + } + } + } +} + +static inline zend_bool pmmpthread_store_update_shared_property(pmmpthread_object_t* ts_obj, zval* key, zval* zstorage) { + zend_bool result = FAILURE; + if (Z_TYPE_P(key) == IS_LONG) { + if (zend_hash_index_update(&ts_obj->props.hash, Z_LVAL_P(key), zstorage)) + result = SUCCESS; + } else { + zend_string* str_key = Z_STR_P(key); + if (GC_FLAGS(str_key) & IS_STR_PERMANENT) { + //only permanent strings can be used directly + if (zend_hash_update(&ts_obj->props.hash, str_key, zstorage)) { + result = SUCCESS; + } + } else { + //refcounted or request-local interned string - this must be hard-copied, regardless of where it came from + if (zend_hash_str_update(&ts_obj->props.hash, ZSTR_VAL(str_key), ZSTR_LEN(str_key), zstorage)) { + result = SUCCESS; + } + } + } + + return result; +} + void pmmpthread_store_clean_local_property(zend_object* object, zval* key, zend_property_info* prop_info) { zval* property; if (prop_info != NULL) { @@ -352,9 +407,15 @@ int pmmpthread_store_delete(zend_object *object, zval *key, zend_property_info* if (pmmpthread_monitor_lock(&ts_obj->monitor)) { zend_bool was_pmmpthread_object = pmmpthread_store_member_is_cacheable(object, &member); - if (Z_TYPE(member) == IS_LONG) { - result = zend_hash_index_del(&ts_obj->props.hash, Z_LVAL(member)); - } else result = zend_hash_del(&ts_obj->props.hash, Z_STR(member)); + if (prop_info != NULL) { + zval indirect; + ZVAL_INDIRECT(&indirect, &PMMPTHREAD_G(unset_property)); + result = pmmpthread_store_update_shared_property(ts_obj, &member, &indirect); + } else { + if (Z_TYPE(member) == IS_LONG) { + result = zend_hash_index_del(&ts_obj->props.hash, Z_LVAL(member)); + } else result = zend_hash_del(&ts_obj->props.hash, Z_STR(member)); + } if (result == SUCCESS && was_pmmpthread_object) { _pmmpthread_store_bump_modcount_nolock(threaded); @@ -396,6 +457,7 @@ zend_bool pmmpthread_store_isset(zend_object *object, zval *key, int has_set_exi isset = 1; if (has_set_exists == ZEND_PROPERTY_NOT_EMPTY) { switch (Z_TYPE_P(zstorage)) { + case IS_INDIRECT: //we only use IS_INDIRECT for uninit/unset known properties case IS_NULL: case IS_FALSE: isset = 0; @@ -433,7 +495,7 @@ zend_bool pmmpthread_store_isset(zend_object *object, zval *key, int has_set_exi break; } } else if (has_set_exists == ZEND_PROPERTY_ISSET) { - if (Z_TYPE_P(zstorage) == IS_NULL) { + if (Z_TYPE_P(zstorage) == IS_NULL || Z_TYPE_P(zstorage) == IS_INDIRECT) { isset = 0; } } else if (has_set_exists != ZEND_PROPERTY_EXISTS) { @@ -449,61 +511,6 @@ zend_bool pmmpthread_store_isset(zend_object *object, zval *key, int has_set_exi return isset; } /* }}} */ -static inline void pmmpthread_store_update_local_property(zend_object* object, zval* key, zend_property_info* prop_info, zval* value) { - zval* property; - if (prop_info != NULL && prop_info != ZEND_WRONG_PROPERTY_INFO) { - Z_TRY_ADDREF_P(value); - property = OBJ_PROP(object, prop_info->offset); - zval_ptr_dtor(property); - ZVAL_COPY_VALUE(property, value); - } else { - pmmpthread_store_init_local_properties(object); - property = Z_TYPE_P(key) == IS_LONG ? - zend_hash_index_find(object->properties, Z_LVAL_P(key)) : - zend_hash_find(object->properties, Z_STR_P(key)); - if (property != value) { - Z_TRY_ADDREF_P(value); - if (Z_TYPE_P(key) == IS_LONG) { - zend_hash_index_update(object->properties, Z_LVAL_P(key), value); - } else { - zend_string* str_key = Z_STR_P(key); - //update_ind() ensures defined properties get properly updated even if we didn't have prop_info - if ((GC_FLAGS(str_key) & (IS_STR_PERSISTENT | IS_STR_INTERNED)) == IS_STR_PERSISTENT) { - //refcounted persistent string from pmmpthread_store - we can't use it directly - //if a bucket with this key already exists, it'll be reused - zend_hash_str_update_ind(object->properties, Z_STRVAL_P(key), Z_STRLEN_P(key), value); - } else { - //any other interned or emalloc'd strings should be safe to use directly here - zend_hash_update_ind(object->properties, str_key, value); - } - } - } - } -} - -static inline zend_bool pmmpthread_store_update_shared_property(pmmpthread_object_t* ts_obj, zval* key, zval* zstorage) { - zend_bool result = FAILURE; - if (Z_TYPE_P(key) == IS_LONG) { - if (zend_hash_index_update(&ts_obj->props.hash, Z_LVAL_P(key), zstorage)) - result = SUCCESS; - } else { - zend_string* str_key = Z_STR_P(key); - if (GC_FLAGS(str_key) & IS_STR_PERMANENT) { - //only permanent strings can be used directly - if (zend_hash_update(&ts_obj->props.hash, str_key, zstorage)) { - result = SUCCESS; - } - } else { - //refcounted or request-local interned string - this must be hard-copied, regardless of where it came from - if (zend_hash_str_update(&ts_obj->props.hash, ZSTR_VAL(str_key), ZSTR_LEN(str_key), zstorage)) { - result = SUCCESS; - } - } - } - - return result; -} - /* {{{ */ int pmmpthread_store_read_ex(zend_object *object, zval *key, zend_property_info *prop_info, int type, zval *read, zend_bool force_cache) { int result = FAILURE; @@ -1108,6 +1115,14 @@ static pmmpthread_storage* pmmpthread_store_create(pmmpthread_ident_t* source, z static zend_result pmmpthread_store_save_zval(pmmpthread_ident_t* source, zval *zstorage, zval *write) { zend_result result = FAILURE; switch (Z_TYPE_P(write)) { + case IS_UNDEF: + if (write == &PMMPTHREAD_G(uninitialized_property)) { + ZVAL_INDIRECT(zstorage, &PMMPTHREAD_G(uninitialized_property)); + } else { + ZVAL_INDIRECT(zstorage, &PMMPTHREAD_G(unset_property)); + } + result = SUCCESS; + break; case IS_NULL: case IS_FALSE: case IS_TRUE: @@ -1212,6 +1227,11 @@ static int pmmpthread_store_convert(pmmpthread_storage *storage, zval *pzval){ static void pmmpthread_store_restore_zval_ex(zval *unstore, zval *zstorage, zend_bool *may_be_locally_cached) { *may_be_locally_cached = pmmpthread_store_storage_is_cacheable(zstorage); switch (Z_TYPE_P(zstorage)) { + case IS_INDIRECT: + //we only use IS_INDIRECT for uninit/unset known properties + //ZVAL_COPY_PROP ensures retention of Z_PROP_FLAG + ZVAL_COPY_PROP(unstore, Z_INDIRECT_P(zstorage)); + break; case IS_NULL: case IS_FALSE: case IS_TRUE: @@ -1455,6 +1475,24 @@ static void pmmpthread_store_storage_dtor (zval *zstorage){ } } /* }}} */ +static void iterator_skip_uninit(pmmpthread_object_t* ts_obj, HashPosition* position) { + while (true) { + if (zend_hash_has_more_elements_ex(&ts_obj->props.hash, position) == FAILURE) { + *position = HT_INVALID_IDX; + break; + } + zval* current = zend_hash_get_current_data_ex(&ts_obj->props.hash, position); + + if (Z_TYPE_P(current) == IS_INDIRECT) { + //we currently only use IS_INDIRECT for uninit and unset properties + ZEND_ASSERT(Z_TYPE_P(Z_INDIRECT_P(current)) == IS_UNDEF); + zend_hash_move_forward_ex(&ts_obj->props.hash, position); + } else { + break; + } + } +} + /* {{{ iteration helpers */ void pmmpthread_store_reset(zend_object *object, HashPosition *position) { pmmpthread_object_t *ts_obj = PMMPTHREAD_FETCH_TS_FROM(object); @@ -1462,11 +1500,8 @@ void pmmpthread_store_reset(zend_object *object, HashPosition *position) { if (pmmpthread_monitor_lock(&ts_obj->monitor)) { if (ts_obj->props.first == HT_INVALID_IDX) { zend_hash_internal_pointer_reset_ex(&ts_obj->props.hash, position); - if (zend_hash_has_more_elements_ex(&ts_obj->props.hash, position) == FAILURE) { //empty - *position = HT_INVALID_IDX; - } else { - ts_obj->props.first = *position; - } + iterator_skip_uninit(ts_obj, position); + ts_obj->props.first = *position; } else { *position = ts_obj->props.first; } @@ -1519,9 +1554,7 @@ void pmmpthread_store_forward(zend_object *object, HashPosition *position) { if (pmmpthread_monitor_lock(&ts_obj->monitor)) { zend_hash_move_forward_ex( &ts_obj->props.hash, position); - if (zend_hash_has_more_elements_ex(&ts_obj->props.hash, position) == FAILURE) { - *position = HT_INVALID_IDX; - } + iterator_skip_uninit(ts_obj, position); pmmpthread_monitor_unlock(&ts_obj->monitor); } } /* }}} */ diff --git a/tests/uninitialized-typed-properties.phpt b/tests/uninitialized-typed-properties.phpt index a76524ba..ada50e47 100644 --- a/tests/uninitialized-typed-properties.phpt +++ b/tests/uninitialized-typed-properties.phpt @@ -32,6 +32,12 @@ function test(object $t1) : void{ var_dump($t1->c); var_dump(isset($t1->c)); var_dump(property_exists($t1, "c")); + + unset($t1->b); + var_dump($t1); + foreach($t1 as $name => $value){ + var_dump($name, $value); + } } echo "--- Normal object ---\n"; @@ -69,6 +75,16 @@ NULL NULL bool(false) bool(true) +object(NTS)#2 (1) { + ["a"]=> + uninitialized(int) + ["b"]=> + uninitialized(string) + ["c"]=> + NULL +} +string(1) "c" +NULL --- pthreads object --- --- properties are currently expected to be in an unstable order --- object(TS)#3 (3) { @@ -97,4 +113,14 @@ NULL NULL bool(false) bool(true) +object(TS)#3 (1) { + ["a"]=> + uninitialized(int) + ["b"]=> + uninitialized(string) + ["c"]=> + NULL +} +string(1) "c" +NULL --- Done --- From 6df6326c58f01ceb02a4c07ddd3546ca46a9cb26 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sat, 12 Apr 2025 22:46:26 +0100 Subject: [PATCH 06/10] handler: suppress bad property access warnings the std handlers will already generate these warnings anyway, so we're just duplicating them. --- src/handlers.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/handlers.c b/src/handlers.c index 3818a9db..ecb9b645 100644 --- a/src/handlers.c +++ b/src/handlers.c @@ -90,7 +90,7 @@ zval* pmmpthread_read_property(PMMPTHREAD_READ_PROPERTY_PASSTHRU_D) { zend_call_known_instance_method_with_1_params(object->ce->__get, object, rv, &zmember); (*guard) &= ~IN_GET; } else { - zend_property_info* info = zend_get_property_info(object->ce, member, 0); + zend_property_info* info = zend_get_property_info(object->ce, member, 1); ZVAL_STR(&zmember, member); if (info != NULL) { if (info->flags & ZEND_ACC_VIRTUAL) { @@ -174,7 +174,7 @@ zval* pmmpthread_write_property(PMMPTHREAD_WRITE_PROPERTY_PASSTHRU_D) { zval zmember; ZVAL_STR(&zmember, member); - zend_property_info* info = zend_get_property_info(object->ce, member, 0); + zend_property_info* info = zend_get_property_info(object->ce, member, 1); if (info != NULL) { if (info->flags & ZEND_ACC_VIRTUAL) { return value; @@ -287,7 +287,7 @@ void pmmpthread_unset_property(PMMPTHREAD_UNSET_PROPERTY_PASSTHRU_D) { } else { zend_std_unset_property(object, member, NULL); if (!EG(exception)) { - zend_property_info* info = zend_get_property_info(object->ce, member, 0); + zend_property_info* info = zend_get_property_info(object->ce, member, 1); if (info != NULL && !PMMPTHREAD_OBJECT_PROPERTY(info)) { info = NULL; //don't send invalid infos into store } From cd4c0ab4adfbb214f96429098dc19169d13ca0df Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sat, 12 Apr 2025 23:31:41 +0100 Subject: [PATCH 07/10] Adjust test expectations --- tests/closure-assign-to-threaded.phpt | 4 +++- tests/foreign-private-members.phpt | 1 + tests/inherited-anon-class-outside-context.phpt | 5 +++++ tests/inherited-anon-class.phpt | 1 + tests/interface.phpt | 3 ++- tests/magic-methods-get-set.phpt | 2 ++ tests/magic-methods-isset-unset.phpt | 2 ++ tests/norefs.phpt | 11 +++++++---- tests/normal-reads.phpt | 1 + tests/object-comparison.phpt | 7 ++++--- tests/oomethods.phpt | 3 ++- tests/php8-socket-copy-overwrite-basic.phpt | 4 +++- tests/property-exists.phpt | 4 +++- tests/staticmethods.phpt | 3 ++- tests/threaded-member-overwrite.phpt | 4 +++- tests/workers-no-collect.phpt | 5 +++-- tests/workers-no-join.phpt | 11 ++++++----- tests/workers-no-stack.phpt | 5 +++-- tests/workers-no-start.phpt | 11 ++++++----- tests/workers-no-unstack.phpt | 11 ++++++----- 20 files changed, 65 insertions(+), 33 deletions(-) diff --git a/tests/closure-assign-to-threaded.phpt b/tests/closure-assign-to-threaded.phpt index 392130c4..bba75f1e 100644 --- a/tests/closure-assign-to-threaded.phpt +++ b/tests/closure-assign-to-threaded.phpt @@ -3,7 +3,9 @@ Test assigning Closure to ThreadSafe doesn't break use()d variables --FILE-- func = function() use($name, $type) : void{ diff --git a/tests/foreign-private-members.phpt b/tests/foreign-private-members.phpt index d0282a3c..1235a34f 100644 --- a/tests/foreign-private-members.phpt +++ b/tests/foreign-private-members.phpt @@ -13,6 +13,7 @@ class MY { } class TEST extends \pmmp\thread\Thread { + private string $my; public function __construct($my) { $this->my = serialize($my); } diff --git a/tests/inherited-anon-class-outside-context.phpt b/tests/inherited-anon-class-outside-context.phpt index ceb51b6c..cbdae018 100644 --- a/tests/inherited-anon-class-outside-context.phpt +++ b/tests/inherited-anon-class-outside-context.phpt @@ -8,6 +8,8 @@ Unbound anon class causing segfaults, we delay copy but still cannot serialize t interface TestInterface{} class Test extends \pmmp\thread\Thread { + public bool $alive = false; + public \pmmp\thread\Thread $anonymous; /** * doccomment run */ @@ -25,6 +27,7 @@ class Test extends \pmmp\thread\Thread { protected $protProp; private $privProp; public static $staticProp; + public bool $ready; public function run() : void{ var_dump('anonymous run'); $this->ready = true; @@ -71,6 +74,8 @@ object(pmmp\thread\Thread@anonymous)#3 (3) { NULL ["privProp":"pmmp\thread\Thread@anonymous":private]=> NULL + ["ready"]=> + uninitialized(bool) } string(13) "anonymous run" object(pmmp\thread\Thread@anonymous)#4 (4) { diff --git a/tests/inherited-anon-class.phpt b/tests/inherited-anon-class.phpt index fcb90c7f..534be0af 100644 --- a/tests/inherited-anon-class.phpt +++ b/tests/inherited-anon-class.phpt @@ -5,6 +5,7 @@ Unbound anon class causing segfaults, we delay copy but still cannot serialize t --FILE-- prop = new class extends \pmmp\thread\ThreadSafe {}; var_dump($this->prop); diff --git a/tests/interface.phpt b/tests/interface.phpt index 1d3eb34a..7c77a4fd 100644 --- a/tests/interface.phpt +++ b/tests/interface.phpt @@ -9,7 +9,8 @@ interface INamedThread { function getName(); } -class TestThread extends \pmmp\thread\Thread implements INamedThread { +class TestThread extends \pmmp\thread\Thread implements INamedThread { + public string $name; public function setName($name) { $this->name = $name; } diff --git a/tests/magic-methods-get-set.phpt b/tests/magic-methods-get-set.phpt index 1ff012a4..ae6d7350 100644 --- a/tests/magic-methods-get-set.phpt +++ b/tests/magic-methods-get-set.phpt @@ -4,6 +4,8 @@ Test magic __get and __set This test verifies that __set and __get work as expected --FILE-- t = new \pmmp\thread\ThreadSafe(); + $this->t = new class extends \pmmp\thread\ThreadSafe{ + public bool $set; + }; $this->t->set = true; } @@ -27,18 +30,18 @@ $t->join(); --EXPECTF-- object(T)#%d (%d) { ["t"]=> - object(pmmp\thread\ThreadSafe)#%d (%d) { + object(%s@anonymous)#%d (%d) { ["set"]=> bool(true) } } -object(pmmp\thread\ThreadSafe)#%d (%d) { +object(%s@anonymous)#%d (%d) { ["set"]=> bool(true) } object(T)#%d (%d) { ["t"]=> - object(pmmp\thread\ThreadSafe)#%d (%d) { + object(%s@anonymous)#%d (%d) { ["set"]=> bool(true) } diff --git a/tests/normal-reads.phpt b/tests/normal-reads.phpt index 0ffbb8ed..b8af5547 100644 --- a/tests/normal-reads.phpt +++ b/tests/normal-reads.phpt @@ -5,6 +5,7 @@ This test verifies that reading properties from the object without var_dump/prin --FILE-- name = sprintf("%s", __CLASS__); } diff --git a/tests/object-comparison.phpt b/tests/object-comparison.phpt index 7f231b00..b9c073d0 100644 --- a/tests/object-comparison.phpt +++ b/tests/object-comparison.phpt @@ -15,9 +15,10 @@ comparison which would return true if the property tables of two distinct object --FILE-- arg1 = $arg1; - $this->arg2 = $arg2; + public function __construct( + private \pmmp\thread\ThreadSafeArray $arg1, + private \pmmp\thread\ThreadSafeArray $arg2 + ) { } public function run() : void{ diff --git a/tests/oomethods.phpt b/tests/oomethods.phpt index e732ecf1..97d7b59b 100644 --- a/tests/oomethods.phpt +++ b/tests/oomethods.phpt @@ -4,7 +4,8 @@ Test access to user defined methods in the object context User methods are now imported from your declared class into the thread --FILE-- value; } diff --git a/tests/php8-socket-copy-overwrite-basic.phpt b/tests/php8-socket-copy-overwrite-basic.phpt index a8d02972..0b77391f 100644 --- a/tests/php8-socket-copy-overwrite-basic.phpt +++ b/tests/php8-socket-copy-overwrite-basic.phpt @@ -5,7 +5,9 @@ Test that standard Socket objects get copied and overwritten properly --FILE-- socket = socket_create(AF_INET, SOCK_DGRAM, SOL_UDP); socket_connect($threaded->socket, "127.0.0.1", 19132); diff --git a/tests/property-exists.phpt b/tests/property-exists.phpt index d8b57522..264ab31b 100644 --- a/tests/property-exists.phpt +++ b/tests/property-exists.phpt @@ -12,7 +12,9 @@ function test(object $t) : void{ } test(new \stdClass); -test(new \pmmp\thread\ThreadSafe); +test(new class extends \pmmp\thread\ThreadSafe{ + public $prop; +}); ?> --EXPECT-- bool(true) diff --git a/tests/staticmethods.phpt b/tests/staticmethods.phpt index 1ee946e7..60c301bd 100644 --- a/tests/staticmethods.phpt +++ b/tests/staticmethods.phpt @@ -4,7 +4,8 @@ Test access to static methods from within user threads Static methods as declared in the users implementation of Thread should now be available for calling in the thread scope --FILE-- a = function(){}; diff --git a/tests/workers-no-collect.phpt b/tests/workers-no-collect.phpt index df9ca98d..b191f2aa 100644 --- a/tests/workers-no-collect.phpt +++ b/tests/workers-no-collect.phpt @@ -5,8 +5,9 @@ This test verifies that workers cannot be misused (collect) --FILE-- worker = $worker; + public function __construct( + private \pmmp\thread\Worker $worker + ) { } public function run() : void{ diff --git a/tests/workers-no-join.phpt b/tests/workers-no-join.phpt index fec52762..5cde2935 100644 --- a/tests/workers-no-join.phpt +++ b/tests/workers-no-join.phpt @@ -5,8 +5,9 @@ This test verifies that workers cannot be misused (join) --FILE-- worker = $worker; + public function __construct( + private \pmmp\thread\Worker $worker + ) { } public function run() : void{ @@ -21,11 +22,11 @@ $test->start(\pmmp\thread\Thread::INHERIT_ALL); $test->join(); ?> --EXPECTF-- -Fatal error: Uncaught RuntimeException: only the creator of this pmmp\thread\Worker may join with it in %s:8 +Fatal error: Uncaught RuntimeException: only the creator of this pmmp\thread\Worker may join with it in %s:%d Stack trace: -#0 %s(8): pmmp\thread\Worker->shutdown() +#0 %s(%d): pmmp\thread\Worker->shutdown() #1 [internal function]: Test->run() #2 {main} - thrown in %s on line 8 + thrown in %s on line %d diff --git a/tests/workers-no-stack.phpt b/tests/workers-no-stack.phpt index f22185f6..c37dd9ef 100644 --- a/tests/workers-no-stack.phpt +++ b/tests/workers-no-stack.phpt @@ -9,8 +9,9 @@ class Work extends \pmmp\thread\Runnable { } class Test extends \pmmp\thread\Thread { - public function __construct(\pmmp\thread\Worker $worker) { - $this->worker = $worker; + public function __construct( + private \pmmp\thread\Worker $worker + ) { } public function run() : void{ diff --git a/tests/workers-no-start.phpt b/tests/workers-no-start.phpt index b23f9e0b..5bdd8b3d 100644 --- a/tests/workers-no-start.phpt +++ b/tests/workers-no-start.phpt @@ -5,8 +5,9 @@ This test verifies that workers cannot be misused (start) --FILE-- worker = $worker; + public function __construct( + private \pmmp\thread\Worker $worker + ) { } public function run() : void{ @@ -20,11 +21,11 @@ $test->start(\pmmp\thread\Thread::INHERIT_ALL); $test->join(); ?> --EXPECTF-- -Fatal error: Uncaught RuntimeException: only the creator of this pmmp\thread\Worker may start it in %s:8 +Fatal error: Uncaught RuntimeException: only the creator of this pmmp\thread\Worker may start it in %s:%d Stack trace: -#0 %s(8): pmmp\thread\Thread->start(%d) +#0 %s(%d): pmmp\thread\Thread->start(%d) #1 [internal function]: Test->run() #2 {main} - thrown in %s on line 8 + thrown in %s on line %d diff --git a/tests/workers-no-unstack.phpt b/tests/workers-no-unstack.phpt index d7968df1..42351e94 100644 --- a/tests/workers-no-unstack.phpt +++ b/tests/workers-no-unstack.phpt @@ -5,8 +5,9 @@ This test verifies that workers cannot be misused (unstack) --FILE-- worker = $worker; + public function __construct( + private \pmmp\thread\Worker $worker + ) { } public function run() : void{ @@ -22,11 +23,11 @@ $test->start(\pmmp\thread\Thread::INHERIT_ALL); $test->join(); ?> --EXPECTF-- -Fatal error: Uncaught RuntimeException: only the creator of this pmmp\thread\Worker may call unstack in %s:8 +Fatal error: Uncaught RuntimeException: only the creator of this pmmp\thread\Worker may call unstack in %s:%d Stack trace: -#0 %s(8): pmmp\thread\Worker->unstack() +#0 %s(%d): pmmp\thread\Worker->unstack() #1 [internal function]: Test->run() #2 {main} - thrown in %s on line 8 + thrown in %s on line %d From 736a5bd1ebae817bbfed05904799c7b7d077b017 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sat, 12 Apr 2025 23:45:20 +0100 Subject: [PATCH 08/10] object: lock source object during updates --- src/object.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/object.c b/src/object.c index 87d6c4ba..c4762db9 100644 --- a/src/object.c +++ b/src/object.c @@ -179,8 +179,8 @@ void pmmpthread_current_thread(zval *return_value) { } /* }}} */ /* {{{ */ -static inline int _pmmpthread_connect_nolock(pmmpthread_zend_object_t* source, pmmpthread_zend_object_t* destination) { - if (source && destination) { +static inline int _pmmpthread_connect_no_global_lock(pmmpthread_zend_object_t* source, pmmpthread_zend_object_t* destination) { + if (source && destination && pmmpthread_monitor_lock(&source->ts_obj->monitor)) { destination->ts_obj = source->ts_obj; ++destination->ts_obj->refcount; @@ -202,6 +202,8 @@ static inline int _pmmpthread_connect_nolock(pmmpthread_zend_object_t* source, p Z_PROP_FLAG_P(Z_INDIRECT_P(shared)) : 0; } + + pmmpthread_monitor_unlock(&source->ts_obj->monitor); return SUCCESS; } else return FAILURE; } /* }}} */ @@ -210,7 +212,7 @@ static inline int _pmmpthread_connect_nolock(pmmpthread_zend_object_t* source, p static int pmmpthread_connect(pmmpthread_zend_object_t* source, pmmpthread_zend_object_t* destination) { int result = FAILURE; if(pmmpthread_globals_lock()){ - result = _pmmpthread_connect_nolock(source, destination); + result = _pmmpthread_connect_no_global_lock(source, destination); pmmpthread_globals_unlock(); } return result; From 91e0536660c1d25f6aeeaebf379c7c75f878bdcf Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sun, 13 Apr 2025 01:10:53 +0100 Subject: [PATCH 09/10] WTF, I have no idea why this worked --- src/handlers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/handlers.c b/src/handlers.c index ecb9b645..f9147354 100644 --- a/src/handlers.c +++ b/src/handlers.c @@ -95,7 +95,7 @@ zval* pmmpthread_read_property(PMMPTHREAD_READ_PROPERTY_PASSTHRU_D) { if (info != NULL) { if (info->flags & ZEND_ACC_VIRTUAL) { //virtual properties are similar to magic methods - don't touch store or cache - return zend_std_read_property(object, &zmember, type, NULL, rv); + return zend_std_read_property(object, member, type, NULL, rv); } if (!PMMPTHREAD_OBJECT_PROPERTY(info)) { info = NULL; //don't send invalid infos into store From 85bd3ef25b4383a37e5beecd8201de9f20607951 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sun, 13 Apr 2025 01:11:08 +0100 Subject: [PATCH 10/10] Remove unused variables --- src/handlers.c | 1 - src/store.c | 4 +--- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/handlers.c b/src/handlers.c index f9147354..391fb840 100644 --- a/src/handlers.c +++ b/src/handlers.c @@ -150,7 +150,6 @@ void pmmpthread_write_dimension(PMMPTHREAD_WRITE_DIMENSION_PASSTHRU_D) { zval* pmmpthread_write_property(PMMPTHREAD_WRITE_PROPERTY_PASSTHRU_D) { zval zmember; - zval tmp; zend_guard* guard; if (object->ce->__set && (guard = zend_get_property_guard(object, member)) && !((*guard) & IN_SET)) { diff --git a/src/store.c b/src/store.c index ed7a8ce7..62e876f5 100644 --- a/src/store.c +++ b/src/store.c @@ -118,7 +118,6 @@ void pmmpthread_store_clean_stale_cache(zend_object* object) { /* {{{ */ zend_string *name; zval *val; pmmpthread_storage *ts_val; - zend_bool remove; if (threaded->local_props_modcount == ts_obj->props.modcount) { return; @@ -883,7 +882,6 @@ void pmmpthread_store_tohash(zend_object *object, HashTable *hash) { if (pmmpthread_monitor_lock(&ts_obj->monitor)) { zend_string *name = NULL; zend_ulong idx; - zval *zstorage; pmmpthread_store_clean_stale_cache(object); @@ -910,7 +908,7 @@ void pmmpthread_store_tohash(zend_object *object, HashTable *hash) { } } - ZEND_HASH_FOREACH_KEY_VAL(&ts_obj->props.hash, idx, name, zstorage) { + ZEND_HASH_FOREACH_KEY(&ts_obj->props.hash, idx, name) { zval pzval; zval* existing;