# [PATCH] internal class' static properties and constants - Thread: https://externals.io/message/17835 - Original archive: https://news-web.php.net/php.internals/17835 - Started by Michael Wallner on 2005-08-11T14:55:58+00:00 - Messages: 13 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #17835 — Michael Wallner — 2005-08-11T14:55:58+00:00 URL: https://externals.io/message/17835#17835 Hi, I revised the previous patch to actually working code. Affected files: zend.h zend_API.c, zend_API.h zend_builtin_functions.c zend_compile.c zend_object_handlers.c zend_opcode.c zend_reflection_api.c The patch fixes a typo of the "original" zend_update_property_double(), value leakage of static properties accross requests and provides an API to update static properties and declare class constants. It seems to work fine for me with PHP-HEAD and Apache2/Prefork on Win32. To get static properties for internal classes working, one needs to declare the properties in MINIT as usual and initialize them with zend_init_static_properties(ce) in RINIT. Regards, -- Michael - < mike(@)php.net --- ## #17868 — Michael Wallner — 2005-08-12T13:54:58+00:00 URL: https://externals.io/message/17835#17868 · In reply to: #17835 The patches are online at: http://dev.iworks.at/PATCHES/ Thanks, -- Michael - < mike(@)php.net --- ## #17884 — Marcus Boerger — 2005-08-12T17:51:02+00:00 URL: https://externals.io/message/17835#17884 · In reply to: #17868 Hello Michael, can you do this again please for HEAD (no 5.1) now that the major unicode work is done? marcus Friday, August 12, 2005, 3:54:58 PM, you wrote: > The patches are online at: > http://dev.iworks.at/PATCHES/ > Thanks, > -- > Michael - < mike(@)php.net > Best regards, Marcus --- ## #17907 — Michael Wallner — 2005-08-12T19:00:11+00:00 URL: https://externals.io/message/17835#17907 · In reply to: #17884 Marcus Boerger wrote: > Hello Michael, > > can you do this again please for HEAD (no 5.1) now that the major unicode > work is done? Uhm, not until Tuesday, sorry. Though there's already a diff against HEAD, but it's now about 7 hours old, so it may be inapplicable. http://dev.iworks.at/PATCHES/ Thanks, Mike --- ## #17908 — Michael Wallner — 2005-08-12T19:03:58+00:00 URL: https://externals.io/message/17835#17908 · In reply to: #17907 I wrote: > Though there's already a diff against HEAD, but it's > now about 7 hours old, so it may be inapplicable. > > http://dev.iworks.at/PATCHES/ Actually it seems that there were no more commits, so the available patch should work fine. Regards, Mike --- ## #18152 — Andi Gutmans — 2005-08-16T00:59:36+00:00 URL: https://externals.io/message/17835#18152 · In reply to: #17835 Hi Michael, Thanks for sending this. Has anyone else reviewed it already? I'm assuming that this supports arrays too (unlike today as emalloc needs to be available for arrays) and has to be created/freed during RINIT/RSHUTDOWN... Andi At 03:54 PM 8/12/2005 +0200, Michael Wallner wrote: >The patches are online at: >http://dev.iworks.at/PATCHES/ > >Thanks, >-- >Michael - < mike(@)php.net --- ## #18157 — Michael Wallner — 2005-08-16T07:50:20+00:00 URL: https://externals.io/message/17835#18157 · In reply to: #18152 Hi Andi Gutmans, you wrote: > Hi Michael, > > Thanks for sending this. Has anyone else reviewed it already? I'm > assuming that this supports arrays too (unlike today as emalloc needs to > be available for arrays) and has to be created/freed during > RINIT/RSHUTDOWN... Yes, there are two convenience functions for RINIT/SHUTDOWN for a transparent API, which are actually not a MUST: zend_init_static_properties(my_ce TSRMLS_CC); zend_clean_static_properties(my_ce TSRMLS_CC); Regards, -- Michael - < mike(@)php.net --- ## #18165 — Michael Wallner — 2005-08-16T10:39:04+00:00 URL: https://externals.io/message/17835#18165 · In reply to: #18152 Hi Andi Gutmans, you wrote: > Thanks for sending this. Has anyone else reviewed it already? I don't know if Marcus had a look at it. I updated the patches to reflect current CVS. It should work with 5.1 pretty well, no idea about HEAD though, as I'm not familiar with the changes yet. Thanks, -- Michael - < mike(@)php.net --- ## #18186 — Marcus Boerger — 2005-08-16T21:19:52+00:00 URL: https://externals.io/message/17835#18186 · In reply to: #17868 Hello Michael, a few questions/comments so that i can understand better: 1) Zend/zend_API.c: +ZEND_API int zend_declare_class_constant_stringl(zend_class_entry *ce, char *name, size_t name_length, char *value, size_t value_length TSRMLS_DC) +{ + zval *constant = new_zval(ce->type & ZEND_INTERNAL_CLASS); + if (ce->type & ZEND_INTERNAL_CLASS) { + ZVAL_STRINGL(constant, zend_strndup(value, value_length), value_length, 0); + } else { + ZVAL_STRINGL(constant, value, value_length, 1); + } + return zend_declare_class_constant(ce, name, name_length, constant TSRMLS_CC); +} + If the internal part uses string duplication so shall use the user space code. 2) Zend/zend_API.c: +ZEND_API int zend_update_static_property(zend_class_entry *scope, char *name, size_t name_len, zval *value TSRMLS_DC) +{ + int retval; + zval **property = NULL; + zend_class_entry *old_scope = EG(scope); + + EG(scope) = scope; + + if (!(property = zend_std_get_static_property(scope, name, name_len, 0 TSRMLS_CC))) { + retval = FAILURE; in the caose you utilize this value is a temp thing thus you need to free it in all cases but those you directly use it. A failure is definitively a thing where you do't use it so it must be fr here. + } else if (*property == value) { + retval = SUCCESS; Here you compare the address of a zval, was that intended? In case you meant it, then i think it cannot be correct because that should never happen, or is that the case you actually want to prevent? In case you meant to prohibit from setting the same value again you need actual zval comparison which won't work easily for strings. + } else { + if (PZVAL_IS_REF(*property)) { + zval_dtor(*property); + (*property)->type = value->type; + (*property)->value = value->value; + + if (value->refcount) { + zval_copy_ctor(*property); + } + } else { + **property = *value; + zval_copy_ctor(*property); + } + retval = SUCCESS; + } + + if (!value->refcount) { + zval_dtor(value); + FREE_ZVAL(value); + } This only works because your zvals are initialized with refcount=0 which cannot be right. In the end it should always read here zval_ptr_dtor(&value); And tmp_zval() should initialize with refcount=1. + + EG(scope) = old_scope; + + return retval; +} Your static properties get duplicated and const updated from 3) Zend/zend.c: static void class_to_unicode(zend_class_entry **ce) + zend_u_hash_init_ex(&new_ce->default_static_properties, (*ce)->default_static_properties.nNumOfElements, NULL, (*ce)->default_static_properties.pDestructor, 1, 1, 0); + zend_hash_copy(&new_ce->default_static_properties, &(*ce)->default_static_properties, (copy_ctor_func_t) zval_ptr_to_unicode, &tmp_zval, sizeof(zval)); + zend_u_hash_init_ex(&new_ce->static_properties, (*ce)->static_properties.nNumOfElements, NULL, (*ce)->default_static_properties.pDestructor, 1, 1, 0); Why is the above not using "(*ce)->default_static_properties.nNumOfElements", since that is the number of entries the static property table will have exactly after the copy operation on the next line so why not initialize the number to this? + zend_hash_copy(&new_ce->static_properties, &(*ce)->static_properties, (copy_ctor_func_t) zval_ptr_to_unicode, &tmp_zval, sizeof(zval)); 4) Zend/zend_API.c: ZEND_API void zend_update_class_constants(zend_class_entry *class_type TSRMLS_DC) zend_hash_apply_with_argument(&class_type->default_properties, (apply_func_arg_t) zval_update_constant, (void *) 1 TSRMLS_CC); - zend_hash_apply_with_argument(class_type->static_members, (apply_func_arg_t) zval_update_constant, (void *) 1 TSRMLS_CC); + zend_hash_apply_with_argument(&class_type->default_static_properties, (apply_func_arg_t) zval_update_constant, (void *) 1 TSRMLS_CC); Before your patch we have the situation that consts can only be decalred in user space. Thus it is ensured that their data is emalloc'd and only ever needs to be const-updated once. Now that you have this as well as your default static properties it (zend_update_class_constants()) must be called once for every request time. Having said that i think we actuall need two tables. cor consts as well. I think default_consts and default_static_properies never need to be const updated because otherwise you wouldn't react on define changes if a defined const is being referred to. With this in mind we also face another problem, the problem that const-update assumes that the memory was emalloc'd.So what we need to do is ensuring that on the first use in a request the two tables default_const and default_static_properties get copied to their working tables consts and static_properties. After that copy we can update the consts. Now we also ensured that for internal classes the values in the defualt_const table and default_static_properties table can become malloc'd. 5) Zend/zend_compile.c: ZEND_API void zend_initialize_class_data(zend_class_entry *ce, zend_bool nullify_handlers TSRMLS_DC) + zend_u_hash_init_ex(&ce->default_static_properties, 0, NULL, zval_ptr_dtor_func, persistent_hashes, UG(unicode), 0); + zend_u_hash_init_ex(&ce->static_properties, 0, NULL, ZVAL_PTR_DTOR, persistent_hashes, UG(unicode), 0); After reading 4) it should be clear that the values in default_static_properties must be malloc'c zval's. With this in mind we of course need to specify a corresponding free method insetad of the currently used one that assumes the memory was emalloc'd. 6) Renamic static_members to static_properties is a very good idea if you ask me but it requires that we have this patch ready and set before 5.1 is out. best regards marcus & nice work so far Friday, August 12, 2005, 3:54:58 PM, you wrote: > The patches are online at: > http://dev.iworks.at/PATCHES/ > Thanks, > -- > Michael - < mike(@)php.net > Best regards, Marcus --- ## #18192 — Michael Wallner — 2005-08-17T07:14:50+00:00 URL: https://externals.io/message/17835#18192 · In reply to: #18186 Hi Marcus Boerger, you wrote: > Hello Michael, > > a few questions/comments so that i can understand better: > > 1) Zend/zend_API.c: > +ZEND_API int zend_declare_class_constant_stringl(zend_class_entry *ce, char *name, size_t name_length, char *value, size_t value_length TSRMLS_DC) > +{ > + zval *constant = new_zval(ce->type & ZEND_INTERNAL_CLASS); > + if (ce->type & ZEND_INTERNAL_CLASS) { > + ZVAL_STRINGL(constant, zend_strndup(value, value_length), value_length, 0); > + } else { > + ZVAL_STRINGL(constant, value, value_length, 1); > + } > + return zend_declare_class_constant(ce, name, name_length, constant TSRMLS_CC); > +} > + > > If the internal part uses string duplication so shall use the user space > code. Yes, the internal part uses zend_strndup() while the userspace part - I guess - estrndup(). > 2) Zend/zend_API.c: > +ZEND_API int zend_update_static_property(zend_class_entry *scope, char *name, size_t name_len, zval *value TSRMLS_DC) > +{ > + int retval; > + zval **property = NULL; > + zend_class_entry *old_scope = EG(scope); > + > + EG(scope) = scope; > + > + if (!(property = zend_std_get_static_property(scope, name, name_len, 0 TSRMLS_CC))) { > + retval = FAILURE; > in the caose you utilize this value is a temp thing thus you need to free it > in all cases but those you directly use it. A failure is definitively a > thing where you do't use it so it must be fr here. > + } else if (*property == value) { > + retval = SUCCESS; > Here you compare the address of a zval, was that intended? > In case you meant it, then i think it cannot be correct because that should > never happen, or is that the case you actually want to prevent? > In case you meant to prohibit from setting the same value again you need > actual zval comparison which won't work easily for strings. This is from zend_reflection_api ReflectionProperty::setValue(). I assume it is meant to prevent useless operations, because when else should the zvals point to the same address? > + } else { > + if (PZVAL_IS_REF(*property)) { > + zval_dtor(*property); > + (*property)->type = value->type; > + (*property)->value = value->value; > + > + if (value->refcount) { > + zval_copy_ctor(*property); > + } > + } else { > + **property = *value; > + zval_copy_ctor(*property); > + } > + retval = SUCCESS; > + } > + > + if (!value->refcount) { > + zval_dtor(value); > + FREE_ZVAL(value); > + } > This only works because your zvals are initialized with refcount=0 which > cannot be right. In the end it should always read here > zval_ptr_dtor(&value); And tmp_zval() should initialize with refcount=1. I actually thought that there should be an "else { zval_ptr_dtor(&value) }"? Every place I looked at where "temporary zvals" are used, initialize them with refcount=0. > + > + EG(scope) = old_scope; > + > + return retval; > +} > > Your static properties get duplicated and const updated from > > 3) Zend/zend.c: static void class_to_unicode(zend_class_entry **ce) > + zend_u_hash_init_ex(&new_ce->default_static_properties, (*ce)->default_static_properties.nNumOfElements, NULL, (*ce)->default_static_properties.pDestructor, 1, 1, 0); > + zend_hash_copy(&new_ce->default_static_properties, &(*ce)->default_static_properties, (copy_ctor_func_t) zval_ptr_to_unicode, &tmp_zval, sizeof(zval)); > + zend_u_hash_init_ex(&new_ce->static_properties, (*ce)->static_properties.nNumOfElements, NULL, (*ce)->default_static_properties.pDestructor, 1, 1, 0); > Why is the above not using > "(*ce)->default_static_properties.nNumOfElements", since that is the number > of entries the static property table will have exactly after the copy > operation on the next line so why not initialize the number to this? Yeah, this is a matter of optimization which I was not thinking about in the first place, sorry. > + zend_hash_copy(&new_ce->static_properties, &(*ce)->static_properties, (copy_ctor_func_t) zval_ptr_to_unicode, &tmp_zval, sizeof(zval)); > > 4) Zend/zend_API.c: ZEND_API void zend_update_class_constants(zend_class_entry *class_type TSRMLS_DC) > zend_hash_apply_with_argument(&class_type->default_properties, (apply_func_arg_t) zval_update_constant, (void *) 1 TSRMLS_CC); > - zend_hash_apply_with_argument(class_type->static_members, (apply_func_arg_t) zval_update_constant, (void *) 1 TSRMLS_CC); > + zend_hash_apply_with_argument(&class_type->default_static_properties, (apply_func_arg_t) zval_update_constant, (void *) 1 TSRMLS_CC); > Before your patch we have the situation that consts can only be decalred in > user space. Thus it is ensured that their data is emalloc'd and only ever > needs to be const-updated once. Now that you have this as well as your > default static properties it (zend_update_class_constants()) must be called > once for every request time. Having said that i think we actuall need two > tables. cor consts as well. I think default_consts and > default_static_properies never need to be const updated because otherwise > you wouldn't react on define changes if a defined const is being referred > to. With this in mind we also face another problem, the problem that > const-update assumes that the memory was emalloc'd.So what we need to do is > ensuring that on the first use in a request the two tables default_const and > default_static_properties get copied to their working tables consts and > static_properties. After that copy we can update the consts. Now we also > ensured that for internal classes the values in the defualt_const table and > default_static_properties table can become malloc'd. Well, I didn't really get the idea of this const updating yet, so apologize if I'm totally wrong here. Ok, so it is a problem that the const updating routine (whatever it does) assumes that memory was emalloc()'d but why do constants_table and default_static_properties need to be updated for every request? They're not supposed to change their values after declaration, are they? > 5) Zend/zend_compile.c: ZEND_API void zend_initialize_class_data(zend_class_entry *ce, zend_bool nullify_handlers TSRMLS_DC) > + zend_u_hash_init_ex(&ce->default_static_properties, 0, NULL, zval_ptr_dtor_func, persistent_hashes, UG(unicode), 0); > + zend_u_hash_init_ex(&ce->static_properties, 0, NULL, ZVAL_PTR_DTOR, persistent_hashes, UG(unicode), 0); > After reading 4) it should be clear that the values in > default_static_properties must be malloc'c zval's. With this in mind we of > course need to specify a corresponding free method insetad of the currently > used one that assumes the memory was emalloc'd. Hm...? default_static_properties' destructor is zval_internal_ptr_dtor, so? > 6) Renamic static_members to static_properties is a very good idea if you > ask me but it requires that we have this patch ready and set before 5.1 is > out. Well, I'm the one who most strongly hopes that, as my HttpResponse class is just a bad hack until so-called v6. As you probably know, I'm a first time extension writer and still learning a lot and my knowledge of the internals is still quite low. Thanks, -- Michael - < mike(@)php.net --- ## #18199 — Michael Wallner — 2005-08-17T18:20:48+00:00 URL: https://externals.io/message/17835#18199 · In reply to: #18192 I wrote: > Hi Marcus Boerger, you wrote: > > […] Now that I know what zval_update_constant() is good for... Is it even needed to call zend_update_class_constants() for ZEND_INTERNAL_CLASSes? One most probably won't declare a property of an internal class with a zval that's IS_CONSTANT. Thoughts? -- Michael - < mike(@)php.net --- ## #18200 — Marcus Boerger — 2005-08-17T19:58:54+00:00 URL: https://externals.io/message/17835#18200 · In reply to: #18192 Hello Michael, Wednesday, August 17, 2005, 9:14:50 AM, you wrote: > Hi Marcus Boerger, you wrote: > > […] > Yes, the internal part uses zend_strndup() while the userspace part > - I guess - estrndup(). Oh, now i see it there is a 1 for copy which means estrndup(). > > […] > This is from zend_reflection_api ReflectionProperty::setValue(). > I assume it is meant to prevent useless operations, > because when else should the zvals point to the same address? Yep in reflection api it is used for the exact same reason. I was only wondering whether that can happen here. Since your code is called outside request time it can only happen if some code tries to update the same prop with the same value twice. > > […] > I actually thought that there should be an "else { zval_ptr_dtor(&value) }"? > Every place I looked at where "temporary zvals" are used, initialize them > with refcount=0. Temp zvals are used in scripts and can never be stored at least that was the goal. In your case always and unfortunatley also in the engine usage for edge cases they are going to be stored. So probably it is easier here to go with normal props initilized with refcount=1. You have to try and see. > > […] > Yeah, this is a matter of optimization which I was not thinking about > in the first place, sorry. > > […] > Well, I didn't really get the idea of this const updating yet, so apologize > if I'm totally wrong here. Ok, so it is a problem that the const updating > routine (whatever it does) assumes that memory was emalloc()'d but why do > constants_table and default_static_properties need to be updated for > every request? They're not supposed to change their values after > declaration, are they? php -r '$d=42; define("a",$d); class t { const x=a;} var_dump(t::x);' That's why. > > […] > Hm...? default_static_properties' destructor is zval_internal_ptr_dtor, so? Ah, yes, you're right here too, i didn't see the full context and got wrong with all those function names somehow. Sorry for getting this mail out that late....i thought it was sent in the morning. Best regards, Marcus --- ## #18202 — Michael Wallner — 2005-08-18T07:14:01+00:00 URL: https://externals.io/message/17835#18202 · In reply to: #18200 Hi Marcus Boerger, you wrote: > Hello Michael, > > Wednesday, August 17, 2005, 9:14:50 AM, you wrote: > > […] > > > […] > > Yep in reflection api it is used for the exact same reason. I was only > wondering whether that can happen here. Since your code is called outside > request time it can only happen if some code tries to update the same prop > with the same value twice. It theoretically avoids zpp = zend_std_get_static_property(...); zend_update_static_property(..., *zpp); Also zend_std_write_property uses the same bit, so I think better be on the safe side... > > […] > > > > […] > > > Temp zvals are used in scripts and can never be stored at least that was the > goal. In your case always and unfortunatley also in the engine usage for > edge cases they are going to be stored. So probably it is easier here to go > with normal props initilized with refcount=1. You have to try and see. zend_update_property() and zend_std_write_property() use the same form of temporary zvals, though the destruction part of the passed value zval should probably be adopted to that of zend_update_property(). > > […] > > > php -r '$d=42; define("a",$d); class t { const x=a;} var_dump(t::x);' > > That's why. So, what about disallowing IS_CONSTANT zvals for internal zvals as already done for arrays or other complex types and never actually zval_update_constant() those zvals and skip internal classes in zend_update_class_constants()? Regards, -- Michael - < mike(@)php.net