Declare ext/soap properties #6759
Conversation
kocsismate
force-pushed
the
soap-properties
branch
2 times, most recently
from
7dd769c to
677d661
Compare
kocsismate
force-pushed
the
soap-properties
branch
from
677d661 to
b2e616b
Compare
ext/soap/php_encoding.c
Outdated
|
|
||
| if ((ztype = zend_hash_str_find_deref(ht, "enc_type", sizeof("enc_type")-1)) == NULL || | ||
| Z_TYPE_P(ztype) != IS_LONG) { | ||
| ztype = zend_read_property(soap_var_class_entry, Z_OBJ_P(data), "enc_type", sizeof("enc_type")-1, 1, &rv); |
There was a problem hiding this comment.
I don't know whether it's better to explicitly specify the class entry or I should just use Z_OBJCE_P(data) in all the zend_read_property() calls. Also, currently I pass 1 to the silent parameter, but probably it shouldn't be the case?
There was a problem hiding this comment.
As these are public properties, it doesn't really make a difference. But generally, this is the scope from which you access the property, so explicitly passing soap_var_class_entry is semantically correct.
Note that read_property doesn't return NULL, an undefined property is &EG(uninitialized_zval). I think using silent mode and explicitly reporting an error is okay in this file, because it uses some super broken error handling -- passing through the normal exception would be ideal, but it's probably not easily possible.
I suspect that you also need to add a ZVAL_DEREF here, don't think read_property does that by itself.
There was a problem hiding this comment.
The fact that &EG(uninitialized_zval) is returned means that the type is effectively null. So if you perform some type check, you can just drop the NULL pointer check entirely.
It may make sense to add a read_property_deref helper as this is probably going to be a common pattern for this type of code.
There was a problem hiding this comment.
Thanks for the hints! I like the idea of adding the helper. And I'll try to fix these issues the following days
There was a problem hiding this comment.
My newest commit (1c75f6d) addresses the review comments, so now only one test fails. Although error handling of property reading is still broken. :(
kocsismate
force-pushed
the
soap-properties
branch
4 times, most recently
from
c32892b to
1c75f6d
Compare
There was a problem hiding this comment.
Something that occurred to me is that strictly speaking, these uses of zend_read_property aren't safe, due to how the rv parameter is treated. Say, if a nefarious user were to extend one of those classes, unset the declared properties and add a __get() handler, then rv would end up being used, and would leak values. Of course, the same mistake is made in many other places :/ It's not easy to use the current API correctly, as it's a thin wrapper around the low-level handler.
| /** @var string|null */ | ||
| public $faultcode; | ||
|
|
||
| /** @var string|null */ |
There was a problem hiding this comment.
As far as I remember I got segfaults when I uncommented them, and I unfortunately didn't manage to find the root cause. I can retry it sometime soon.
There was a problem hiding this comment.
It's not easy to use the current API correctly, as it's a thin wrapper around the low-level handler.
Do you have any idea (like a new API) in mind? Or we should rather accept the fact that there are problems with zend_read_property()?
kocsismate
force-pushed
the
soap-properties
branch
from
1c75f6d to
ac36bec
Compare
|
Closing this PR because soap properties has recently became declared. |
No description provided.