Skip to content

Commit 988edd2

Browse files
morrisonlevibwoebi
morrisonlevi
and
bwoebi
authored
Fix possible crash in end hook of traced closure (#3624)
* tests: add ASAN repro for eval parse error in internal fake closure hook Adds a PHPT that forces an eval() parse error inside an end-hook on an internal fake closure. Under ASAN this triggers a crash in ddtrace backtrace collection. * tests: extend ASAN repro with hook removal + re-entry Updates the internal fake closure ASAN reproducer to use two hooks and re-enter after removing one hook, while forcing eval() into the error path. * tests: reduce ASAN repro loop counts Reduces iterations and re-entry calls to the smallest values while preserving the ASAN crash. * tests: drop secondary hook from ASAN repro Removes the extra hook installation/removal while preserving the ASAN crash. * tests: reduce ASAN repro to single hook Drops the secondary closure and re-entry call while preserving the ASAN crash in ddtrace backtrace collection. * tests: reduce ASAN repro to single internal fake closure Simplifies the reproducer to a single internal fake closure with one hook, while forcing eval() into the error path and still crashing under ASAN. * tests: stop removing hook during ASAN repro The crash occurs during eval() error handling, so hook removal is unnecessary; keep the hook installed. * tests: drop begin hook and hook id from ASAN repro Passes no begin hook and removes unused hook-id plumbing, while still reproducing the ASAN SEGV in ddtrace backtrace collection. * tests: call internal fake closure directly in ASAN repro Removes the unnecessary callable indirection; the reduced test still deterministically triggers the ASAN SEGV in ddtrace backtrace collection. * tests: drop telemetry env from ASAN repro Removes the non-essential DD_INSTRUMENTATION_TELEMETRY_ENABLED environment section; the test still deterministically crashes under ASAN. * tests: make ASAN repro use valid eval that throws Replace the eval parse-error payload with valid PHP that throws an exception, and catch the exception at the call site. The ASAN SEGV in ddtrace backtrace collection still reproduces deterministically. * Cache scope for end hook as it may be invalidated before Signed-off-by: Bob Weinand <[email protected]> --------- Signed-off-by: Bob Weinand <[email protected]> Co-authored-by: Bob Weinand <[email protected]>
1 parent 057d031 commit 988edd2

4 files changed

Lines changed: 48 additions & 12 deletions

File tree

‎ext/hook/uhook.c‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ typedef struct {
7474

7575
typedef struct {
7676
dd_hook_data *hook_data;
77+
zend_class_entry *called_scope;
7778
} dd_uhook_dynamic;
7879

7980
#if PHP_VERSION_ID < 70400
@@ -234,14 +235,14 @@ void dd_uhook_report_sandbox_error(zend_execute_data *execute_data, zend_object
234235
})
235236
}
236237

