Skip to content

Fix GH-16572: Incorrect result with reflection in low-trigger JIT #16575

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

nielsdos
Copy link
Member

When a recursive call happens with invalid arguments, the maximum valid arguments are computed and stored in num_args, but the RECV entry block we jump to is call_num_args instead. This can skip argument validation checks. Fix this by using num_args instead.

When a recursive call happens with invalid arguments, the maximum valid
arguments are computed and stored in `num_args`, but the RECV entry
block we jump to is `call_num_args` instead. This can skip argument
validation checks. Fix this by using `num_args` instead.
@nielsdos nielsdos marked this pull request as ready for review October 24, 2024 20:27
@nielsdos nielsdos requested a review from dstogov as a code owner October 24, 2024 20:27
Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix looks right.
I have no idea why this bug was introduced during switching to IR.

Probably call_num_args also should be replaced to num_args on line 10178

-					if (!trace && op_array == &func->op_array && call_num_args >= op_array->required_num_args) {
+					if (!trace && op_array == &func->op_array && num_args >= op_array->required_num_args) {

@@ -9887,6 +9887,7 @@ static int zend_jit_do_fcall(zend_jit_ctx *jit, const zend_op *opline, const zen
if ((!func || func->type == ZEND_USER_FUNCTION)
&& opline->opcode != ZEND_DO_ICALL) {
bool recursive_call_through_jmp = 0;
uint32_t num_args;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to initialize it to avoid warnings.

@nielsdos
Copy link
Member Author

The fix looks right. I have no idea why this bug was introduced during switching to IR.

It seems JIT in 8.3 and below just don't do the call->jmp optimization.

Probably call_num_args also should be replaced to num_args on line 10178

-					if (!trace && op_array == &func->op_array && call_num_args >= op_array->required_num_args) {
+					if (!trace && op_array == &func->op_array && num_args >= op_array->required_num_args) {

Unless I misunderstand the code, I don't think this is right.
My understand is that: call_num_args is the number of passed arguments, while num_args is the number of arguments that are definitely valid and don't need argument validation. We can do the call->jmp optimization even if not all arguments are valid.

Consider the following code sample, modified from the test in the PR:

<?php
function dumpType(int $x, ReflectionType $rt) {
    dumpType(1, null);
}
function test1(): int { }
dumpType(0, (new ReflectionFunction('test1'))->getReturnType());

I added an int parameter. This is valid, so num_args is 1 and call_num_args is 2. If I make your suggested change, then the recursive call will actually be a call instead of a jmp into the entry. When I keep the code like it is now, it results in this assembly:

JIT$dumpType:
	cmpl $1, 0x2c(%r14) ; argument count check
	jl .L8
	cmpb $4, 0x58(%r14) ; type check for first argument to be IS_LONG
	jne .L6
.ENTRY_1:
	cmpl $2, 0x2c(%r14) ; argument count check
	jl .L7
; ... etc

.L1:
; (...) a lot of code
	leaq EG(current_execute_data)(%rip), %rax
	movq %r15, (%rax)
	movq %r15, %r14
	leaq -0x77288ef(%rip), %r15
	jmp .ENTRY_1 ; call->jmp optimization, we can skip first argument check but not second one

This seems right to me unless I miss something.

@dstogov
Copy link
Member

dstogov commented Oct 28, 2024

This seems right to me unless I miss something.

You are right. Please merge this as is.

@nielsdos nielsdos closed this in 38e1b0a Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect result with reflection in low-trigger JIT
2 participants