fix(agent): Additional checks needed before iterating backwards in opcodes.

[zsistla]

nr_php_observer_attempt_call_cufa_handler is a new function with oapi. The equivalent function for non-oapi is in php_vm.c. In that case, it has been called while we are in the middle of php_execute and can assume certain things among them being we can just check the execute_data opcodes and that the start opcode is ZEND_DO_FCALL (of which we know certain facts) and iterate back as needed.

In legacy, to determine determine this is a call_user_func_array() call we have to look at the previous opcodes of zend_execute_data.
In oapi, to determine if this is a call_user_func_array() call we have to look at the previous opcodes of zend_execute_data->prev_execute_data.
For both we know:
ZEND_DO_FCALL will never be the first opcode in an op array -- minimally, there is always at least a ZEND_INIT_FCALL before it -- so it is safe to iterate backwards in the opcode like execute_data->prev_execute_data->opline - 1

In oapi, the path to the equivalent function didn't have a guaranteed existence of zend_execute_data and therefore was causing some segfaults (specifically noticed for PHP 8.2 on wordpress which makes extensive use of cufa for hook implementation). Additionally, we are not guaranteed that the prev_execute_data opcode is ZEND_DO_FCALL so we can't blindly iterate backwards on it.

This PR does two things:

1. Check if execute_data is NULL before attempting to access its elements.
2. verify the opcode is ZEND_DO_FCALL before we iterate back to determine if it was called from call_user_func_array.

Testing:
Build from source soak tests for PHP 8.2 and wordpress no longer segfault.

[zsistla]

ok jenkins

[lavarou]

@zsistla I would appreciate if you could you add some context to the description of this PR. I.e. what code path caused the execute_data to be NULL and under which circumstances this function was called when ZEND_DO_FCALL != execute_data->prev_execute_data->opline->opcode? This would be most helpful to understand the need for these changes.

[zsistla]

@zsistla I would appreciate if you could you add some context to the description of this PR. I.e. what code path caused the execute_data to be NULL and under which circumstances this function was called when ZEND_DO_FCALL != execute_data->prev_execute_data->opline->opcode? This would be most helpful to understand the need for these changes.

Please see updated description.

[zsistla]

ok jenkins

[zsistla]

ok jenkins

[zsistla]

ok jenkins

[zsistla]

ok jenkins

[lavarou]

  if (ZEND_DO_FCALL != execute_data->prev_execute_data->opline->opcode) {
    return;
  }

is the key in this PR and addresses the segfault, caused by OAPI's instrumentation of user functions called via call_user_function_array which assumes that DO_FCALL is the opcode of the previous call frame. This needs to be ensured and that extra check does it. Nice find!

[lavarou]

Here's a suggestion I have that will improve code review experience: apply a coding style with clang-format only to new code added to an existing code base. This will reduce 'background noise' and help reviewers keep focused on the new/changed code. Here's how this can be added to the code/commit workflow:

1. Disable automatic execution of clang-format on save in your editor!!!
2. Code like there's no tomorrow to add new features or fix bugs
3. git add
4. git clang-format
5. git status
6. git add <files formatted in step 4.>
7. git commit

This could be semi-automated with git hook: https://github.com/barisione/clang-format-hooks/#using-the-pre-commit-hook.

[lavarou]

Obsolete. Superseded by #708.