Resolve Inconsistency in zend_jit_return Between JIT and Non-JIT Engines

[segrax]

This resolves an inconsistency in both the x86 and ARM64 implementation of zend_jit_return between the JIT and non-JIT engines (op: ZEND_RETURN_SPEC_OBSERVER).

Issue

• Non-JIT Engine: zend_observer_fcall_end is invoked after the returned zval is copied into execute_data->return_value.
• JIT Engine: zend_observer_fcall_end is invoked before the returned zval is copied.

Behaviour

Extensions using the value in execute_data->return_value behave differently in JIT mode, which can lead to incorrect readings, and/or segfaults if values are being modified.

Affects

PHP 8.1 to 8.4 (issue is present in the new JIT)

Tests

I've extended the zend_test extension to allow hooks to be executed from calls to zend_observer_fcall_end, and added a test which modifies the return value

[devnexen]

I asked to the rest of the team if your PR would qualify as security fix (8.1 is EOL), if not then it would need to target PHP-8.2 instead. We shall see.

[segrax]

I asked to the rest of the team if your PR would qualify as security fix (8.1 is EOL), if not then it would need to target PHP-8.2 instead. We shall see.

Ah, I misread the contributing guide and version table. I'm happy to rebase on 8.2

[segrax]

Re-based to PHP-8.2

[segrax]

There was another small fix regarding parameters,

In non-JIT mode, IS_CONST types pass a pointer to a temporary _zval_struct into zend_observer_fcall_end for the retval parameter, resulting in two different _zval_struct objects with a matching zend_value. The new hook function in zend_test copies the zval from the executed hook return value to execute_data->return_value. Consequently, the retval parameter in observer_end still referred to the original zval stored in the temporary _zval_struct.

This caused the test to display different return values for JIT and non-JIT modes, while still producing the expected final result outside the observer.

Data comparison inside observer_end:

BEFORE 2nd Fix:
JIT:
execute_data->return_value: ptr:d0216080 zval.value.lval:41b3cf90
retval:			    ptr:421e0830 zval.value.lval:41b3cf90

NON:
execute_data->return_value: ptr:fc016080 zval.value.lval:4065bf90
retval:			    ptr:fc016080 zval.value.lval:4065bf90

AFTER 2nd Fix:
JIT:
execute_data->return_value: ptr:b5016080 zval.value.lval:6aa32c78
retval:			    ptr:b5016080 zval.value.lval:6aa32c78

NON:
execute_data->return_value: ptr:c1616080 zval.value.lval:20232c78
retval:			    ptr:c1616080 zval.value.lval:20232c78

[iluuu1994]

@segrax Thank you for your PR! Note that Dmitry, who is responsible for the JIT, is on vacation for the next two weeks. So it might take a minute for you to get a proper review.

[dstogov]

Can you please try to minimize the patch, avoiding indentation changes. e.g.

	if (return_value_used == 0) {
		/* pass */
	} else if (opline->op1_type == IS_CONST) {

[segrax]

Yep, done

[dstogov]

cc: @bwoebi

[dstogov]

Thanks!
Now it's clear that the patch doesn't affect JIT without observer.
Porting to master also should be simpler.

@bwoebi please check the observer related behavior and merge this.

[bwoebi]

I'm confused, like the patch seems to be only concerned with the pass case and IS_CONST and IS_TMP_VAR?

Reading the IS_CV case, isn't it sending the pointer to the CV rather than the ret_addr to zend_observer_fcall_end?
And same for the IS_TMP_VAR and the IS_VAR case (the else part)?

I must admit I don't really understand what the

		if (Z_MODE(op1_addr) == IS_REG) {
			zend_jit_addr dst = ZEND_ADDR_MEM_ZVAL(ZREG_FP, opline->op1.var);

			if (!zend_jit_spill_store(Dst, op1_addr, dst, op1_info, 1)) {
				return 0;
			}
			op1_addr = dst;
		}

part is doing.

At least - IF the value is returned - it's always supposed to be the return address, not sometimes op1_addr. Like, wouldn't this be correct:

	if (return_value_used == 0) {
		if (ZEND_OBSERVER_ENABLED && Z_MODE(op1_addr) == IS_REG) {
			zend_jit_addr dst = ZEND_ADDR_MEM_ZVAL(ZREG_FP, opline->op1.var);

			if (!zend_jit_spill_store(Dst, op1_addr, dst, op1_info, 1)) {
				return 0;
			}
			op1_addr = dst;
		}
		ret_addr = op1_addr;
	} else if (..) {
		// everything else unchanged here for IS_CV/VAR/TMP_VAR/CONST handling
	}

	if (ZEND_OBSERVER_ENABLED) {
		|	LOAD_ZVAL_ADDR FCARG2a, ret_addr
		|	mov FCARG1a, FP
		|	SET_EX_OPLINE opline, r0
		|	EXT_CALL zend_observer_fcall_end, r0
	}

Maybe @dstogov can help here?

[dstogov]

The last @bwoebi suggestion seems right!

[segrax]

I'm confused, like the patch seems to be only concerned with the pass case and IS_CONST and IS_TMP_VAR?

@bwoebi yeah, currently it only fixes the issue i found while working with the OpenTelemetry extension, there is likely the same issue with the other types, but too be honest, i wasn't entirely sure what everything here was doing.

Figured I would submit what I had, and work from there

At least - IF the value is returned - it's always supposed to be the return address, not sometimes op1_addr. Like, wouldn't this be correct:

	if (return_value_used == 0) {
		if (ZEND_OBSERVER_ENABLED && Z_MODE(op1_addr) == IS_REG) {
			zend_jit_addr dst = ZEND_ADDR_MEM_ZVAL(ZREG_FP, opline->op1.var);

			if (!zend_jit_spill_store(Dst, op1_addr, dst, op1_info, 1)) {
				return 0;
			}
			op1_addr = dst;
		}
		ret_addr = op1_addr;
	} else if (..) {
		// everything else unchanged here for IS_CV/VAR/TMP_VAR/CONST handling
	}

	if (ZEND_OBSERVER_ENABLED) {
		|	LOAD_ZVAL_ADDR FCARG2a, ret_addr
		|	mov FCARG1a, FP
		|	SET_EX_OPLINE opline, r0
		|	EXT_CALL zend_observer_fcall_end, r0
	}

It seems this change causes a test failure, but only in the debug builds (my local build is fine, but I also haven't checked what flags are being used in the debug pipeline builds).

EX(opline) is correctly set for nested JIT user code calls [ext/opcache/tests/jit/gh13772.phpt]

[dstogov]

@segrax did you see ext/opcache/tests/jit/gh13772.phpt test failure?

[segrax]

@segrax did you see ext/opcache/tests/jit/gh13772.phpt test failure?

Hey @dstogov,

Yeah, I did see the failure, it came from the change in the last recommendation, but it seems to have caused the test failure, So was waiting to hear back from @bwoebi before reverting it or making any further changes

It seems this change causes a test failure, but only in the debug builds (my local build is fine, but I also haven't checked what flags are being used in the debug pipeline builds).

EX(opline) is correctly set for nested JIT user code calls [ext/opcache/tests/jit/gh13772.phpt]

[dstogov]

OK. ping we when you'll need the next round of my review.

[segrax]

Hey @dstogov, I've reverted that last change

[dstogov]

This shouldn't change anything for JIT without observer.

I don't care about observer.
@bwoebi if this is right for you please take care about merging and porting to master.

[segrax]

Hey @dstogov and @bwoebi,

Just checking to see if there's been any progress on this,
Let me know if there’s anything I can help with to move things along

Thanks!