Conversation
Change zend_function_entry.flags to a uint64_t to that both ZEND_ACC_ and ZEND_ACC2_ flags can be represented. Introduce ZEND_FENTRY_FLAGS(flags, flags2) to pass ZEND_ACC2_ flags to ZEND_RAW_FENTRY(), ZEND_FENTRY(). Source-level backwards compatibility is maintained, as passing raw ZEND_ACC_ flags to ZEND_RAW_FENTRY(), ZEND_FENTRY() still works.
Functions that use zend_forbid_dynamic_call() must be flagged with ZEND_ACC2_FORBID_DYN_CALLS. In stubs, this is done by using @forbid-dynamic-calls. To ensure consistency, we assert that the flag exists in zend_forbid_dynamic_call(), and we assert that flagged functions thrown after a dynamic call.
arnaud-lb
force-pushed
the
forbid-dyn-call
branch
from
8b437c7 to
ab1b483
Compare
TimWolla
left a comment
TimWolla
left a comment
There was a problem hiding this comment.
Should Closure::getCurrent() be added to that list or does it work?
iluuu1994
left a comment
iluuu1994
left a comment
There was a problem hiding this comment.
Same comments as Tim, but LGTM otherwise.
I've checked, and |
| return ''; | ||
| } | ||
| return sprintf($codeTemplate, implode("|", $flagsByPhpVersions[$currentPhpVersion])); | ||
| return sprintf($codeTemplate, $this->formatFlags($flagsByPhpVersions[$currentPhpVersion])); |
There was a problem hiding this comment.
I'm wondering if ZEND_FENTRY_FLAGS should also be used when the minimum version compatibility is PHP 8.6? At least it would be useful later, when 8.6 will be a few years old, and there will be more ACC2 flags. That's why I think the other two returns should use generate ZEND_FENTRY_FLAGS when possible.
I'm not sure how I didn't see it immediately, but I've just realized that you have thought about this :) Very nice!
DanielEScherzer
left a comment
DanielEScherzer
left a comment
There was a problem hiding this comment.
So overall this makes sense, but copying from my note on #20848
Also, if you have that flag, you might not even need to call zend_forbid_dynamic_call() in each function, if you instead enforce the flag in the function dispatch logic. Otherwise there is a possibility that the flag is added to a function but not enforced with a zend_forbid_dynamic_call() call
And the reply from @arnaud-lb
This would add overhead for a something that is rare, so I would prefer not to. However, debug builds can verify that an exception was thrown after dynamically calling a flagged function.
I would suggest that the UPGRADING.INTERNALS be a bit stronger than
Functions using zend_forbid_dynamic_call() must be flagged with ZEND_ACC2_FORBID_DYN_CALLS (@forbid-dynamic-calls in stubs)
maybe
Functions using zend_forbid_dynamic_call() must be flagged with ZEND_ACC2_FORBID_DYN_CALLS (@forbid-dynamic-calls in stubs). In debug builds, failing to include that flag will lead to assertion failures.
or something to make it clear that this isn't just a suggestion
5 participants
Flag functions that forbid dynamic calls with
ZEND_ACC2_FORBID_DYN_CALLS, so that we can recognize them without making a hard-coded list.This will be used in the PFA implementation.
See individual commits for details.
UPGRADING.INTERNALS: