Handle extra arguments for internal overrideArgs (#2348)

* Handle extra arguments for internal overrideArgs Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> * Fix possible leak in zai_call_function * Fix compilation Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> * Fix scope in PDOIntegration Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> --------- Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> Co-authored-by: Gustavo Lopes <mail@geleia.net>
DataDog · Nov 8, 2023 · a1f35f6 · a1f35f6
1 parent b046038
commit a1f35f6
Show file tree

Hide file tree

Showing 6 changed files with 96 additions and 16 deletions.
diff --git a/ext/compatibility.h b/ext/compatibility.h
@@ -280,6 +280,8 @@ static zend_always_inline void zend_array_release(zend_array *array)
         }
     }
 }
+
+#define ZEND_ARG_SEND_MODE(arg_info) (arg_info)->pass_by_reference
 #endif
 
 #if PHP_VERSION_ID < 80100

diff --git a/ext/hook/uhook.c b/ext/hook/uhook.c
@@ -54,6 +54,7 @@ typedef struct {
     zval property_exception;
     zend_ulong invocation;
     zend_execute_data *execute_data;
+    zval *vm_stack_top;
     zval *retval_ptr;
     ddtrace_span_data *span;
     ddtrace_span_stack *prior_stack;
@@ -223,6 +224,7 @@ static bool dd_uhook_begin(zend_ulong invocation, zend_execute_data *execute_dat
     }
 
     dyn->hook_data = (dd_hook_data *)dd_hook_data_create(ddtrace_hook_data_ce);
+    dyn->hook_data->vm_stack_top = EG(vm_stack_top);
 
     dyn->hook_data->invocation = invocation;
     ZVAL_LONG(&dyn->hook_data->property_id, def->id);
@@ -656,6 +658,16 @@ ZEND_METHOD(DDTrace_HookData, overrideArguments) {
         RETURN_FALSE;
     }
 
+    if (!ZEND_USER_CODE(func->type) && (int)zend_hash_num_elements(args) > passed_args) {
+        if (zend_hash_num_elements(args) - passed_args > hookData->vm_stack_top - ZEND_CALL_VAR_NUM(hookData->execute_data, 0)) {
+            RETURN_FALSE;
+        }
+#if PHP_VERSION_ID >= 80200
+        // temporaries like the last observed frame are already initialized. Move them.
+        memmove(ZEND_CALL_VAR_NUM(hookData->execute_data, zend_hash_num_elements(args)), ZEND_CALL_VAR_NUM(hookData->execute_data, passed_args), func->common.T * sizeof(zval *));
+#endif
+    }
+
     if (func->common.required_num_args > zend_hash_num_elements(args)) {
         LOG_LINE_ONCE(Error, "Not enough args provided for hook");
         RETURN_FALSE;
@@ -684,15 +696,27 @@ ZEND_METHOD(DDTrace_HookData, overrideArguments) {
 
     // When observers are executed, moving extra args behind the last temporary already happened
     zval *arg = ZEND_CALL_VAR_NUM(hookData->execute_data, 0), *last_arg = ZEND_USER_CODE(func->type) ? arg + func->common.num_args : ((void *)~0);
-    zval *val;
+    zval *val, zv;
     int i = 0;
     ZEND_HASH_FOREACH_VAL(args, val) {
         if (arg >= last_arg) {
             // extra-arg handling
             arg = ZEND_CALL_VAR_NUM(hookData->execute_data, func->op_array.last_var + func->op_array.T);
             last_arg = (void *)~0;
         }
-        if (i++ < passed_args || Z_TYPE_P(arg) != IS_UNDEF) {
+        if (i < (int)func->common.num_args && func->common.arg_info && (ZEND_ARG_SEND_MODE(&func->common.arg_info[i]) & ZEND_SEND_BY_REF) && !Z_ISREF_P(val)) {
+            Z_TRY_ADDREF_P(val); // copying into the ref
+            ZVAL_NEW_REF(&zv, val);
+            Z_DELREF_P(&zv); // we'll copy it right below
+            val = &zv;
+        }
+#if PHP_VERSION_ID < 80000
+        // While the observer API, triggers immediately after args passing, on PHP 7 the interceptor only triggers after all args have been parsed
+        // This only applied to user functions: in internal functions it's directly from zend_execute_internal
+        if (i++ < (ZEND_USER_CODE(func->type) ? MAX(passed_args, (int)func->common.num_args) : passed_args)) {
+#else
+        if (i++ < passed_args) {
+#endif
             zval garbage;
             ZVAL_COPY_VALUE(&garbage, arg);
             ZVAL_COPY(arg, val);

diff --git a/src/Integrations/Integrations/PDO/PDOIntegration.php b/src/Integrations/Integrations/PDO/PDOIntegration.php
@@ -273,7 +273,7 @@ private static function parseDsn($dsn)
         return $tags;
     }
 
-    private static function injectDBIntegration($pdo, $hook)
+    public static function injectDBIntegration($pdo, $hook)
     {
         $driver = $pdo->getAttribute(\PDO::ATTR_DRIVER_NAME);
         if ($driver === "odbc") {

diff --git a/tests/ext/sandbox/install_hook/replace_hook_args_internal.phpt b/tests/ext/sandbox/install_hook/replace_hook_args_internal.phpt
@@ -0,0 +1,35 @@
+--TEST--
+Adding additional function arguments on internal functions via install_hook()
+--FILE--
+<?php
+
+$hook = DDTrace\install_hook("preg_replace_callback_array", function($hook) {
+    $args = $hook->args;
+    $args[] = 2;
+    $args[] = null;
+    $hook->overrideArguments($args);
+});
+
+var_dump(preg_replace_callback_array(["((a))" => function () { var_dump(func_get_args()); }], "ababab"));
+
+?>
+--EXPECT--
+array(1) {
+  [0]=>
+  array(2) {
+    [0]=>
+    string(1) "a"
+    [1]=>
+    string(1) "a"
+  }
+}
+array(1) {
+  [0]=>
+  array(2) {
+    [0]=>
+    string(1) "a"
+    [1]=>
+    string(1) "a"
+  }
+}
+string(4) "bbab"
diff --git a/zend_abstract_interface/hook/hook.c b/zend_abstract_interface/hook/hook.c
@@ -862,6 +862,11 @@ zai_hook_continued zai_hook_continue(zend_execute_data *ex, zai_hook_memory_t *m
         return ZAI_HOOK_SKIP;
     }
 
+    // Ensure there's a bit of space in case arguments are going to be overridden.
+    if (!ZEND_USER_CODE(ex->func->type) && ZEND_CALL_NUM_ARGS(ex) < ex->func->common.num_args) {
+        EG(vm_stack_top) = MIN(EG(vm_stack_end), EG(vm_stack_top) + ex->func->common.num_args - ZEND_CALL_NUM_ARGS(ex));
+    }
+
     size_t hook_info_size = allocated_hook_count * sizeof(zai_hook_info);
     size_t dynamic_size = hooks->dynamic + hook_info_size;
     // a vector of first N hook_info entries, then N entries of variable size (as much memory as the individual hooks require)

diff --git a/zend_abstract_interface/symbols/call.c b/zend_abstract_interface/symbols/call.c
@@ -75,6 +75,16 @@ static zend_execute_data *zai_set_observed_frame(zend_execute_data *execute_data
 }
 #endif
 
+static inline int zai_symbol_try_call(zend_fcall_info *fci, zend_fcall_info_cache *fcc) {
+    volatile int ret;
+    zend_try {
+        ret = zend_call_function(fci, fcc);
+    } zend_catch {
+        ret = 2;
+    } zend_end_try();
+    return ret;
+}
+
 /* {{{ private call code */
 bool zai_symbol_call_impl(
     // clang-format off
@@ -172,11 +182,11 @@ bool zai_symbol_call_impl(
     }
 
     // clang-format off
-    volatile int  zai_symbol_call_result    = FAILURE;
-    volatile bool zai_symbol_call_bailed    = false;
-    volatile bool rebound_closure = false;
-    volatile zval new_closure;
-    zend_op_array *volatile op_array;
+    int  zai_symbol_call_result    = FAILURE;
+    bool zai_symbol_call_bailed    = false;
+    bool rebound_closure = false;
+    zval new_closure;
+    zend_op_array *op_array;
 
     if (function_type == ZAI_SYMBOL_FUNCTION_CLOSURE && fcc.called_scope) {
         zend_class_entry *closure_called_scope;
@@ -216,7 +226,7 @@ bool zai_symbol_call_impl(
 #if PHP_VERSION_ID >= 70400
                 op_array->fn_flags |= ZEND_ACC_HEAP_RT_CACHE;
 #if PHP_VERSION_ID >= 80200
-                void *ptr = emalloc(op_array->cache_size);
+                void *ptr = emalloc((size_t)op_array->cache_size);
                 ZEND_MAP_PTR_INIT(op_array->run_time_cache, ptr);
 #else
                 void *ptr = emalloc(op_array->cache_size + sizeof(void *));
@@ -242,7 +252,11 @@ bool zai_symbol_call_impl(
         for (uint32_t arg = 0; arg < argc; arg++) {
             zval *param = va_arg(*args, zval *);
 
-            ZVAL_COPY_VALUE(&fci.params[arg], param);
+            // zend_call_function may change fci.params by replacing some
+            // parameters with references. To detect this and later free the
+            // references, we need to increase the refcount here and call
+            // zval_ptr_dtor() for each parameter after the call.
+            ZVAL_COPY(&fci.params[arg], param);
         }
         fci.param_count = argc;
     }
@@ -251,14 +265,14 @@ bool zai_symbol_call_impl(
     zend_execute_data *prev_observed = zai_set_observed_frame(NULL);
 #endif
 
-    zend_try {
-        zai_symbol_call_result =
-            zend_call_function(&fci, &fcc);
-    } zend_catch {
-        zai_symbol_call_bailed = true;
-    } zend_end_try();
+    zai_symbol_call_result = zai_symbol_try_call(&fci, &fcc);
+    zai_symbol_call_bailed = zai_symbol_call_result == 2;
     // clang-format on
 
+    for (uint32_t arg = 0; arg < argc; arg++) {
+        zval_ptr_dtor(&fci.params[arg]);
+    }
+
     if (zai_symbol_call_bailed) {
         zai_sandbox_bailout(&sandbox);
 #if PHP_VERSION_ID >= 80000