Support the actual #[\SensitiveParameter] attribute in stubs
#8836
Conversation
TimWolla
force-pushed
the
sensitive-parameter-attribute-stub
branch
from
d121af5 to
0f9321a
Compare
TimWolla
force-pushed
the
sensitive-parameter-attribute-stub
branch
from
0f9321a to
14b31f2
Compare
|
I suppose we can now also get rid of zend_mark_function_parameter_as_sensitive() and replace it by a proper mechanism akin to #8839 for arbitrary attributes. |
|
@bwoebi Was already working on it. Pushed now, PTAL 😃 |
| && $this->defaultValue === $other->defaultValue | ||
| && $this->isSensitive === $other->isSensitive; | ||
| && $this->defaultValue === $other->defaultValue; |
There was a problem hiding this comment.
Not sure about this part. I don't believe that attributes need to be part of equals(), as they are not part of the ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX stuff.
TimWolla
force-pushed
the
sensitive-parameter-attribute-stub
branch
from
06ecf2b to
5820abe
Compare
|
Rebased for #8931. |
build/gen_stub.php
Outdated
| if ($funcInfo->name instanceof MethodName) { | ||
| $functionTable = "&class_entry->function_table"; | ||
| } else { | ||
| $functionTable = "CG(function_table)"; | ||
| } | ||
|
|
||
| $code .= "\tzend_mark_function_parameter_as_sensitive($functionTable, \"" . $funcInfo->name->getNameForAttributes() . "\", $index);\n"; | ||
| foreach ($arg->attributes as $attribute) { | ||
| $code .= $attribute->generateCode("zend_add_parameter_attribute(zend_hash_str_find_ptr($functionTable, \"" . $funcInfo->name->getNameForAttributes() . "\", sizeof(\"" . $funcInfo->name->getNameForAttributes() . "\") - 1), $index", "{$funcInfo->getArgInfoName()}_arg{$index}", $allConstInfos); |
There was a problem hiding this comment.
I don't really like the fact that $funcInfo->getArgInfoName() is used for generating the attribute name, because the arginfo_ part is unnecessary (and I didn't realize for a while why it's there). Even though the format of $funcInfo->name->getMethodSynopsisFilename() looks a bit better, it would be weird to use this name here.. So either we should rename this method (getSnakeCasedName??) or just leave the name as-is.
There was a problem hiding this comment.
Thanks! $funcInfo->name->getMethodSynopsisFilename() is indeed much better. It's was hard for me to "discover" something appropriate to use and ->getArgInfoName() worked.
ext/openssl/openssl_arginfo.h
Outdated
| zend_mark_function_parameter_as_sensitive(CG(function_table), "openssl_pkey_derive", 1); | ||
| zend_mark_function_parameter_as_sensitive(CG(function_table), "openssl_spki_new", 0); | ||
|
|
||
| zend_string *attribute_name_SensitiveParameter_arginfo_openssl_x509_check_private_key_arg1 = zend_string_init("SensitiveParameter", sizeof("SensitiveParameter") - 1, 1); |
There was a problem hiding this comment.
I'm wondering if it would be beneficial to declare the different attribute names first and then use them in the zend_add_parameter_attribute() invocations. It would save quite a few lines when an extension has a lot of attributes with the same name
There was a problem hiding this comment.
It likely would, but that should probably be a follow-up, as it also affects the class attributes.
|
@kocsismate Thanks for the review! I've made the requested changes. Feel free to merge if you are happy with the new version. |
see #8689
see #8776