From 94c04a3d1fc3ecdb949a91aa5ffbb839bf7a0d2e Mon Sep 17 00:00:00 2001 From: Zach Neumann Date: Fri, 28 Jul 2023 12:01:22 -0600 Subject: [PATCH 1/3] fix: add guards to oapi cufa opcode check --- agent/php_execute.c | 35 +++++++++++++++---- .../test_wordpress_do_action.php8.php | 10 ++++++ 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/agent/php_execute.c b/agent/php_execute.c index 18465ceb0..f9bbb07bb 100644 --- a/agent/php_execute.c +++ b/agent/php_execute.c @@ -1934,6 +1934,7 @@ static void nr_php_observer_attempt_call_cufa_handler(NR_EXECUTE_PROTO) { nrl_verbosedebug(NRL_AGENT, "%s: cannot get previous execute data", __func__); return; } + if (NULL == execute_data->prev_execute_data->opline) { nrl_verbosedebug(NRL_AGENT, "%s: cannot get previous opline", __func__); return; @@ -1966,20 +1967,40 @@ static void nr_php_observer_attempt_call_cufa_handler(NR_EXECUTE_PROTO) { * cause additional performance overhead, this should be considered a last * resort. */ - const zend_op* prev_opline = execute_data->prev_execute_data->opline - 1; + + /* + * We cannot safely access the opline of internal functions, and we only + * want to instrument cufa calls from user functions anyway + */ + if (UNEXPECTED(NULL == execute_data->prev_execute_data->func)) { + nrl_verbosedebug(NRL_AGENT, "%s: cannot get previous function", __func__); + return; + } + if (!ZEND_USER_CODE(execute_data->prev_execute_data->func->type)) { + nrl_verbosedebug(NRL_AGENT, "%s: caller is php internal function", __func__); + return; + } + + const zend_op* prev_opline = execute_data->prev_execute_data->opline; + + /* + * Extra safety check. Previously, we instrumented by overwritting ZEND_DO_FCALL. + * Within OAPI, for consistencies sake, we will ensure consistency here + */ + if (ZEND_DO_FCALL != prev_opline->opcode) { + nrl_verbosedebug(NRL_AGENT, "%s: cannot get previous function name", __func__); + return; + } + prev_opline -= 1; // Checks previous opcode if (ZEND_CHECK_UNDEF_ARGS == prev_opline->opcode) { - prev_opline = execute_data->prev_execute_data->opline - 2; + prev_opline -= 1; // Checks previous opcode } if (ZEND_SEND_ARRAY == prev_opline->opcode) { + if (UNEXPECTED((NULL == execute_data->func))) { nrl_verbosedebug(NRL_AGENT, "%s: cannot get current function", __func__); return; } - if (UNEXPECTED(NULL == execute_data->prev_execute_data->func)) { - nrl_verbosedebug(NRL_AGENT, "%s: cannot get previous function", __func__); - return; - } - if (UNEXPECTED(NULL == execute_data->prev_execute_data->func->common.function_name)) { nrl_verbosedebug(NRL_AGENT, "%s: cannot get previous function name", __func__); return; diff --git a/tests/integration/frameworks/wordpress/test_wordpress_do_action.php8.php b/tests/integration/frameworks/wordpress/test_wordpress_do_action.php8.php index a24492dff..56280eaad 100644 --- a/tests/integration/frameworks/wordpress/test_wordpress_do_action.php8.php +++ b/tests/integration/frameworks/wordpress/test_wordpress_do_action.php8.php @@ -21,6 +21,7 @@ */ /*EXPECT +g f h g @@ -70,4 +71,13 @@ function f() { } } +/* + * Initiates a non-flattened call stack of internal->user_code + * to ensure that cufa instrumentation properly handles skipping + * opline lookups of internal functions + */ +$function = new ReflectionFunction('g'); +$function->invoke(); + + do_action("f"); From f56cbb0bae9a15cf5bee44e0b7e341973458ea20 Mon Sep 17 00:00:00 2001 From: ZNeumann Date: Thu, 3 Aug 2023 10:14:26 -0600 Subject: [PATCH 2/3] docs: comment clarity --- agent/php_execute.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/php_execute.c b/agent/php_execute.c index f9bbb07bb..ff7297fd9 100644 --- a/agent/php_execute.c +++ b/agent/php_execute.c @@ -1985,7 +1985,7 @@ static void nr_php_observer_attempt_call_cufa_handler(NR_EXECUTE_PROTO) { /* * Extra safety check. Previously, we instrumented by overwritting ZEND_DO_FCALL. - * Within OAPI, for consistencies sake, we will ensure consistency here + * Within OAPI, for consistency's sake, we will ensure the same */ if (ZEND_DO_FCALL != prev_opline->opcode) { nrl_verbosedebug(NRL_AGENT, "%s: cannot get previous function name", __func__); From 0b4d8db87ec9cc4c4be0f8c97f35b8c72e930837 Mon Sep 17 00:00:00 2001 From: ZNeumann Date: Tue, 8 Aug 2023 13:16:30 -0600 Subject: [PATCH 3/3] Update agent/php_execute.c Co-authored-by: Michal Nowacki --- agent/php_execute.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/agent/php_execute.c b/agent/php_execute.c index ff7297fd9..a0ee83ed2 100644 --- a/agent/php_execute.c +++ b/agent/php_execute.c @@ -1969,8 +1969,11 @@ static void nr_php_observer_attempt_call_cufa_handler(NR_EXECUTE_PROTO) { */ /* - * We cannot safely access the opline of internal functions, and we only - * want to instrument cufa calls from user functions anyway + * When Observer API is used, this code executes in the context of + * zend_execute and not in the context of VM (as was the case pre-OAPI), + * therefore we need to ensure we're dealing with a user function. We cannot + * safely access the opline of internal functions, and we only want to + * instrument cufa calls from user functions anyway. */ if (UNEXPECTED(NULL == execute_data->prev_execute_data->func)) { nrl_verbosedebug(NRL_AGENT, "%s: cannot get previous function", __func__);