Fix infinite recursion on deprecated attribute evaluation #17712
Conversation
|
The failures are caused by modification of shm... I'll need to switch some things around but was too tired yesterday. |
iluuu1994
force-pushed
the
gh-17711
branch
2 times, most recently
from
1780131 to
0d31ece
Compare
|
I think it should be fine now. We only need to add recursion protection when the constant value is a constant AST. If it isn't, we won't attempt to evaluate it again. Recursive errors through |
| #define CONST_IS_RECURSIVE(c) (Z_CONSTANT_FLAGS((c)->value) & CONST_RECURSIVE) | ||
| #define CONST_PROTECT_RECURSION(c) \ | ||
| do { \ | ||
| if (Z_TYPE((c)->value) == IS_CONSTANT_AST) { \ |
There was a problem hiding this comment.
This doesn't seem to be enough to fix #16952, when I wrapped all zend_deprecated_constant() calls in recursion checks, because the type isn't IS_CONSTANT_AST, but rather appears to be IS_STRING.
Is there a reason to only add/remove the flag for IS_CONSTANT_AST? I was able to get things working by handling both IS_CONSTANT_AST and IS_STRING, but it seems a bit fragile to list just those types. Why not just do the addition/removal of CONST_RECURSIVE regardless of the value type?
There was a problem hiding this comment.
When the value is not a constant AST, it may live in shm which is read-only. I'll have to check why this doesn't work for you. For #[Deprecated(C)] const C = 'c';, the constant AST is inside the attribute, so it seems like this should work.
There was a problem hiding this comment.
@iluuu1994 is there anything I can do to help move this along? Or, do you want to merge this (since it does fix the existing bug) and move handling of non-AST constants to #16952 ?
There was a problem hiding this comment.
@DanielEScherzer I'll need to revisit this, because (as you correctly hinted to me), this fix doesn't work for GH-16952. I'll have a look again soon.
Fixes GH-17711
Discovered in GH-16952, where the issue is more obvious to reproduce. Optimally, this could have been moved into
zend_deprecated_class_constant()to avoid duplication, but that's not possible because it marks thecasconst.