Zend: handle IS_UNDEF in the generic conversion helpers that still lack it#22824
Closed
brzuchal wants to merge 1 commit into
Closed
Zend: handle IS_UNDEF in the generic conversion helpers that still lack it#22824brzuchal wants to merge 1 commit into
brzuchal wants to merge 1 commit into
Conversation
…ck it
IS_UNDEF is a legitimate runtime tag that internal code can obtain from an
object property table, and the generic conversion helpers in zend_operators.c
are ZEND_API entry points taking an arbitrary zval. Some already account for
that and some do not:
handled: zendi_try_get_long(), zval_get_long_func(),
__zval_get_string_func(), _convert_to_string()
missing: _zendi_try_convert_scalar_to_number(), convert_to_long(),
convert_to_double(), convert_to_boolean(), zval_get_double_func()
The asymmetry is what is left after two point fixes. 2b38384 taught
zval_try_get_long() about IS_REFERENCE and 9e1b285 (phpGH-22142) taught
zendi_try_get_long() about IS_UNDEF, each by adding the tag to the one
function that had been reported. The siblings were not revisited, so
zval_get_long(undef) returns 0 while zval_get_double(undef) reaches
ZEND_UNREACHABLE(): an assertion failure in a debug build, undefined
behaviour in a release build.
Add the missing cases to arms that already express the intended result, so
IS_UNDEF behaves as IS_NULL does for the cast helpers and produces a normal
TypeError for the operator helper, matching the bitwise operators which are
already safe through zendi_try_get_long().
increment_function() and decrement_function() are deliberately not changed.
IS_UNDEF must not reach them: every VM handler normalizes it to IS_NULL
before the call (zend_vm_def.h), and the JIT asserts the precondition
outright (zend_jit_helpers.c). Accepting it there would weaken a check
another subsystem relies on.
No behaviour changes for any tag that works today, and no catch-all default
is added: IS_CONSTANT_AST, IS_INDIRECT, IS_PTR, IS_ALIAS_PTR and _IS_ERROR
indicate a caller bug and must keep failing loudly.
Girgias
requested changes
Jul 20, 2026
Girgias
left a comment
Member
There was a problem hiding this comment.
If you have an IS_UNDEF zval at this point you have massive problems, this is not something that should happen.
Member
|
As for your references: |
Contributor
Author
|
Thanks, that distinction makes sense. I'll revisit the affected call sites instead. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Up front: no userland reproducer
This is a consistency fix and I could not construct a PHP-level trigger: every VM handler
normalizes
IS_UNDEFtoIS_NULLbefore calling these helpers. I would rather state that thanhave it asked. The argument is the asymmetry below plus the fact that this exact bug class has
already been fixed twice upstream, both times by adding the missing
case. If the view is thata reproducer is required, I am happy for this to be closed.
The asymmetry
These are
ZEND_APIentry points taking an arbitrary zval, andIS_UNDEFis a tag internalcode legitimately holds, most obviously from an object property table, where uninitialized
typed properties are
IS_UNDEF. Four helpers account for it, five do not:IS_UNDEFzendi_try_get_long()_zendi_try_convert_scalar_to_number()zval_get_long_func()zval_get_double_func()__zval_get_string_func()convert_to_long()_convert_to_string()convert_to_double()convert_to_boolean()zval_get_long(undef)returns0;zval_get_double(undef)reachesZEND_UNREACHABLE(), whichis an assertion failure in a debug build and undefined behaviour in a release build. Sibling
APIs, same contract, and the difference does not look designed.
Why it looks like this
Two point fixes, each scoped to the single function that had been reported:
2b383848a73zval_try_get_long()(#18761), adding
IS_REFERENCE. Its commitmessage is the precedent: "This API can't handle references, yet everyone keeps forgetting
that it can't and that you should DEREF upfront."
9e1b285f65azendi_try_get_long(const zval *, _Bool *): Assertion '0' failed.#22142: Assertion failure inzendi_try_get_long()on IS_UNDEF(#22142, fixed by #22143), found by fuzzing, adding
IS_UNDEF.Neither time were the neighbouring helpers revisited. This finishes that sweep.
What it does
Adds
case IS_UNDEF:to five helpers, each joining an arm that already exists:IS_UNDEFbehaves as
IS_NULLfor the cast helpers, and yields a normalTypeErrorfor the operatorhelper, matching the bitwise operators, which are already safe through
zendi_try_get_long().What it deliberately does not do
increment_function()anddecrement_function()are left alone.IS_UNDEFreaching them is agenuine invariant violation, not an input to coerce: every VM handler normalizes it before the
call (
zend_vm_def.h), and the JIT asserts the precondition outright(
ZEND_ASSERT(Z_TYPE_P(var_ptr) != IS_UNDEF), four sites inzend_jit_helpers.c). Acceptingit there would weaken a check another subsystem relies on.
No catch-all
default:is added either.IS_CONSTANT_AST,IS_INDIRECT,IS_PTR,IS_ALIAS_PTRand_IS_ERRORindicate a caller bug and must keep failing loudly.Related
Companion fix, same bug class at a call site rather than in the helper:
#22822. Independent, either can merge first.
Behaviour and tests
No behaviour change for any tag that works today.
Zend/tests,ext/date/tests,ext/zlib/tests: 6039 pass, no failures attributable to this change.No test is added: without a userland path reaching these helpers with
IS_UNDEF, the only wayto write one would be a test-only API for fabricating zval tags, which seems a worse trade than
shipping the fix untested.