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); 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