From 338e0d172bef02012f932327aeb3d5cca0d2743e Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sat, 18 Mar 2023 16:56:51 +0000 Subject: [PATCH 1/2] First look at readonly property support Before this, readonly was silently ignored, so properties would be treated the same as normal properties. This change enforces write-once and no-unset policies for readonly properties. It also permits lockless reads of locally cached property values of readonly properties. Currently this only covers stuff that is kept in the local cache, such as thread-safe objects, closures, sockets and strings. The cache could be extended to more stuff to permit this optimisation to apply to other types of properties, but that's a task for another time. --- src/handlers.c | 92 +++++++++++++++++++++++++++++++++++++++----------- src/store.c | 55 +++++++++++++++++++++++++----- src/store.h | 9 +++++ 3 files changed, 129 insertions(+), 27 deletions(-) diff --git a/src/handlers.c b/src/handlers.c index c4850313..fa3d083a 100644 --- a/src/handlers.c +++ b/src/handlers.c @@ -92,7 +92,14 @@ zval* pthreads_read_property(PTHREADS_READ_PROPERTY_PASSTHRU_D) { } else { //defined property, use mangled name ZVAL_STR(&zmember, info->name); + +#if PHP_VERSION_ID >= 80100 + if ((info->flags & ZEND_ACC_READONLY) == 0 || pthreads_store_read_local_property(object, member, type, rv) == FAILURE) { + pthreads_store_read(object, &zmember, type, rv); + } +#else pthreads_store_read(object, &zmember, type, rv); +#endif if (Z_ISUNDEF_P(rv)) { if (type != BP_VAR_IS && !EG(exception)) { @@ -139,28 +146,57 @@ zval* pthreads_write_property(PTHREADS_WRITE_PROPERTY_PASSTHRU_D) { } else { bool ok = true; zend_property_info* info = zend_get_property_info(object->ce, member, 0); + if (info != ZEND_WRONG_PROPERTY_INFO) { - if (info != NULL && (info->flags & ZEND_ACC_STATIC) == 0) { - ZVAL_STR(&zmember, info->name); //use mangled name to avoid private member shadowing issues + bool overwrite = true; - 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)); + if (info != NULL) { + if ((info->flags & ZEND_ACC_STATIC) == 0) { + ZVAL_STR(&zmember, info->name); //use mangled name to avoid private member shadowing issues - if (ZEND_TYPE_IS_SET(info->type) && !zend_verify_property_type(info, value, strict)) { - ok = false; + 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)); + + if (ZEND_TYPE_IS_SET(info->type) && !zend_verify_property_type(info, value, strict)) { + ok = false; + } } +#if PHP_VERSION_ID >= 80100 + overwrite = (info->flags & ZEND_ACC_READONLY) == 0; +#endif } - if (ok && pthreads_store_write(object, &zmember, value, PTHREADS_STORE_NO_COERCE_ARRAY) == FAILURE && !EG(exception)) { - zend_throw_error( - NULL, - "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) - ); + if (ok) { + pthreads_store_write_result result = pthreads_store_write_ex(object, &zmember, value, PTHREADS_STORE_NO_COERCE_ARRAY, overwrite); + if (result != WRITE_SUCCESS && !EG(exception)) { + switch (result) { + case WRITE_FAIL_NOT_THREAD_SAFE: { + zend_throw_error( + NULL, + "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) + ); + break; + } + case WRITE_FAIL_WOULD_OVERWRITE: { + zend_throw_error( + NULL, + "Cannot modify readonly property %s::$%s", + ZSTR_VAL(object->ce->name), + ZSTR_VAL(member) + ); + break; + } + default: { + ZEND_ASSERT(0); + break; + } + } + } } } } @@ -231,10 +267,28 @@ void pthreads_unset_property(PTHREADS_UNSET_PROPERTY_PASSTHRU_D) { } else { zend_property_info* info = zend_get_property_info(object->ce, member, 0); if (info != ZEND_WRONG_PROPERTY_INFO) { - if (info != NULL && (info->flags & ZEND_ACC_STATIC) == 0) { - ZVAL_STR(&zmember, info->name); //defined property, use mangled name + zend_bool ok = true; + if (info != NULL) { + if ((info->flags & ZEND_ACC_STATIC) == 0) { + ZVAL_STR(&zmember, info->name); //defined property, use mangled name + } + +#if PHP_VERSION_ID >= 80100 + if ((info->flags & ZEND_ACC_READONLY) != 0) { + zend_throw_error( + NULL, + "Cannot unset readonly property %s::$%s", + ZSTR_VAL(object->ce->name), + ZSTR_VAL(member) + ); + ok = false; + } +#endif + } + + if (ok) { + pthreads_store_delete(object, &zmember); } - pthreads_store_delete(object, &zmember); } } } diff --git a/src/store.c b/src/store.c index a9992754..cb08666d 100644 --- a/src/store.c +++ b/src/store.c @@ -406,6 +406,25 @@ static inline zend_bool pthreads_store_update_shared_property(pthreads_object_t* return result; } +/* {{{ */ +int pthreads_store_read_local_property(zend_object* object, zend_string* key, int type, zval* read) { + zval* property; + pthreads_zend_object_t* threaded = PTHREADS_FETCH_FROM(object); + + if (threaded->std.properties) { + property = zend_hash_find(threaded->std.properties, key); + + if (property && pthreads_store_valid_local_cache_item(property)) { + pthreads_monitor_unlock(&threaded->ts_obj->monitor); + ZVAL_DEINDIRECT(property); + ZVAL_COPY(read, property); + return SUCCESS; + } + } + + return FAILURE; +} /* }}} */ + /* {{{ */ int pthreads_store_read(zend_object *object, zval *key, int type, zval *read) { int result = FAILURE; @@ -492,8 +511,8 @@ static zend_string* pthreads_store_restore_string(zend_string* string) { } /* {{{ */ -int pthreads_store_write(zend_object *object, zval *key, zval *write, zend_bool coerce_array_to_threaded) { - int result = FAILURE; +pthreads_store_write_result pthreads_store_write_ex(zend_object *object, zval *key, zval *write, zend_bool coerce_array_to_threaded, zend_bool overwrite) { + pthreads_store_write_result result = WRITE_FAIL_UNKNOWN; zval vol, member, zstorage; pthreads_zend_object_t *threaded = PTHREADS_FETCH_FROM(object); @@ -511,7 +530,7 @@ int pthreads_store_write(zend_object *object, zval *key, zval *write, zend_bool } if (pthreads_store_save_zval(&threaded->owner, &zstorage, write) != SUCCESS) { - return FAILURE; + return WRITE_FAIL_NOT_THREAD_SAFE; } if (pthreads_monitor_lock(&ts_obj->monitor)) { @@ -523,10 +542,26 @@ int pthreads_store_write(zend_object *object, zval *key, zval *write, zend_bool coerced = pthreads_store_coerce(key, &member); } - zend_bool was_pthreads_object = pthreads_store_member_is_cacheable(object, &member); - result = pthreads_store_update_shared_property(ts_obj, &member, &zstorage); - if (result == SUCCESS && was_pthreads_object) { - _pthreads_store_bump_modcount_nolock(threaded); + zend_bool ok = true; + if (!overwrite) { + if (Z_TYPE(member) == IS_LONG) { + ok = !zend_hash_index_exists(&ts_obj->props.hash, Z_LVAL(member)); + } else { + ok = !zend_hash_exists(&ts_obj->props.hash, Z_STR(member)); + } + if (!ok) { + result = WRITE_FAIL_WOULD_OVERWRITE; + } + } + + if (ok) { + zend_bool was_pthreads_object = pthreads_store_member_is_cacheable(object, &member); + if (pthreads_store_update_shared_property(ts_obj, &member, &zstorage) == SUCCESS) { + result = WRITE_SUCCESS; + if (was_pthreads_object) { + _pthreads_store_bump_modcount_nolock(threaded); + } + } } //this isn't necessary for any specific property write, but since we don't have any other way to clean up local //cached Threaded references that are dead, we have to take the opportunity @@ -535,7 +570,7 @@ int pthreads_store_write(zend_object *object, zval *key, zval *write, zend_bool pthreads_monitor_unlock(&ts_obj->monitor); } - if (result != SUCCESS) { + if (result != WRITE_SUCCESS) { pthreads_store_storage_dtor(&zstorage); } else { pthreads_store_update_local_property(&threaded->std, &member, write); @@ -547,6 +582,10 @@ int pthreads_store_write(zend_object *object, zval *key, zval *write, zend_bool return result; } /* }}} */ +int pthreads_store_write(zend_object* object, zval* key, zval* write, zend_bool coerce_array_to_threaded) { + return pthreads_store_write_ex(object, key, write, coerce_array_to_threaded, true) == WRITE_SUCCESS ? SUCCESS : FAILURE; +} + /* {{{ */ int pthreads_store_count(zend_object *object, zend_long *count) { pthreads_object_t* ts_obj = PTHREADS_FETCH_TS_FROM(object); diff --git a/src/store.h b/src/store.h index d805a6d0..aa4f449a 100644 --- a/src/store.h +++ b/src/store.h @@ -34,6 +34,13 @@ typedef struct _pthreads_store_t { zend_long modcount; } pthreads_store_t; +typedef enum { + WRITE_SUCCESS = 0, + WRITE_FAIL_UNKNOWN = -1, + WRITE_FAIL_NOT_THREAD_SAFE = -2, + WRITE_FAIL_WOULD_OVERWRITE = -3 +} pthreads_store_write_result; + void pthreads_store_init(pthreads_store_t* store); void pthreads_store_destroy(pthreads_store_t* store); void pthreads_store_sync_local_properties(zend_object* object); @@ -41,8 +48,10 @@ void pthreads_store_full_sync_local_properties(zend_object *object); int pthreads_store_merge(zend_object *destination, zval *from, zend_bool overwrite, zend_bool coerce_array_to_threaded); int pthreads_store_delete(zend_object *object, zval *key); int pthreads_store_read(zend_object *object, zval *key, int type, zval *read); +int pthreads_store_read_local_property(zend_object* object, zend_string* key, int type, zval* read); zend_bool pthreads_store_isset(zend_object *object, zval *key, int has_set_exists); int pthreads_store_write(zend_object *object, zval *key, zval *write, zend_bool coerce_array_to_threaded); +pthreads_store_write_result pthreads_store_write_ex(zend_object *object, zval *key, zval *write, zend_bool coerce_array_to_threaded, zend_bool overwrite); void pthreads_store_tohash(zend_object *object, HashTable *hash); int pthreads_store_shift(zend_object *object, zval *member); int pthreads_store_chunk(zend_object *object, zend_long size, zend_bool preserve, zval *chunk); From d643d0c538b92d55017c21ef8699f3b940ab4072 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sat, 18 Mar 2023 17:05:38 +0000 Subject: [PATCH 2/2] added test --- tests/readonly-properties-thread-safe.phpt | 137 +++++++++++++++++++++ 1 file changed, 137 insertions(+) create mode 100644 tests/readonly-properties-thread-safe.phpt diff --git a/tests/readonly-properties-thread-safe.phpt b/tests/readonly-properties-thread-safe.phpt new file mode 100644 index 00000000..a358fb9a --- /dev/null +++ b/tests/readonly-properties-thread-safe.phpt @@ -0,0 +1,137 @@ +--TEST-- +Test that readonly properties behave as expected on thread-safe class descendents +--DESCRIPTION-- +pthreads doesn't (yet) mirror the exact behaviour of readonly properties on standard objects +for now, this test just ensures that the behaviour plays out as expected with lockless reads, +since those use local cache without locks if possible. +--SKIPIF-- + +--FILE-- +a = 1; + $this->array = new \ThreadedArray(); + } + + public function unsetProperties() : void{ + try{ + unset($this->a); + }catch(\Error $e){ + echo $e->getMessage() . PHP_EOL; + } + try{ + unset($this->array); + }catch(\Error $e){ + echo $e->getMessage() . PHP_EOL; + } + try{ + unset($this->initiallyUninit); + }catch(\Error $e){ + echo $e->getMessage() . PHP_EOL; + } + } + + public function writeProperties() : void{ + try{ + $this->a = 2; + }catch(\Error $e){ + echo $e->getMessage() . PHP_EOL; + } + try{ + $this->a++; + }catch(\Error $e){ + echo $e->getMessage() . PHP_EOL; + } + try{ + $this->array = new \ThreadedArray(); + }catch(\Error $e){ + echo $e->getMessage() . PHP_EOL; + } + try{ + $this->initiallyUninit = 2; + }catch(\Error $e){ + echo $e->getMessage() . PHP_EOL; + } + } + + public function readProperties() : void{ + var_dump($this->a); + var_dump($this->array); + var_dump($this->initiallyUninit); + } + + public function issetProperties() : void{ + var_dump(isset($this->a)); + var_dump(isset($this->array)); + var_dump(isset($this->initiallyUninit)); + } +} + +function test(T $t) : void{ + //these must run first, to ensure they work on an unpopulated local cache + $t->unsetProperties(); + $t->writeProperties(); + $t->issetProperties(); + $t->readProperties(); +} + +echo "--- main thread start ---\n"; +test(new T()); +echo "--- main thread end ---\n"; + +//init properties from the main thread - ensure that the child thread receives an empty local cache +$thread = new class(new T) extends \Thread{ + public function __construct( + private T $t + ){} + + public function run() : void{ + echo "--- child thread start ---\n"; + test($this->t); + echo "--- child thread end ---\n"; + } +}; +$thread->start(); +$thread->join(); +echo "OK\n"; +?> +--EXPECTF-- +--- main thread start --- +Cannot unset readonly property T::$a +Cannot unset readonly property T::$array +Cannot unset readonly property T::$initiallyUninit +Cannot modify readonly property T::$a +Cannot modify readonly property T::$a +Cannot modify readonly property T::$array +bool(true) +bool(true) +bool(true) +int(1) +object(ThreadedArray)#%d (0) { +} +int(2) +--- main thread end --- +--- child thread start --- +Cannot unset readonly property T::$a +Cannot unset readonly property T::$array +Cannot unset readonly property T::$initiallyUninit +Cannot modify readonly property T::$a +Cannot modify readonly property T::$a +Cannot modify readonly property T::$array +bool(true) +bool(true) +bool(true) +int(1) +object(ThreadedArray)#%d (0) { +} +int(2) +--- child thread end --- +OK