fix(agent): add guards to oapi cufa opcode check (#708)

newrelic · Aug 8, 2023 · 1de4976 · 1de4976
1 parent 8cbe130
commit 1de4976
Show file tree

Hide file tree

Showing 2 changed files with 38 additions and 7 deletions.
diff --git 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 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__);
+    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
@@ -21,6 +21,7 @@
 */
 
 /*EXPECT
+g
 f
 h
 g
@@ -74,4 +75,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");