237-
static bool dd_uhook_call_hook(zend_execute_data *execute_data, dd_uhook_callback *callback, dd_hook_data *hook_data) {
238+
static bool dd_uhook_call_hook(zend_execute_data *execute_data, dd_uhook_callback *callback, dd_hook_data *hook_data, zend_class_entry *scope) {
238239
zval hook_data_zv;
239240
ZVAL_OBJ(&hook_data_zv, &hook_data->std);
240241

241242
zval rv;
242243
zai_sandbox sandbox;
243244
zai_sandbox_open(&sandbox);
244-
dd_uhook_callback_ensure_scope(callback, execute_data);
245+
dd_uhook_callback_ensure_scope(callback, execute_data, scope);
245246
zend_fcall_info fci = dd_fcall_info(1, &hook_data_zv, &rv);
246247
bool success = zai_sandbox_call(&sandbox, &fci, &callback->fcc);
247248
if (!success || PG(last_error_message)) {
@@ -321,6 +322,7 @@ static bool dd_uhook_begin(zend_ulong invocation, zend_execute_data *execute_dat
321322
return true;
322323
}
323324

325+
dyn->called_scope = zend_get_called_scope(execute_data);
324326
dyn->hook_data = (dd_hook_data *)dd_hook_data_create(ddtrace_hook_data_ce);
325327
dyn->hook_data->returns_reference = execute_data->func->common.fn_flags & ZEND_ACC_RETURN_REFERENCE;
326328
dyn->hook_data->vm_stack_top = EG(vm_stack_top);
@@ -356,7 +358,7 @@ static bool dd_uhook_begin(zend_ulong invocation, zend_execute_data *execute_dat
356358
LOGEV(HOOK_TRACE, dd_uhook_log_invocation(log, execute_data, "begin", def->begin.closure););
357359

358360
def->running = true;
359-
dd_uhook_call_hook(execute_data, &def->begin, dyn->hook_data);
361+
dd_uhook_call_hook(execute_data, &def->begin, dyn->hook_data, dyn->called_scope);
360362
def->running = false;
361363
dyn->hook_data->retval_ptr = NULL;
362364
}
@@ -480,7 +482,7 @@ static void dd_uhook_end(zend_ulong invocation, zend_execute_data *execute_data,
480482
def->running = true;
481483
dyn->hook_data->retval_ptr = retval;
482484
dyn->hook_data->execute_data = execute_data;
483-
keep_span = dd_uhook_call_hook(execute_data, &def->end, dyn->hook_data);
485+
keep_span = dd_uhook_call_hook(execute_data, &def->end, dyn->hook_data, dyn->called_scope);
484486
dyn->hook_data->execute_data = NULL;
485487
dyn->hook_data->retval_ptr = NULL;
486488
def->running = false;

‎ext/hook/uhook.h‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,14 +22,12 @@ void zai_uhook_minit(int module_number);
2222
void zai_uhook_mshutdown();
2323

2424
void dd_uhook_callback_apply_scope(dd_uhook_callback *cb, zend_class_entry *scope);
25-
static inline void dd_uhook_callback_ensure_scope(dd_uhook_callback *cb, zend_execute_data *execute_data) {
26-
zend_class_entry *scope;
25+
// Note that we cannot access zend_get_called_scope(execute_data) here - we need to have it provided from earlier, it might have been invalidated by now, e.g. in ZEND_NAMED_FUNCTION(zend_closure_internal_handler).
26+
static inline void dd_uhook_callback_ensure_scope(dd_uhook_callback *cb, zend_execute_data *execute_data, zend_class_entry *scope) {
2727
if (!cb->fcc.function_handler) {
28-
scope = zend_get_called_scope(execute_data);
2928
goto apply_scope;
3029
} else if (!cb->is_static) {
3130
bool has_this;
32-
scope = zend_get_called_scope(execute_data);
3331
if (scope != cb->fcc.called_scope) {
3432
apply_scope:
3533
dd_uhook_callback_apply_scope(cb, scope);

‎ext/hook/uhook_legacy.c‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ typedef struct {
2424
typedef struct {
2525
zend_array *args;
2626
ddtrace_span_data *span;
27+
zend_class_entry *called_scope;
2728
bool skipped;
2829
bool dropped_span;
2930
bool was_primed;
@@ -35,7 +36,7 @@ static bool dd_uhook_call(dd_uhook_callback *callback, bool tracing, dd_uhook_dy
3536

3637
#define ZVAL_EXCEPTION(zv) do { if (EG(exception)) ZVAL_OBJ(zv, EG(exception)); else ZVAL_NULL(zv); } while (0)
3738
if (tracing) {
38-
dd_uhook_callback_ensure_scope(callback, execute_data);
39+
dd_uhook_callback_ensure_scope(callback, execute_data, dyn->called_scope);
3940

4041
ZVAL_OBJ(&params[0], &dyn->span->std);
4142
ZVAL_ARR(&params[1], dyn->args);
@@ -62,9 +63,8 @@ static bool dd_uhook_call(dd_uhook_callback *callback, bool tracing, dd_uhook_dy
6263
ZVAL_COPY_VALUE(&params[0], This);
6364
callback->fcc.object = Z_OBJ_P(This);
6465
}
65-
zend_class_entry *scope_ce = zend_get_called_scope(execute_data);
66-
if (scope_ce) {
67-
ZVAL_STR(&params[1], scope_ce->name);
66+
if (dyn->called_scope) {
67+
ZVAL_STR(&params[1], dyn->called_scope->name);
6868
} else {
6969
ZVAL_NULL(&params[1]);
7070
}
@@ -108,6 +108,7 @@ static bool dd_uhook_begin(zend_ulong invocation, zend_execute_data *execute_dat
108108
dyn->skipped = false;
109109
dyn->was_primed = false;
110110
dyn->dropped_span = false;
111+
dyn->called_scope = zend_get_called_scope(execute_data);
111112
dyn->args = dd_uhook_collect_args(execute_data);
112113

113114
if (def->tracing) {

‎tests/ext/sandbox/install_hook/internal_fake_closure_forced_parse_error_asan.phpt‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
--TEST--
2+
ASAN repro: internal fake closure + forced eval parse error
3+
--SKIPIF--
4+
<?php
5+
if (PHP_VERSION_ID < 80000) {
6+
die('skip: test requires PHP 8+');
7+
}
8+
?>
9+
--INI--
10+
datadog.trace.generate_root_span=0
11+
datadog.trace.auto_flush_enabled=0
12+
--FILE--
13+
<?php
14+
$closure = (new ReflectionFunction("intval"))->getClosure();
15+
16+
\DDTrace\install_hook(
17+
$closure,
18+
null,
19+
function () {
20+
eval('throw new \\Exception("boom");');
21+
},
22+
\DDTrace\HOOK_INSTANCE
23+
);
24+
25+
try {
26+
$closure(1);
27+
} catch (Throwable $e) {
28+
// ignore
29+
}
30+
31+
echo "ok\n";
32+
?>
33+
--EXPECT--
34+
ok
35+

0 commit comments

Comments
 (0)